💬 TheCharlatan commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816753074)
> 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.

🤔 stickies-v reviewed a pull request: "util: Treat Assume as Assert when evaluating at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31150#pullrequestreview-2395423920)
Post-merge ACK fa69a5f4b76a4e2a02db6c32d9c3311ce5fe29bd - nice!

💬 stickies-v commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816767397)
> The rest of this PR seems fine.

We can't have the rest of this PR without updating `bitcoin-cli.cpp`, unless someone implements the complete format string validation logic, for which as per my earlier comment, see https://github.com/bitcoin/bitcoin/pull/30546.

💬 l0rinc commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1816775973)
Seems it was a [deliberate formatting](https://github.com/bitcoin/bitcoin/commit/efd2474d17098c754367b844ec646ebececc7c74#diff-73552341d85aec344497b47f1c2aa53a7e01a415030e40f56b1c454aef5f209dR221-R222), so I'll revert.
Will push after I have the block serialization benchmark working.

💬 l0rinc commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1816776293)
Seems a bit excessive for a test, but ended up changing it to e.g. `const size_t key_size{rng.randrange(static_cast<size_t>(10))};`. Will push a bit later.

💬 l0rinc commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1816776448)
Indeed!

🚀 fanquake merged a pull request: "fuzz: fuzz connman with non-empty addrman + ASMap"
(https://github.com/bitcoin/bitcoin/pull/29536)

💬 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`