💬 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?
(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.
(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?
(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!
(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?
(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.
(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
(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.
(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.
(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.
(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
...
(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.
(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
...
(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
(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
(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.
(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.
(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.
💬 brunoerg commented on pull request "test: addrman: tried 3 times and never a success so `isTerrible=true`":
(https://github.com/bitcoin/bitcoin/pull/30445#discussion_r1847043582)
Note that `IsTerrible` has other verifications (e.g. we don't remove things tried in the last minute). So the timing setting here is to explicity reach exactly what we want to test but could be any other value. Every time we call `Attempt`, `m_last_try` is updated. So, yes, you're right about the threshold. Worth adding a comment?
(https://github.com/bitcoin/bitcoin/pull/30445#discussion_r1847043582)
Note that `IsTerrible` has other verifications (e.g. we don't remove things tried in the last minute). So the timing setting here is to explicity reach exactly what we want to test but could be any other value. Every time we call `Attempt`, `m_last_try` is updated. So, yes, you're right about the threshold. Worth adding a comment?
💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-2483780319)
I'm using the ThreadPool here in https://github.com/bitcoin/bitcoin/pull/31132 as a cherry-picked commit, modulo changing `ThreadPool() {}` to `ThreadPool() = default;`. Perhaps we could pull this out to a separate PR since it would be useful for both changes.
One request for the ThreadPool would be to track in flight tasks being executed. That way we could write tests that ensure that all tasks have been completed before continuing, even if we don't have access to the futures.
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-2483780319)
I'm using the ThreadPool here in https://github.com/bitcoin/bitcoin/pull/31132 as a cherry-picked commit, modulo changing `ThreadPool() {}` to `ThreadPool() = default;`. Perhaps we could pull this out to a separate PR since it would be useful for both changes.
One request for the ThreadPool would be to track in flight tasks being executed. That way we could write tests that ensure that all tasks have been completed before continuing, even if we don't have access to the futures.
🤔 furszy reviewed a pull request: "refactor: Clamp worker threads in ChainstateManager constructor"
(https://github.com/bitcoin/bitcoin/pull/31313#pullrequestreview-2443430178)
Code ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f
(https://github.com/bitcoin/bitcoin/pull/31313#pullrequestreview-2443430178)
Code ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f