💬 hebasto commented on pull request "subprocess: Let shell parse command on non-Windows systems":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2905201466)
Reopened per [request](https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2104898126).

💬 hebasto commented on pull request "test: Fix `system_tests/run_command` test":
(https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2105083977)
> Yes, but it is unclear to me why it was closed, when it looks like the correct approach

Reopened.

💬 jonatack commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2905206387)
Concept ACK, need to review this

🤔 jonatack reviewed a pull request: "wallet, rpc: clarify wallet version in getwalletinfo help"
(https://github.com/bitcoin/bitcoin/pull/32603#pullrequestreview-2865264982)
ACK 21ddfbf888dabed1bb89b30481f1391727f6212

modulo nit suggestion, and PR/commit title `s/wallet, rpc/rpc, doc/`

💬 jonatack commented on pull request "wallet, rpc: clarify wallet version in getwalletinfo help":
(https://github.com/bitcoin/bitcoin/pull/32603#discussion_r2105156607)
```suggestion
{RPCResult::Type::NUM, "walletversion", "the current wallet format (the oldest client version guaranteed to understand this wallet)"},
```

💬 Crypt-iQ commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2905381340)
> So I had thought the code would check if there were too many missing transactions and just request a full block if all were missing. That doesn't address the general attack (since the attacker could admit some real txn that are in the block) but it would mostly address the blocksonly case (and could easily get a if blocksonly check so that it always did) -- but I can't find that. It should be added.

Is this still useful if we add a patch to drop unsolicited `cmpctblock` messages? An attacker
...

🤔 jonatack reviewed a pull request: "log: print reason when writing chainstate"
(https://github.com/bitcoin/bitcoin/pull/32404#pullrequestreview-2865314208)
ACK cfc8056c316be4742938d64ea5207b90d10d1e28

💬 jonatack commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2105186391)
nit, "FlushStateToDisk" might be redundant here, as here is the log with `-logsourcelocations=1`

```
2025-05-23T17:59:04.348160Z [validation.cpp:2868] [FlushStateToDisk] [coindb] FlushStateToDisk write: flush mode=PERIODIC, prune=0, cache_large=1, cache_critical=0, periodic=0
```

✅ mzumsande closed a pull request: "validation: Do less work in NeedsRedownload"
(https://github.com/bitcoin/bitcoin/pull/31714)

💬 mzumsande commented on pull request "validation: Do less work in NeedsRedownload":
(https://github.com/bitcoin/bitcoin/pull/31714#issuecomment-2905395016)
I just thought that there should be a point in time where this edge-case scenario with multiple switches between pre-segwit and post-segwit datadirs would not be sufficiently realistic anymore to justify that each node runs these extra checks with each startup.

I could change the PR to describe this switching back-and-forth scenario explicitly (it alludes to it with "less thorough" / "most typical case"), but the currently saved time during startup (~40milliseconds on mainnet for me) does not
...

💬 achow101 commented on pull request "wallet, rpc: Return normalized descriptor in parent_descs":
(https://github.com/bitcoin/bitcoin/pull/32594#issuecomment-2905414873)
> nit: "prividing" -> "providing"

Fixed

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2105211527)
`musig()` expressions are a key expression, which itself contains multiple key expressions. It needs to be able to increment `key_exp_index` such that the caller will also know what the `key_exp_index` has been incremented to. The easiest way to do this was a reference.

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2105211725)
Done

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2105211791)
Done

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2105211849)
Done

💬 l0rinc commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2105212407)
Would this be more descriptive?
```suggestion
LogDebug(BCLog::COINDB, "Writing chainstate to disk: flush mode=%s, prune=%d, cache_large=%d, cache_critical=%d, periodic=%d",

```

💬 jonatack commented on issue "test: `tool_wallet.py` references no-longer used CI":
(https://github.com/bitcoin/bitcoin/issues/32576#issuecomment-2905417856)
@m3dwards those comments can be dropped completely; see #28116, which I will update and open.

💬 achow101 commented on pull request "walletdb: Log additional exception error messages for corrupted wallets":
(https://github.com/bitcoin/bitcoin/pull/32598#issuecomment-2905423874)
> Should also change [HasLegacyRecords()](https://github.com/bitcoin/bitcoin/blob/2df824f4e62b6bc569044819cd64f66f3839ba13/src/wallet/walletdb.cpp#L544) as it would log the same error twice with this change.

Done

💬 davidgumberg commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2105223515)
nit for here and above:
```suggestion
$installationPath = & $vswherePath -latest -property installationPath
```

or could drop the line where `$vswherePath` is defined, whatever your preference is.

💬 jonatack commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2105227345)
That does seem nicer.