💬 jonatack commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816776909)
> I'm sorry, what? For what reason would we not use the logic we already have to catch (some/most) of these format errors at compile-time, and defer them to run-time instead, relying on manual review to make sure each code-path is ran, potentially crashing user software?

> relying on manual review to make sure each code-path is ran, potentially crashing user software

Because, as I wrote:

- I think the run-time error is more clear to understand, as the person working on the code
- This
...

💬 jonatack commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816781445)
> > I'd prefer avoiding the churn.
>
> We usually call things churn that change things multiple times within the same PR. Indenting the lines below a changed line that belong to the same logical unit is a good thing in my eyes. If we would not do that anywhere, the code would look weird and probably make `clang-format` unusable after a while.

You're changing those lines needlessly. Whether you prefer to call it noise or churn is unimportant.

💬 jonatack commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816785037)
@TheCharlatan mind being explicit what part of my feedback your emoji relates to?

💬 stickies-v commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816788216)
> I think the run-time error is more clear to understand, as the person working on the code

And I think prioritizing devs getting nicer error messages over increased software run-time stability is almost never a good trade-off. It seems we have fundamentally different views, so I'll leave this thread unresolved but I strongly disagree with your approach and won't change the PR because of it.

> This code path is manually tested anyway, otherwise the command with any non-zero level won't run
...

📝 dergoegge opened a pull request: "bench: Remove various extraneous benchmarks"
(https://github.com/bitcoin/bitcoin/pull/31153)
Measuring performance of components that do not meaningfully impact the performance of critical aspects of the system (e.g. block validation or mining) seems extraneous.

Maintaining extraneous benchmark tests incentivizes non-impactful improvements. This frequently comes up with new contributors and they can't be blamed for thinking that e.g. hex parsing should be made faster if a benchmark test exists.

💬 TheCharlatan commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816806742)
> mind being explicit what part of my feedback your emoji relates to?

Sure

> I haven't found this kind of error to be an issue for me over the past half decade on this code.

I've seen a bunch of positional args errors in fairly recent times, and catching them at compile time is certainly worthwhile. People tried doing this with various linters, but the linters produced false-positives, or missed certain cases.

🤔 marcofleon reviewed a pull request: "Introduce `g_fuzzing` global for fuzzing checks"
(https://github.com/bitcoin/bitcoin/pull/31093#pullrequestreview-2395527351)
Tested ACK 9f243cd7fa6654e3b71ba6bff82cceed547c5d53

Reviewed the code, which changes the fuzzing check from compile time to run time. This should work for our purposes and is a reasonable solution to [#30950](https://github.com/bitcoin/bitcoin/issues/30950). It's better than [#31028](https://github.com/bitcoin/bitcoin/pull/31028), which I'll close if this gets merged.

Built with `BUILD_FUZZ_BINARY` (without `BUILD_FOR_FUZZING`) and ran the `p2p_headers_presync` target on a corpus that I ha
...

💬 jonatack commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816826831)
> And I think prioritizing devs getting nicer error messages over increased software run-time stability is almost never a good trade-off. It seems we have fundamentally different views

I think we agree there, given that you wrote "almost" ;)

🤔 stickies-v reviewed a pull request: "bench: Remove various extraneous benchmarks"
(https://github.com/bitcoin/bitcoin/pull/31153#pullrequestreview-2395573594)
Concept ACK. Perhaps also worth updating `doc/benchmarking.md` with a brief section on which benchmarks are typically useful to add, and which benchmarks should rather be ephemeral (e.g. provided on a separate branch, supplementary to a PR)?

💬 rkrux commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1816815761)
For my understanding, can you explain why is there a difference b/w `mempool_entry_minrate ` and `mempoolmin_feerate ` currently as `mempool_entry_minrate ` is calculated based on the current mempool entries that don't include the to be added parents and child?
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mempool.cpp#L703

💬 rkrux commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1816851072)
"If trimming of a parent were to happen,
# package evaluation would happen to reintrodce the evicted parent"

Out of curiosity: Is the only way for us to know if trimming happened is through debug logs?

💬 rkrux commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1816843302)
Aah I see, the size of `big_parent_txids ` is only 3 - I guess it won't be an issue perf wise.

💬 rkrux commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1816765913)
Typo `reintrodce`

👍 rkrux approved a pull request: "functional test: Additional package evaluation coverage"
(https://github.com/bitcoin/bitcoin/pull/31152#pullrequestreview-2395429428)
tACK f32c34d0c3d4041a301822b27e88d6db4cbf631e

Successful make and functional tests. In my system, 1 out of the 72 txs in the mempool was evicted when this package of 3 parents and 1 child was submitted.

Thanks for adding this one, it is a fantastic functional test to have that tests different pieces, I enjoyed going through it. Asked couple questions for my understanding.

📝 fanquake opened a pull request: "[27.x] rc2 or final"
(https://github.com/bitcoin/bitcoin/pull/31154)
Not expecting much more here. This backports one other change (that doesn't warrant an rc), which fixes noisey output from newer versions of Clang (19+). If no other changes are needed/added, I'll push the changes for final.

🤔 jonatack reviewed a pull request: "bench: Remove various extraneous benchmarks"
(https://github.com/bitcoin/bitcoin/pull/31153#pullrequestreview-2395650389)
Not sure about some of these. People can be asked to add benchmarks for performance improvements or to verify that a change to existing code doesn't degrade performance, and having existing benchmarks to refer to may make that easier. Seems odd to throw out work like https://github.com/bitcoin/bitcoin/pull/22284.

Are any of the benchmarks called by outside sites that track their results over time?

💬 jonatack commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1816916312)
Not sure about removing the logging benchmarks. Logging can be very high frequency. Even if that is more likely to occur for developers, it may be useful to be able to track this.

💬 fanquake commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2438144997)
Concept ACK - agree with stickies-v re updating the doc.

💬 instagibbs commented on pull request "rpc: getorphantxs follow-up":
(https://github.com/bitcoin/bitcoin/pull/31043#issuecomment-2438159398)
> Is the preference to reject undefined verbosity values for new code (e.g. -1, 5)? If so, we can adjust that and any future verbosity levels would be accompanied by updated test(s).

Rejection now means we can expand usage later if desired, and is more obvious what's happening.

💬 instagibbs commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1816951913)
A gap can/does happen naturally in that the highest descendant feerate evicted, plus minrelay(1 sat/vbyte) becomes the new floating fee.

I guess `fill_mempool` does it in such a way that all the leftover transactions are higher than the new minrelay?

I'll take a longer look into seeing if we can guarantee this more directly somehow