💬 HowHsu commented on pull request "checkqueue: set MAX_SCRIPTCHECK_THREADS to nCores - 1":
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-2949835222)
> > I think par=N means N workers, plus one master worker which is actually the main bitcoind thread itself.
>
> That's not correct. The number of worker threads is one less than the argument to `-par`: https://github.com/bitcoin/bitcoin/blob/v29.0/src/node/chainstatemanager_args.cpp#L62.
I see, sorry for missing this part, but `MAX_SCRIPTCHECK_THREADS` is to limit `opt.worker_threads_num`, so it still should be `nCores-1`
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-2949835222)
> > I think par=N means N workers, plus one master worker which is actually the main bitcoind thread itself.
>
> That's not correct. The number of worker threads is one less than the argument to `-par`: https://github.com/bitcoin/bitcoin/blob/v29.0/src/node/chainstatemanager_args.cpp#L62.
I see, sorry for missing this part, but `MAX_SCRIPTCHECK_THREADS` is to limit `opt.worker_threads_num`, so it still should be `nCores-1`
💬 sipa commented on pull request "checkqueue: set MAX_SCRIPTCHECK_THREADS to nCores - 1":
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-2949838394)
@HowHsu I am so sorry! You are right.
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-2949838394)
@HowHsu I am so sorry! You are right.
👍 hebasto approved a pull request: "depends: fix multiprocess build on OpenBSD (apply capnp patch, correct SHA256SUM command)"
(https://github.com/bitcoin/bitcoin/pull/32690#pullrequestreview-2905521389)
ACK 8713e8060d504f561fed705b4aa5af7b96c36e75.
(https://github.com/bitcoin/bitcoin/pull/32690#pullrequestreview-2905521389)
ACK 8713e8060d504f561fed705b4aa5af7b96c36e75.
💬 hebasto commented on issue "depends: OpenBSD (aarch64) needs gcc (instruction) for libevent":
(https://github.com/bitcoin/bitcoin/issues/32691#issuecomment-2949888947)
This [patch](https://github.com/hebasto/bitcoin/commit/dfc6ea0f8a27c945a60c6ea0eb63e04b665d29ac) seems working.
cc @theStack
(https://github.com/bitcoin/bitcoin/issues/32691#issuecomment-2949888947)
This [patch](https://github.com/hebasto/bitcoin/commit/dfc6ea0f8a27c945a60c6ea0eb63e04b665d29ac) seems working.
cc @theStack
💬 marcofleon commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#issuecomment-2949906433)
ReACK 95969bc58ae0cd928e536d7cb8541de93e8c7205
Swtiched from `ConsumeTransaction` to `ConsumeDeserializable` since my last review. [Coverage](https://marcofleon.github.io/coverage/merkle/) looks good!
(https://github.com/bitcoin/bitcoin/pull/32243#issuecomment-2949906433)
ReACK 95969bc58ae0cd928e536d7cb8541de93e8c7205
Swtiched from `ConsumeTransaction` to `ConsumeDeserializable` since my last review. [Coverage](https://marcofleon.github.io/coverage/merkle/) looks good!
🤔 l0rinc reviewed a pull request: "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`"
(https://github.com/bitcoin/bitcoin/pull/32532#pullrequestreview-2904289512)
Thanks for the reviews, addressed more of the concerns in smaller commits, even more tests: https://github.com/bitcoin/bitcoin/compare/5b11b8706f14e0545f7e07c7c029e24811dc6c9d..0819a34565d01e241e5a3f884a508472861c96e1
@darosior, this change isn't about IBD, it was mostly about block connect time optimization originally - now it's rather meant as a cleanup and flooding it with tests and benchmarks to enable optimizations in follow-ups.
(https://github.com/bitcoin/bitcoin/pull/32532#pullrequestreview-2904289512)
Thanks for the reviews, addressed more of the concerns in smaller commits, even more tests: https://github.com/bitcoin/bitcoin/compare/5b11b8706f14e0545f7e07c7c029e24811dc6c9d..0819a34565d01e241e5a3f884a508472861c96e1
@darosior, this change isn't about IBD, it was mostly about block connect time optimization originally - now it's rather meant as a cleanup and flooding it with tests and benchmarks to enable optimizations in follow-ups.
💬 l0rinc commented on pull request "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2132580473)
Update: we do need those, but only for the compressor, not for the solver or other uses
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2132580473)
Update: we do need those, but only for the compressor, not for the solver or other uses
💬 l0rinc commented on pull request "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2131817342)
Update: same, those checks need to be on use site, in compressor only
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2131817342)
Update: same, those checks need to be on use site, in compressor only
💬 l0rinc commented on pull request "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2132373283)
Excellent, we can even split `ResetChecked` out of `CBlock::SetNull`
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2132373283)
Excellent, we can even split `ResetChecked` out of `CBlock::SetNull`
💬 l0rinc commented on pull request "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2132579545)
Indeed!
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2132579545)
Indeed!
💬 l0rinc commented on pull request "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2131816504)
thought about it, it seems more redundant (which I think could be safer for legacy code) if we state the sizes explicitly, otherwise changing the `CPubKey::COMPRESSED_SIZE` value might not fail immediately. Every other helper here has fixed size - and I couldn't find a reasonable value for all of them, decided to hard-code the sizes instead.
Do you have an idea that we could apply to all of these?
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2131816504)
thought about it, it seems more redundant (which I think could be safer for legacy code) if we state the sizes explicitly, otherwise changing the `CPubKey::COMPRESSED_SIZE` value might not fail immediately. Every other helper here has fixed size - and I couldn't find a reasonable value for all of them, decided to hard-code the sizes instead.
Do you have an idea that we could apply to all of these?
💬 l0rinc commented on pull request "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2132400987)
> this duplicating the tx_verify function
At the moment it seemed simpler to copy it over, but you're right that I can now just call it directly - thanks.
We can also hard-code the `expected_sigops` to have more redundancy here, since it's legacy code.
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2132400987)
> this duplicating the tx_verify function
At the moment it seemed simpler to copy it over, but you're right that I can now just call it directly - thanks.
We can also hard-code the `expected_sigops` to have more redundancy here, since it's legacy code.
💬 l0rinc commented on pull request "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2132590229)
> is adding new coverage
> but not really new coverage
Not sure, many of these branches were never covered - based on https://maflcko.github.io/b-c-cov/total.coverage/src/script/script.cpp.gcov.html
And based on https://corecheck.dev/bitcoin/bitcoin/pulls/32532 many more are covered now
Added comments to the tests, please let me know what I'm still missing.
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2132590229)
> is adding new coverage
> but not really new coverage
Not sure, many of these branches were never covered - based on https://maflcko.github.io/b-c-cov/total.coverage/src/script/script.cpp.gcov.html
And based on https://corecheck.dev/bitcoin/bitcoin/pulls/32532 many more are covered now
Added comments to the tests, please let me know what I'm still missing.
💬 l0rinc commented on pull request "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2131814130)
Some of these are rechecked later, but it looks like we need some unit tests for this area - thanks for the detailed review, I have incorrectly removed it [during a rebase](https://github.com/bitcoin/bitcoin/compare/ff835469baef8f048c44608c6d6a1a376bba87b7..432ff7e736942c6dd6fc923300b5fa5f0f94c287#diff-93c5ddf703879287802d651b68c9c6199644afe23db0dd3ff77e8d1ce3c20045) - since I got confused of the different ways of checking the same.
It was also noticed by https://github.com/bitcoin/bitcoin/pu
...
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2131814130)
Some of these are rechecked later, but it looks like we need some unit tests for this area - thanks for the detailed review, I have incorrectly removed it [during a rebase](https://github.com/bitcoin/bitcoin/compare/ff835469baef8f048c44608c6d6a1a376bba87b7..432ff7e736942c6dd6fc923300b5fa5f0f94c287#diff-93c5ddf703879287802d651b68c9c6199644afe23db0dd3ff77e8d1ce3c20045) - since I got confused of the different ways of checking the same.
It was also noticed by https://github.com/bitcoin/bitcoin/pu
...
💬 l0rinc commented on pull request "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2131814823)
Thank you, split these into smaller commits to make sure we don't miss these anymore
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2131814823)
Thank you, split these into smaller commits to make sure we don't miss these anymore
💬 Zeegaths commented on issue "doc: references to unshipped documentation":
(https://github.com/bitcoin/bitcoin/issues/32565#issuecomment-2949982526)
Yeah I saw it.. so that means writing the docs again on the paths defined in the comments? and updating regularly as the online versions update?
(https://github.com/bitcoin/bitcoin/issues/32565#issuecomment-2949982526)
Yeah I saw it.. so that means writing the docs again on the paths defined in the comments? and updating regularly as the online versions update?
💬 sipa commented on issue "doc: references to unshipped documentation":
(https://github.com/bitcoin/bitcoin/issues/32565#issuecomment-2950024257)
You can link to the documentation in the specific releases, I guess. For example [`https://github.com/bitcoin/bitcoin/blob/v29.0/doc/cjdns.md`](https://github.com/bitcoin/bitcoin/blob/v29.0/doc/cjdns.md) for 29.0.
(https://github.com/bitcoin/bitcoin/issues/32565#issuecomment-2950024257)
You can link to the documentation in the specific releases, I guess. For example [`https://github.com/bitcoin/bitcoin/blob/v29.0/doc/cjdns.md`](https://github.com/bitcoin/bitcoin/blob/v29.0/doc/cjdns.md) for 29.0.
💬 Zeegaths commented on issue "doc: references to unshipped documentation":
(https://github.com/bitcoin/bitcoin/issues/32565#issuecomment-2950041591)
Ok, thanks for the tip
On Fri, 6 Jun 2025 at 20:57, Pieter Wuille ***@***.***> wrote:
> *sipa* left a comment (bitcoin/bitcoin#32565)
> <https://github.com/bitcoin/bitcoin/issues/32565#issuecomment-2950024257>
>
> You can link to the documentation in the specific releases, I guess. For
> example https://github.com/bitcoin/bitcoin/blob/v29.0/doc/cjdns.md for
> 29.0.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/issues/32565#issuecomme
...
(https://github.com/bitcoin/bitcoin/issues/32565#issuecomment-2950041591)
Ok, thanks for the tip
On Fri, 6 Jun 2025 at 20:57, Pieter Wuille ***@***.***> wrote:
> *sipa* left a comment (bitcoin/bitcoin#32565)
> <https://github.com/bitcoin/bitcoin/issues/32565#issuecomment-2950024257>
>
> You can link to the documentation in the specific releases, I guess. For
> example https://github.com/bitcoin/bitcoin/blob/v29.0/doc/cjdns.md for
> 29.0.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/issues/32565#issuecomme
...
💬 darosior commented on pull request "refactor,test: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#issuecomment-2950052477)
> @darosior, this change isn't about IBD, it was mostly about block connect time optimization originally - now it's rather meant as a cleanup and flooding it with tests and benchmarks to enable optimizations in follow-ups.
This further strengthens my [point](https://github.com/bitcoin/bitcoin/pull/32532#pullrequestreview-2851775828). I don't think we should make a major overhaul to tricky consensus-critical code that was never touched in 15 years just to "make it nicer" (and possibly enabling
...
(https://github.com/bitcoin/bitcoin/pull/32532#issuecomment-2950052477)
> @darosior, this change isn't about IBD, it was mostly about block connect time optimization originally - now it's rather meant as a cleanup and flooding it with tests and benchmarks to enable optimizations in follow-ups.
This further strengthens my [point](https://github.com/bitcoin/bitcoin/pull/32532#pullrequestreview-2851775828). I don't think we should make a major overhaul to tricky consensus-critical code that was never touched in 15 years just to "make it nicer" (and possibly enabling
...
💬 achow101 commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2132644553)
> But `LockCoin` isn't loading the from the database, which is what makes this call site confusing.
Neither does `SetAddressPreviouslySpent` but it still calls `LoadAddressPreviouslySpent` to set it in memory.
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2132644553)
> But `LockCoin` isn't loading the from the database, which is what makes this call site confusing.
Neither does `SetAddressPreviouslySpent` but it still calls `LoadAddressPreviouslySpent` to set it in memory.