💬 enirox001 commented on pull request "log: show reindex progress in `ImportBlocks`":
(https://github.com/bitcoin/bitcoin/pull/33353#discussion_r2353411309)
Would it make sense to log `nFile+1/total_files` (e.g. `4/125`) alongside the percent? That gives clearer, more granular progress than an integer percent alone.
(https://github.com/bitcoin/bitcoin/pull/33353#discussion_r2353411309)
Would it make sense to log `nFile+1/total_files` (e.g. `4/125`) alongside the percent? That gives clearer, more granular progress than an integer percent alone.
💬 enirox001 commented on pull request "log: show reindex progress in `ImportBlocks`":
(https://github.com/bitcoin/bitcoin/pull/33353#discussion_r2353404361)
Use a defensive guard when computing percent, e.g. `total_files ? (100 * (nFile + 1) / total_files) : 0`, to avoid accidental divide-by-zero after future refactors.
(https://github.com/bitcoin/bitcoin/pull/33353#discussion_r2353404361)
Use a defensive guard when computing percent, e.g. `total_files ? (100 * (nFile + 1) / total_files) : 0`, to avoid accidental divide-by-zero after future refactors.
💬 enirox001 commented on pull request "log: show reindex progress in `ImportBlocks`":
(https://github.com/bitcoin/bitcoin/pull/33353#discussion_r2353403899)
Consider computing the progress as `(nFile + 1) * 100 / total_files` so the indicator advances past 0% on the first file. Is there a reason we intentionally show 0% initially?
(https://github.com/bitcoin/bitcoin/pull/33353#discussion_r2353403899)
Consider computing the progress as `(nFile + 1) * 100 / total_files` so the indicator advances past 0% on the first file. Is there a reason we intentionally show 0% initially?
💬 musaHaruna commented on pull request "rpc: add scan_utxoset option to getbalance(s) to verify wallet balance accuracy":
(https://github.com/bitcoin/bitcoin/pull/33392#issuecomment-3300125137)
> I have not looked into making it an option for importdecriptors, but I will look into it, and get back to you on that.
Yes, I have looked into adding the option to importdescriptors by introducing a scan_utxoset flag and it's possible. The idea is that when enabled, the wallet would scan the UTXO set immediately after import to verify balances against chainstate, and if a discrepancy is detected (for example due to an incorrect birthdate), it could automatically trigger a full rescan from h
...
(https://github.com/bitcoin/bitcoin/pull/33392#issuecomment-3300125137)
> I have not looked into making it an option for importdecriptors, but I will look into it, and get back to you on that.
Yes, I have looked into adding the option to importdescriptors by introducing a scan_utxoset flag and it's possible. The idea is that when enabled, the wallet would scan the UTXO set immediately after import to verify balances against chainstate, and if a discrepancy is detected (for example due to an incorrect birthdate), it could automatically trigger a full rescan from h
...
💬 hebasto commented on pull request "Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#issuecomment-3300132443)
Rebased to refresh the CI.
(https://github.com/bitcoin/bitcoin/pull/32380#issuecomment-3300132443)
Rebased to refresh the CI.
💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2353518252)
6000fc8c3fcc0446b3665f7fccf102068919fa3c
Didn't nail down exactly when this would become a concern, but do note that this call can trigger `tx.vin.size()` number staged parents and `GetAncestors` calls based on those parents.
Think this is potentially a huge number of clusters with each up to 64 transactions at a time.
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2353518252)
6000fc8c3fcc0446b3665f7fccf102068919fa3c
Didn't nail down exactly when this would become a concern, but do note that this call can trigger `tx.vin.size()` number staged parents and `GetAncestors` calls based on those parents.
Think this is potentially a huge number of clusters with each up to 64 transactions at a time.
💬 theStack commented on pull request "key: use static context for libsecp256k1 calls where applicable":
(https://github.com/bitcoin/bitcoin/pull/33399#discussion_r2353526094)
Makes sense yes, done. Packed the scope reduction in the same commit as I think the diff is still digestible, but happy to divide it into two commits if that's preferable for reviewers.
(https://github.com/bitcoin/bitcoin/pull/33399#discussion_r2353526094)
Makes sense yes, done. Packed the scope reduction in the same commit as I think the diff is still digestible, but happy to divide it into two commits if that's preferable for reviewers.
💬 theStack commented on pull request "key: use static context for libsecp256k1 calls where applicable":
(https://github.com/bitcoin/bitcoin/pull/33399#issuecomment-3300168851)
Thanks for the reviews, fixed the typo in commit message and PR description (s/paramter/parameter/) and reduced the scope of `secp256k1_context_sign` in the fuzz test as suggested (h/t @l0rinc). Didn't do any renaming of the context object, as I haven't stumbled upon a name yet where I was convinced that it's much better (I agree with @real-or-random though that's it's imprecise).
(https://github.com/bitcoin/bitcoin/pull/33399#issuecomment-3300168851)
Thanks for the reviews, fixed the typo in commit message and PR description (s/paramter/parameter/) and reduced the scope of `secp256k1_context_sign` in the fuzz test as suggested (h/t @l0rinc). Didn't do any renaming of the context object, as I haven't stumbled upon a name yet where I was convinced that it's much better (I agree with @real-or-random though that's it's imprecise).
🤔 danielabrozzoni reviewed a pull request: "coins: warn on oversized `-dbcache`"
(https://github.com/bitcoin/bitcoin/pull/33333#pullrequestreview-3230365817)
tACK 13fa0d0a433722f294854001b0561c079db12dbc
I tested locally and reproduced the warning:
```
❯ ./build/bin/bitcoind -dbcache=64000
...
2025-09-16T13:57:22Z [warning] A 64000 MiB dbcache may be too large for a system memory of only 64219 MiB.
Warning: A 64000 MiB dbcache may be too large for a system memory of only 64219 MiB.
❯ ./build/bin/bitcoin-cli getblockchaininfo
{
[...]
"warnings": [
"A 64000 MiB dbcache may be too large for a system memory of only 64219 MiB.",
...
(https://github.com/bitcoin/bitcoin/pull/33333#pullrequestreview-3230365817)
tACK 13fa0d0a433722f294854001b0561c079db12dbc
I tested locally and reproduced the warning:
```
❯ ./build/bin/bitcoind -dbcache=64000
...
2025-09-16T13:57:22Z [warning] A 64000 MiB dbcache may be too large for a system memory of only 64219 MiB.
Warning: A 64000 MiB dbcache may be too large for a system memory of only 64219 MiB.
❯ ./build/bin/bitcoin-cli getblockchaininfo
{
[...]
"warnings": [
"A 64000 MiB dbcache may be too large for a system memory of only 64219 MiB.",
...
💬 danielabrozzoni commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2352760672)
nit: could benefit from a comment explaining the conditional check
From my understanding you want to check that `GetTotalRAM()` wouldn't return an enormous value, and in order to do so, you need to make sure that `size_t` can accommodate an enormous value (if `size_t` was `UINT32_MAX`, `GetTotalRAM()` would never return values over ~4GB)
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2352760672)
nit: could benefit from a comment explaining the conditional check
From my understanding you want to check that `GetTotalRAM()` wouldn't return an enormous value, and in order to do so, you need to make sure that `size_t` can accommodate an enormous value (if `size_t` was `UINT32_MAX`, `GetTotalRAM()` would never return values over ~4GB)
💬 TheCharlatan commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2353555803)
It seems to have found something:
```
#234857 REDUCE cov: 862 ft: 5138 corp: 265/28Kb lim: 1689 exec/s: 303 rss: 854Mb L: 1178/1640 MS: 4 ChangeByte-EraseBytes-InsertByte-InsertByte-
#236845 NEW cov: 862 ft: 5140 corp: 266/29Kb lim: 1699 exec/s: 303 rss: 854Mb L: 1652/1652 MS: 3 CopyPart-CMP-CopyPart- DE: "N10__cxxabiv"-
#237681 REDUCE cov: 862 ft: 5140 corp: 266/29Kb lim: 1699 exec/s: 303 rss: 855Mb L: 23/1652 MS: 2 EraseBytes-PersAutoDict- DE: "St9exception"-
ALARM: working on the last
...
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2353555803)
It seems to have found something:
```
#234857 REDUCE cov: 862 ft: 5138 corp: 265/28Kb lim: 1689 exec/s: 303 rss: 854Mb L: 1178/1640 MS: 4 ChangeByte-EraseBytes-InsertByte-InsertByte-
#236845 NEW cov: 862 ft: 5140 corp: 266/29Kb lim: 1699 exec/s: 303 rss: 854Mb L: 1652/1652 MS: 3 CopyPart-CMP-CopyPart- DE: "N10__cxxabiv"-
#237681 REDUCE cov: 862 ft: 5140 corp: 266/29Kb lim: 1699 exec/s: 303 rss: 855Mb L: 23/1652 MS: 2 EraseBytes-PersAutoDict- DE: "St9exception"-
ALARM: working on the last
...
💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2353578407)
I think it would still be necessary to note what the session id is composed of.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2353578407)
I think it would still be necessary to note what the session id is composed of.
💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2353592463)
A cannot add their sig before B has added a pubnonce. Furthermore, if A added their sig, then the secnonce would be already be deleted.
If B instead provides a "new" PSBT which is really just the original PSBT with their pubnonce, and without A's pubnonce, then it's fine to "reuse" the nonce since that is equivalent to combining both PSBTs.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2353592463)
A cannot add their sig before B has added a pubnonce. Furthermore, if A added their sig, then the secnonce would be already be deleted.
If B instead provides a "new" PSBT which is really just the original PSBT with their pubnonce, and without A's pubnonce, then it's fine to "reuse" the nonce since that is equivalent to combining both PSBTs.
💬 fjahr commented on pull request "rpc: add scan_utxoset option to getbalance(s) to verify wallet balance accuracy":
(https://github.com/bitcoin/bitcoin/pull/33392#issuecomment-3300319224)
> Yes, I have looked into adding the option to importdescriptors by introducing a scan_utxoset flag and it's possible. The idea is that when enabled, the wallet would scan the UTXO set immediately after import to verify balances against chainstate, and if a discrepancy is detected (for example due to an incorrect birthdate), it could automatically trigger a full rescan from height 0 to restore missing history. This way, users wouldn’t have to manually diagnose incomplete balances — the import fl
...
(https://github.com/bitcoin/bitcoin/pull/33392#issuecomment-3300319224)
> Yes, I have looked into adding the option to importdescriptors by introducing a scan_utxoset flag and it's possible. The idea is that when enabled, the wallet would scan the UTXO set immediately after import to verify balances against chainstate, and if a discrepancy is detected (for example due to an incorrect birthdate), it could automatically trigger a full rescan from height 0 to restore missing history. This way, users wouldn’t have to manually diagnose incomplete balances — the import fl
...
💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2353648451)
Very nice!
Played a bit with the code, pushed some (dirty and untested) extensions here: https://github.com/furszy/bitcoin-core/commits/2022_parallelize_blockfilter_index_2_fuzz/
> It seems to have found something
will check it out. Thanks!. I was having fun with the code first :).
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2353648451)
Very nice!
Played a bit with the code, pushed some (dirty and untested) extensions here: https://github.com/furszy/bitcoin-core/commits/2022_parallelize_blockfilter_index_2_fuzz/
> It seems to have found something
will check it out. Thanks!. I was having fun with the code first :).
🤔 l0rinc reviewed a pull request: "log: always print initial signature verification state"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3231017555)
Thanks for the reviews, I have simplified the commits, removed the enum and replaced it with direct reason string, added an `off best‑header path` (thanks to @hodlinator), reworded the log to say "script verification" instead of "signature validations" to be in alignment with the "Script verification uses ... additional threads" and reverted the `fScriptChecks` and `m_prev_script_checks_logged` symbols.
There's a remaining suggestion to have friendlier reason in case of:
```
2025-09-16T21:00:03
...
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3231017555)
Thanks for the reviews, I have simplified the commits, removed the enum and replaced it with direct reason string, added an `off best‑header path` (thanks to @hodlinator), reworded the log to say "script verification" instead of "signature validations" to be in alignment with the "Script verification uses ... additional threads" and reverted the `fScriptChecks` and `m_prev_script_checks_logged` symbols.
There's a remaining suggestion to have friendlier reason in case of:
```
2025-09-16T21:00:03
...
💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2353625992)
Reverted since it caused more confusion that it fixed
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2353625992)
Reverted since it caused more confusion that it fixed
💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2353349979)
Cheap script checks (such as sigop counts) are still checked regardless of this option, it's the signatures that aren't (so *some* script sanity checking is still happening, but the scripts aren't interpreted - that's my understanding, please correct me if I'm wrong. Maybe I should update the warning message in that case to make it clear that a few other checks are also skipped, not just the signatures).
And I want to propose a change to [skip bip30 checks](https://github.com/bitcoin/bitcoin/bl
...
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2353349979)
Cheap script checks (such as sigop counts) are still checked regardless of this option, it's the signatures that aren't (so *some* script sanity checking is still happening, but the scripts aren't interpreted - that's my understanding, please correct me if I'm wrong. Maybe I should update the warning message in that case to make it clear that a few other checks are also skipped, not just the signatures).
And I want to propose a change to [skip bip30 checks](https://github.com/bitcoin/bitcoin/bl
...
💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2353190912)
Was trying to recreate production code logic in test, but I don't mind making it more explicit if you think it's more descriptive.
The block validation errors seem out-of-scope, but I don't mind them either, thanks, added you as coauthor to the last commit as well.
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2353190912)
Was trying to recreate production code logic in test, but I don't mind making it more explicit if you think it's more descriptive.
The block validation errors seem out-of-scope, but I don't mind them either, thanks, added you as coauthor to the last commit as well.
💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2353637282)
I'm not sure I fully understand - do you want me to add a different reason when the current block is valid, but just after the assumevalid block?
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2353637282)
I'm not sure I fully understand - do you want me to add a different reason when the current block is valid, but just after the assumevalid block?