💬 mzumsande commented on pull request "net: do not apply whitelist permissions to onion inbounds":
(https://github.com/bitcoin/bitcoin/pull/33395#issuecomment-3299745335)
[e46a7a5](https://github.com/bitcoin/bitcoin/commit/e46a7a547371317a4d116b9b1a314917508ea480) to [f563ce9](https://github.com/bitcoin/bitcoin/commit/f563ce90818d486d2a199439d2f6ba39cd106352): Addressed @vasild suggestion.

💬 mzumsande commented on pull request "net: do not apply whitelist permissions to onion inbounds":
(https://github.com/bitcoin/bitcoin/pull/33395#discussion_r2353219694)
think I'll leave this for a refactoring PR that can apply this more systematically.

👍 darosior approved a pull request: "net: do not apply whitelist permissions to onion inbounds"
(https://github.com/bitcoin/bitcoin/pull/33395#pullrequestreview-3231159963)
ACK f563ce90818d486d2a199439d2f6ba39cd106352

🤔 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.

💬 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.

💬 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`

👍 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

🤔 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.

💬 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.

💬 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.

💬 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?

💬 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
...

💬 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.

💬 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.

💬 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.

💬 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).

🤔 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.",

...

💬 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)

💬 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
...

💬 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.