💬 maflcko commented on pull request "refactor: Fix remaining clang-tidy performance-inefficient-vector errors":
(https://github.com/bitcoin/bitcoin/pull/31305#discussion_r1846855789)
This seems backwards? Fuzz engines will generally prefer smaller fuzz inputs over larger ones, given identical coverage metrics. Optimizing for the worst case, which will likely never happen, and possibly pessimising the happy case reads odd.

Either this has no effect at all on the fuzz target anyway, in which case the code reads odd, or this is actually pessimising.

💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#issuecomment-2483491305)
Renamed to `coinbase_output_script`, added warning and discouragement, fixed fuzz binary build.

💬 flack commented on pull request "refactor: convert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1846892284)
Nit: double semicolon

💬 PastaPastaPasta commented on pull request "refactor: convert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1846901519)
resolved both!

💬 TheCharlatan commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1846908145)
I'm confused, this is printing `Script verification uses 15 additional threads` for both master and this commit when I provide `-par=16`. As far as I can tell the logic as applied is:
```
worker_threads_num = script_threads - 1;
worker_threads_num = std::clamp(worker_threads_num, 0 , 15);
```
Which should be equivalent?

💬 furszy commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1846928649)
Yeah, never mind. The subtraction happens before the clamping, not after as I wrote above. So the behavior remains the same.
I guess I'm a bit sleepy today.. sorry.

💬 maflcko commented on issue "bitcoin-qt failed assertion on startup":
(https://github.com/bitcoin/bitcoin/issues/31289#issuecomment-2483574310)
A fix would be to revert da73f1513a6, but that adds another bug? Also, I presume it doesn't happen in multiprocess?

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1846954861)
Thanks, done!

💬 l0rinc commented on pull request "refactor: Fix remaining clang-tidy performance-inefficient-vector errors":
(https://github.com/bitcoin/bitcoin/pull/31305#discussion_r1846966750)
Are you saying this will affect fuzzing behavior? Does it look under the hood to check how many items to create? I wasn't aware of that and find it odd.
What value do you recommend here instead of the worst case?

🤔 l0rinc requested changes to a pull request: "refactor: convert ContainsNoNUL to ContainsNUL"
(https://github.com/bitcoin/bitcoin/pull/31301#pullrequestreview-2443242297)
Based on https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1846027455, to set your expectations, this likely won't be merged (we can just postpone the fix until C++23, we're not in a rush), but I don't mind reviewing anyway.
Please update the description after every change.

💬 l0rinc commented on pull request "refactor: convert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1846969549)
the comment just repeats what the code already states clearly, I'd just remove it

💬 l0rinc commented on pull request "refactor: convert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1846970477)
what was the side effect and why is that important? Calling this without the return value would just be removed as dead code.

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1846977562)
I've discussed this question with @sipa a bit; our thought was that changesets are for evaluating changes that we might need to back out and not apply; removals due to a block being found or to make the mempool consistent after a reorg have to happen, so we don't bother with a changeset and just apply them directly.

If you have other thoughts on this let me know-- happy to reconsider if there's a good reason to structure removals differently.

👍 tdb3 approved a pull request: "test: Revert to random path element"
(https://github.com/bitcoin/bitcoin/pull/31317#pullrequestreview-2443254703)
code review and light test ACK fa80b08fef0eaa600339caa678fdf80a8aec3ce3

Ran unit tests and benchmarks, and saw that the dirs generated were random.

👍 theStack approved a pull request: "Safegcd-based modular inverses in MuHash3072"
(https://github.com/bitcoin/bitcoin/pull/21590#pullrequestreview-2443257294)
Fuzz-tested ACK ad67fd2e0bfa6f43f350066596b6cca146391362

With the friendly help of @dergoegge I managed to get differential fuzzing running last week and let that ran for the last ~77 hours. Here are the rough instructions for those who also want to give it a try:

<details>
<summary>Instructions for differential fuzzing using semsan</summary>

1. created branches on top of master and the PR each that add a _characterization_ to the MuHash fuzz test, writing to a shared memory for comp
...

💬 PastaPastaPasta commented on pull request "refactor: convert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1846980244)
> Calling this without the return value would just be removed as dead code.

Yes, it almost certainly would; but with [[nodiscard]] it simply won't compile. This is better as having some code such that calls ContainsNUL but doesn't use it, must be a bug, as there is no purpose to call it, and not use it.

💬 PastaPastaPasta commented on pull request "refactor: convert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#issuecomment-2483675312)
> Based on [#31301 (comment)](https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1846027455), to set your expectations, this likely won't be merged (we can just postpone the fix until C++23, we're not in a rush)

I would argue against this mentality :) I can't tell for sure, but my guess is requiring c++23 is at least a year away from being merged? Makes more sense imo, to merge this in, and then once c++23 is available, update this code. (Also, I try to do PRs like this to move towards
...

💬 PastaPastaPasta commented on pull request "refactor: convert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1846996998)
I would disagree, this way IDEs can show it as popup / doxygen can use it. Plus it is still accurate

👍 itornaza approved a pull request: "test: Add `leaf_version` parameter to `taproot_tree_helper()`"
(https://github.com/bitcoin/bitcoin/pull/29371#pullrequestreview-2443297256)
re ACK 6ffdabd4a00bf5578f975d0ed60c27adabf36c0b

💬 l0rinc commented on pull request "refactor: convert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1847041055)
Given that you forgot to update it, I'd say it doesn't have a big added value, let's just delete it.