💬 maflcko commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1846813317)
not sure about making this optional. Even if this is a dummy, it would be good to spell it out.
Using `std::optional` here is just making code more complex, adding two lines of code in one place to save one line of code in another place.
Also, it may be better to call mention *output* script in the name.
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1846813317)
not sure about making this optional. Even if this is a dummy, it would be good to spell it out.
Using `std::optional` here is just making code more complex, adding two lines of code in one place to save one line of code in another place.
Also, it may be better to call mention *output* script in the name.
💬 ryanofsky commented on pull request "wallet: Translate [default wallet] string in progress messages":
(https://github.com/bitcoin/bitcoin/pull/31296#issuecomment-2483435630)
re: https://github.com/bitcoin/bitcoin/pull/31296#issuecomment-2481190404
> What are the steps to test this with sqlite? If it isn't needed after the bdb removal, it can probably be skipped?
Not sure if this will always be the case, but it is currently possible to create an sqlite wallet with no name with `bitcoin-cli -regtest createwallet ""`.
Regardless of the details of the bug though, I think the real problem is that the `GetDisplayName()` method name suggests it should be used to g
...
(https://github.com/bitcoin/bitcoin/pull/31296#issuecomment-2483435630)
re: https://github.com/bitcoin/bitcoin/pull/31296#issuecomment-2481190404
> What are the steps to test this with sqlite? If it isn't needed after the bdb removal, it can probably be skipped?
Not sure if this will always be the case, but it is currently possible to create an sqlite wallet with no name with `bitcoin-cli -regtest createwallet ""`.
Regardless of the details of the bug though, I think the real problem is that the `GetDisplayName()` method name suggests it should be used to g
...
💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1846841935)
In production this is always a dummy. `CreateNewBlock()` sets the script to `OP_TRUE` and then the `getblocktemplate` RPC omits the whole coinbase transaction.
It's not safe for the caller to set an _arbitrary_ dummy, because IIUC if it's larger than `coinbase_max_additional_weight` the `TestBlockValidity` internal check for `getblocktemplate` will spuriously fail. Which is why I think it's better for `CreateNewBlock` to decide what the dummy should be.
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1846841935)
In production this is always a dummy. `CreateNewBlock()` sets the script to `OP_TRUE` and then the `getblocktemplate` RPC omits the whole coinbase transaction.
It's not safe for the caller to set an _arbitrary_ dummy, because IIUC if it's larger than `coinbase_max_additional_weight` the `TestBlockValidity` internal check for `getblocktemplate` will spuriously fail. Which is why I think it's better for `CreateNewBlock` to decide what the dummy should be.
💬 maflcko commented on pull request "test: Revert to random path element":
(https://github.com/bitcoin/bitcoin/pull/31317#discussion_r1846842029)
> loop
I am not sure that running tests in a loop until they pass is a good approach. Generally, when there is an issue, it should be addressed, instead of being silently hidden.
> Probably we might also need to lock the directory like we do for the custom datadir path.
Maybe a follow-up can do this? The goal of this pull request is to unbreak OSS-Fuzz (not sure if any other deployment is affected) by simply restoring what has been done for all previous releases in the unit tests.
Al
...
(https://github.com/bitcoin/bitcoin/pull/31317#discussion_r1846842029)
> loop
I am not sure that running tests in a loop until they pass is a good approach. Generally, when there is an issue, it should be addressed, instead of being silently hidden.
> Probably we might also need to lock the directory like we do for the custom datadir path.
Maybe a follow-up can do this? The goal of this pull request is to unbreak OSS-Fuzz (not sure if any other deployment is affected) by simply restoring what has been done for all previous releases in the unit tests.
Al
...
🤔 hebasto reviewed a pull request: "refactor: Fix remaining clang-tidy performance-inefficient-vector errors"
(https://github.com/bitcoin/bitcoin/pull/31305#pullrequestreview-2443044464)
Tested 01e54b7d82a053238b62121eeba2ac2c45b88859 on Ubuntu 24.04. The `clang-tidy-19` with the enabled "performance-inefficient-vector-operation" check is happy.
(https://github.com/bitcoin/bitcoin/pull/31305#pullrequestreview-2443044464)
Tested 01e54b7d82a053238b62121eeba2ac2c45b88859 on Ubuntu 24.04. The `clang-tidy-19` with the enabled "performance-inefficient-vector-operation" check is happy.
🤔 furszy reviewed a pull request: "test: Revert to random path element"
(https://github.com/bitcoin/bitcoin/pull/31317#pullrequestreview-2443054669)
Code ACK fa80b08fef0eaa600339caa678fdf80a8aec3ce3
(https://github.com/bitcoin/bitcoin/pull/31317#pullrequestreview-2443054669)
Code ACK fa80b08fef0eaa600339caa678fdf80a8aec3ce3
💬 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.
(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.
(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
(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!
(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?
(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.