💬 glozow commented on pull request "[28.x] More backports":
(https://github.com/bitcoin/bitcoin/pull/33415#issuecomment-3304507492)
> Should we also backport #33106 if we're going to be doing a 28.3 anyways?

Here is a branch to do this: https://github.com/glozow/bitcoin/tree/2025-09-28.3-backport

Similar to #33226, needed to do some test massaging. I also needed #30948 and #30748 for the test utils/refactors. Feel free to pull; I updated the final changes as well.

💬 hodlinator commented on pull request "build: Remove bitness suffix from Windows installer":
(https://github.com/bitcoin/bitcoin/pull/32132#issuecomment-3304511251)
Maybe we could amend the .NSI-script to unregister versions with the old name. I plan to take a stab at this tomorrow unless something unexpected comes up or someone else wants to pick it up.

💬 achow101 commented on pull request "Fix #25980: Validate transactions in combinerawtransaction":
(https://github.com/bitcoin/bitcoin/pull/33361#issuecomment-3304515191)
#31298 also fixes this issue.

CI is failing because you've added a test that uses wallet behavior to a non-wallet test.

💬 achow101 commented on pull request "test: Fix TestFlushBehavior when coin has non-zero dynamic memory usage":
(https://github.com/bitcoin/bitcoin/pull/33381#issuecomment-3304517512)
This change breaks the test.

💬 achow101 commented on pull request "rpc: Add validation for invalid taproot signatures in analyzepsbt":
(https://github.com/bitcoin/bitcoin/pull/33360#discussion_r2356755028)
This PSBT isn't serialized correctly.

💬 achow101 commented on pull request "doc: Add documentation explaining different build types":
(https://github.com/bitcoin/bitcoin/pull/33355#issuecomment-3304532974)
Please stop using LLMs to generate PRs.

✅ achow101 closed a pull request: "doc: Add documentation explaining different build types"
(https://github.com/bitcoin/bitcoin/pull/33355)

💬 l0rinc commented on pull request "index: remove unnecessary locator cleaning in BaseIndex::Init()":
(https://github.com/bitcoin/bitcoin/pull/32882#issuecomment-3304577242)
ACK 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.

Also, not sure why `refactor: remove redundant locator cleanup in BaseIndex::Init()` is repeated (slightly differently) in the PR description.

💬 achow101 commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2356816234)
In 65a10fc3c52ea09a4794345bcf607dff908c783a "p2p: add assertion for BlockTransactionsRequest indexes"

Can checking this be done in deserialization directly, rather than iterating all of the indexes again? Since this is part of compact block relay, we want to avoid doing as much unnecessary work as possible.

💬 achow101 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3304776460)
> 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 chang
...

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

This shouldn't be deleted.

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