💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2356967273)
In 9aaf55397159bb0e839c19d3d4578dfa144a059b "rpc: Handle -named argument parsing where '=' character is used"

nit: space between `s` and `:`
```suggestion
for (std::string_view s : strParams) {
```

💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2356965832)
In 9aaf55397159bb0e839c19d3d4578dfa144a059b "rpc: Handle -named argument parsing where '=' character is used"

Extraneous newline

💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3304806063)
> > because it used legacy wallet operations
>
> That's not why it fails, it does not use anything specific to legacy wallets at all. It fails initially because test nodes are started with `-disablewallet` if `self.uses_wallet == False`, and then fails again because of multiwallet, and lastly because `settxfee` is deprecated. But none of these are because of legacy wallet.
>
> Ultimately, the issues stem from the fact that this code path is pretty much never executed, and if MiniWallet were to
...

💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3304812339)
> > because it used legacy wallet operations
>
> That's not why it fails, it does not use anything specific to legacy wallets at all. It fails initially because test nodes are started with `-disablewallet` if `self.uses_wallet == False`, and then fails again because of multiwallet, and lastly because `settxfee` is deprecated. But none of these are because of legacy wallet.
>
> Ultimately, the issues stem from the fact that this code path is pretty much never executed, and if MiniWallet were to
...

💬 Raimo33 commented on pull request "Remove unnecessary casts when calling socket operations":
(https://github.com/bitcoin/bitcoin/pull/33378#issuecomment-3304898737)
ACK 67f632b6deb8b4aa190c458b71d2bc8c793626d5

💬 luke-jr commented on pull request "cmake: Inherit WERROR setting for secp256k1 build":
(https://github.com/bitcoin/bitcoin/pull/33297#issuecomment-3305020767)
Concept ACK. As long as -DWERROR is supported, it should work correctly.

Discussion about dropping it in favour of other solutions should be considered separately (and existing branches should get fixed even if it's dropped in newer versions)

👍 davidgumberg approved a pull request: "Remove unnecessary casts when calling socket operations"
(https://github.com/bitcoin/bitcoin/pull/33378#pullrequestreview-3236853659)
ACK https://github.com/bitcoin/bitcoin/commit/67f632b6deb8b4aa190c458b71d2bc8c793626d5

As described in the commit messages, the `Sock::(G|S)etSockOpt` functions are already casting correctly, so there shouldn't be casting when using those functions. The only other users of `(g|s)etsockopt` are the httpserver (which doesn't use `Sock`):

https://github.com/bitcoin/bitcoin/blob/67f632b6deb8b4aa190c458b71d2bc8c793626d5/src/httpserver.cpp#L404

and the socket unit tests:

https://github.com
...

💬 luke-jr commented on pull request "RFC: blocks: add `-reobfuscate-blocks` arg to xor existing blk/rev on startup":
(https://github.com/bitcoin/bitcoin/pull/33324#issuecomment-3305066838)
I agree a separate utility for this seems better - this requires very little of the existing codebase, in theory.

Also suggest making the files with the same names, but in a new directory, and then atomically rename the directory when complete, rather than every single file.

💬 luke-jr commented on pull request "crypto: optimize SipHash Write() method with chunked processing":
(https://github.com/bitcoin/bitcoin/pull/33325#issuecomment-3305075347)
> I just realized this was attempted in the past. merged in Bitcoin knots 19. restored to match Core's implementation in knots 29. 🤔.

#18014 is still in Knots 29...

💬 l0rinc commented on pull request "crypto: optimize SipHash Write() method with chunked processing":
(https://github.com/bitcoin/bitcoin/pull/33325#issuecomment-3305097747)
@luke-jr where is this general writer needed? We already have separate specialized siphash implementation for the critical usecases...

💬 l0rinc commented on pull request "RFC: blocks: add `-reobfuscate-blocks` arg to xor existing blk/rev on startup":
(https://github.com/bitcoin/bitcoin/pull/33324#issuecomment-3305103297)
> I agree a separate utility for this seems better

Can you quote what you're agreeing with specifically, not sure who suggested that.
Besides, @andrewtoth already has a tool for that, it was mentioned in the PR description.

> but in a new directory, and then atomically rename

I will think about it, could make sense, but in that case unrelated files should also be copied over (maybe duplicated to be safe) - and listing the directory content wouldn't make the progress obvious. What's wr
...

💬 HowHsu commented on pull request "index: remove unnecessary locator cleaning in BaseIndex::Init()":
(https://github.com/bitcoin/bitcoin/pull/32882#issuecomment-3305245694)
> ACK [facd01e](https://github.com/bitcoin/bitcoin/commit/facd01e6ffbbd019312f370a3807de0b95bbd745)
>
> Note that we don't usually add the `Signed off by` to the commit message, it just seems like noise to me. You can still sign your commits if you want, but I'm no sure what this line adds that isn't already obvious.
>

Added by my git config automatically, that is a kind of DCO for GPLv2, I'll remove it in later PRs since Bitcoin Core is MIT, so yes, it is not necessary to Sign.

> Als
...

💬 luke-jr commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2357526353)
Should we be logging which chainstate this is, in case there's multiple?

💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2357539686)
Currently we're only logging this if there's a single one, i.e. `GetRole() == ChainstateRole::NORMAL`

💬 luke-jr commented on pull request "help: enrich help text for `-loadblock`":
(https://github.com/bitcoin/bitcoin/pull/33343#issuecomment-3305484748)
I think it would make sense to support XOR for loadblock if we're assuming the chain has data that can trigger bad things

💬 kannapoix commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#discussion_r2357586680)
This command may result in an error like:
```
error code: -8
error message:
Unknown named parameter cHNidP8BAH0CAAAAASMcBn5VJaIET6lL4b76V676ffyrvpxT6UI++gY14LwWAQAAAAD9////AoAaBgAAAAAAFgAUhv4Mbo6zEszAaF4zBry18DXCw+CuhQEAAAAAACIAIKUhPbGKzw2gMuSI/kFtVa5EYyTBZdlS7zeuyJg5kJ2kAAAAAAABAH0CAAAAARULOYOh2W+IbDULcj5n6NF4uzQx8ZcqXa+YrNgx+Y/XmwMAAAD9////AqQGAAAAAAAAFgAUvI6wMmXzdKiS7/nhen+pYfbaY8MgoQcAAAAAACIAILvTfOv022A80QUkWRjFA2e0RkHffMZf5J8TeDWo3ajRIB8EAAEBKyChBwAAAAAAIgAgu9N86/TbYDzRBSRZGMUDZ7RGQd98xl/k
...

💬 luke-jr commented on pull request "log: show reindex progress in `ImportBlocks`":
(https://github.com/bitcoin/bitcoin/pull/33353#discussion_r2357830141)
Files are not necessarily the same size/complexity...

💬 hebasto commented on pull request "cmake: Inherit WERROR setting for secp256k1 build":
(https://github.com/bitcoin/bitcoin/pull/33297#discussion_r2357851661)
Why not reusing the `working_compiler_werror_flag` variable?

💬 frankomosh commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2357861031)
Thanks for the review @achow101

I agree it could be done directly in deserialization. The reason it’s in net_processing is that it acts as a boundary for untrusted peer input, so having the invariant explicit here serves as a defense-in-depth check (similar to other message level sanity checks here).
The extra loop is only executed after we already know the message is GETBLOCKTXN and after successful deserialization, so it’s a one-time O(n) pass per compact block request. Given the bounded
...

💬 dergoegge commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2357909155)
Since this is using Assume, the loop shouldn't exist in the release binaries (i.e the compiler optimises it out), so the overhead doesn't exist.