🤔 furszy reviewed a pull request: "net: do not apply whitelist permissions to onion inbounds"
(https://github.com/bitcoin/bitcoin/pull/33395#pullrequestreview-3231188034)
ACK f563ce90818d486d2a199439d2f6ba39cd106352
Comments are nits, not-blocking. The code is good as is.
(https://github.com/bitcoin/bitcoin/pull/33395#pullrequestreview-3231188034)
ACK f563ce90818d486d2a199439d2f6ba39cd106352
Comments are nits, not-blocking. The code is good as is.
💬 furszy commented on pull request "net: do not apply whitelist permissions to onion inbounds":
(https://github.com/bitcoin/bitcoin/pull/33395#discussion_r2353308719)
nano nit:
usually, it is better to be explicit with the empty optionals:
```suggestion
AddWhitelistPermissionFlags(permission_flags, inbound_onion ? std::nullopt : std::make_optional(addr), vWhitelistedRangeIncoming);
```
but it is a non-blocking comment.
(https://github.com/bitcoin/bitcoin/pull/33395#discussion_r2353308719)
nano nit:
usually, it is better to be explicit with the empty optionals:
```suggestion
AddWhitelistPermissionFlags(permission_flags, inbound_onion ? std::nullopt : std::make_optional(addr), vWhitelistedRangeIncoming);
```
but it is a non-blocking comment.
💬 furszy commented on pull request "net: do not apply whitelist permissions to onion inbounds":
(https://github.com/bitcoin/bitcoin/pull/33395#discussion_r2353315252)
nano nit for later:
could skip the loop when `addr == std::nullopt`
(https://github.com/bitcoin/bitcoin/pull/33395#discussion_r2353315252)
nano nit for later:
could skip the loop when `addr == std::nullopt`
👍 vasild approved a pull request: "net: do not apply whitelist permissions to onion inbounds"
(https://github.com/bitcoin/bitcoin/pull/33395#pullrequestreview-3231237773)
ACK f563ce90818d486d2a199439d2f6ba39cd106352
(https://github.com/bitcoin/bitcoin/pull/33395#pullrequestreview-3231237773)
ACK f563ce90818d486d2a199439d2f6ba39cd106352
🤔 enirox001 reviewed a pull request: "log: show reindex progress in `ImportBlocks`"
(https://github.com/bitcoin/bitcoin/pull/33353#pullrequestreview-3231320525)
Concept ACK d7de5b1
I tested this locally, the percentage logging provides a clear indication of progress during reindex and is useful visibility while the node processes block files.
I have left a few non-blocking nits for consideration.
(https://github.com/bitcoin/bitcoin/pull/33353#pullrequestreview-3231320525)
Concept ACK d7de5b1
I tested this locally, the percentage logging provides a clear indication of progress during reindex and is useful visibility while the node processes block files.
I have left a few non-blocking nits for consideration.
💬 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 :).