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

💬 PastaPastaPasta commented on pull request "refactor: convert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1847043402)
I'd like to see another concept ACK to removing the comment I think before I do so, if someone else thinks it's valuable to drop the comment, I will.