💬 theStack commented on pull request "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs":
(https://github.com/bitcoin/bitcoin/pull/32596#discussion_r2104795226)
Ah interesting, wasn't aware that this was proposed already in an earlier PR. Added a link to your comment in the PR description.

💬 gmaxwell commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2904766683)
FWIW, this is solvable by changing to more fibre like methods for block transmission. Because FEC coded missing data doesn't identify what was missing.

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

💬 theStack commented on pull request "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs":
(https://github.com/bitcoin/bitcoin/pull/32596#discussion_r2104807129)
As this is an optional return field, I wouldn't consider its removal as a breaking change, considering that the condition for returning a value (i.e. executing the RPC on a legacy wallet) can't be fulfilled anymore. Curious about other opinions though; I guess one could argue that there is still a point in mentioning the removal in the release notes?

📝 rkrux opened a pull request: "wallet, rpc: clarify wallet version in getwalletinfo help"
(https://github.com/bitcoin/bitcoin/pull/32603)
I noted in review of a previous PR here: https://github.com/bitcoin/bitcoin/pull/32553#pullrequestreview-2853743852.

Relaying the same information in the RPC help that's present in a code comment: https://github.com/bitcoin/bitcoin/blob/af65fd1a333011137dafd3df9a51704fd319feb4/src/wallet/wallet.h#L809-L810

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests
...

🤔 l0rinc reviewed a pull request: "fuzz: Add target for coins database"
(https://github.com/bitcoin/bitcoin/pull/32602#pullrequestreview-2864770398)
Concept ACK

Quickly reviewed it again, will do it more deeply later - do we still need the `cachedCoinsUsage` comment after https://github.com/bitcoin/bitcoin/pull/32313, which should fix the accounting bug instead of working around it?

💬 l0rinc commented on pull request "fuzz: Add target for coins database":
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2104799482)
There's a helper introduced since:
```suggestion
.cache_bytes = 1_MiB,
```

💬 l0rinc commented on pull request "fuzz: Add target for coins database":
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2104802538)
I still think this comment is way to verbose, we don't need that much context here.
Is this still needed after [`327ee45` (#32313)](https://github.com/bitcoin/bitcoin/pull/32313/commits/327ee453cab9d7c8482f74b195f2be6c6f1a4cc9) where we might not get into the described situation anymore since `cachedCoinsUsage` update only happens after the mentioned exception now.

💬 l0rinc commented on pull request "fuzz: Add target for coins database":
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2104798281)
nit: in other cases in this file the param hints are written as:
```suggestion
TestCoinsView(fuzzed_data_provider, coins_db, /*is_db=*/ true);
```

💬 l0rinc commented on pull request "fuzz: Add target for coins database":
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2104812882)
The default value for `is_db` may not be intuitive, can we make it explicit instead?

💬 l0rinc commented on pull request "fuzz: Add target for coins database":
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2104819427)
can you please add a short comment to the `best_block = uint256::ONE` cases?

💬 rkrux commented on pull request "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs":
(https://github.com/bitcoin/bitcoin/pull/32596#discussion_r2104831591)
Ah, I note now that its presence was conditional on legacy wallets.

💬 maflcko commented on pull request "test: Fix `system_tests/run_command` test":
(https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2104835433)
Then it seems like it should be fixed in `RunCommandParseJSON`, because real users could run into the bug as well?

`RunCommandParseJSON` should just take a string and then pass it to the shell as-is?

💬 hebasto commented on pull request "subprocess: Let shell parse command on non-Windows systems":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2904837194)
> * It would probably be good if the new behavior were controlled by a shell boolean option like python & the upstream library

Reverting 633e45b2e2728efcb0637afa94fcbd5756dfbe76 is still an option on the table. https://github.com/bitcoin/bitcoin/pull/32566 may also [benefit](https://github.com/bitcoin/bitcoin/pull/32566#discussion_r2099983137) from it.

The issues mentioned in https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2899448860 might be addressed by switching to an alterna
...

💬 instagibbs commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2904837679)
Dropping seemingly-unsolicited compact blocks seems fine, we already do that for the parallel portion, this should be a change just to the first one? blocksonly nodes should never set cb peers.

💬 hebasto commented on pull request "test: Fix `system_tests/run_command` test":
(https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2104848828)
That was the approach in https://github.com/bitcoin/bitcoin/pull/32577.

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2104866651)
There was a build failure on 32 bit systems because of the alignment changes - fixed it by moving the `std::assume_aligned` check inside the alignment condition itself.
Remeasured the benchmarks (all 3 compilers are the same as the previous push) and redid the endianness check (as described in the PR) and updated the https://godbolt.org/z/35nveanf5 again.

💬 m3dwards commented on issue "test: `tool_wallet.py` references no-longer used CI":
(https://github.com/bitcoin/bitcoin/issues/32576#issuecomment-2904880163)
Removing '666' fails tests on windows: https://github.com/m3dwards/bitcoin/actions/runs/15213032706/job/42791654918

Suggest just changing comment to:

`666 on Windows`

💬 maflcko commented on pull request "ci: Downgrade DEBUG=1 to -D_GLIBCXX_ASSERTIONS in centos task":
(https://github.com/bitcoin/bitcoin/pull/32586#issuecomment-2904920622)
(edited a prior comment of mine, which said there are two libc++ debug runs in the CI. In reality there is the libc++ debug build (msan) and the glibc++ debug build (32-bit).)

💬 maflcko commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout) under GLIBCXX debug mode":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2904926114)
The 32-bit GLIBCXX debug run (gcc-13) is also passing fine: https://cirrus-ci.com/task/6401825881980928?logs=ci#L3769

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