🤔 marcofleon reviewed a pull request: "[29.x] 33106 backport and final changes for rc2"
(https://github.com/bitcoin/bitcoin/pull/33226#pullrequestreview-3138288017)
ACK 0034dcfba9dc599449e7569ed1b30e58d4f4434f

Went through the commits to confirm they match. The only differences are the addition of the `datacarriersize` argument in `feature_rbf.py` and the additional changes in `mempool_limit.py`. Also ran unit and functional tests on `29.x` branch locally.

💬 davidgumberg commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2289338049)
Ah, I see, thanks.

💬 davidgumberg commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2289342465)
I don't think we should be that worried about a 1 in 2^37 chance of spurious test failure, but we could just double the number of headers if that's a concern.

🚀 ryanofsky merged a pull request: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802)

💬 achow101 commented on pull request "rpc: followups for 33106":
(https://github.com/bitcoin/bitcoin/pull/33189#discussion_r2289350681)
None of the settings have decimals though

💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3208172294)
Thanks to all who reviewed, tested, and commented on this. This has been very useful and should lead to a lot of good followups. I think all concerns can be addressed in future PRs even if they have not been addressed here and I think there are clear next steps to address all the issues listed in the [review summary](https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3200849464)

💬 TheCharlatan commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3208178263)
Post-merge ACK ce7d94a492e61f2a43ea315e75be607d6aa71702

💬 davidgumberg commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3208186878)
crACK fa3d88ac35de75af

The test changes here look good to me. I left some nits that can be addressed if following-up or rebasing. I sanity checked that a fresh node doing headers syncing for mainnet, testnet, and signet work on my machine with this branch.

I think it would be good to have input from one of the authors/reviewers of #25717 on the non-test changes to headers sync in this PR, which are https://github.com/bitcoin/bitcoin/pull/32579/commits/4aea5fa3690310bfc2617e3bf31f555dae7d93
...

💬 ryanofsky commented on pull request "cli: Handle arguments that can be either JSON or string":
(https://github.com/bitcoin/bitcoin/pull/33230#issuecomment-3208194243)
Seems like a reasonable change. I wonder if an alternate solution might have the client treat hash_or_height as a string parameter instead of a JSON parameter, and update the server to accept height strings? But I guess that might not be backwards compatible if there are old scripts calling bitcoin-cli with extra quotes around the hash string.

💬 polespinasa commented on pull request "cli: Handle arguments that can be either JSON or string":
(https://github.com/bitcoin/bitcoin/pull/33230#issuecomment-3208206289)
> I wonder if an alternate solution might have the client treat hash_or_height as a string parameter instead of a JSON parameter, and update the server to accept height strings?

Even if there is, there are other use cases to taking lists and strings as parameters. See: https://github.com/bitcoin/bitcoin/pull/32468

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

Please use the new class and variable naming style.

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

Please use the new variable naming style.

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

We can use `contains` here rather than `count() > 0`

💬 fjahr commented on pull request "wallet: Remove isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#issuecomment-3208236720)
reACK be776a1443fdf1a72e0d363c1566d71cb0cda8b5

💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2289424563)
Removed

🤔 murchandamus reviewed a pull request: "[29.x] 33106 backport and final changes for rc2"
(https://github.com/bitcoin/bitcoin/pull/33226#pullrequestreview-3138488628)
crACK 0034dcfba9dc599449e7569ed1b30e58d4f4434f

Had previously reviewed https://github.com/bitcoin/bitcoin/pull/33106, read the code changes here again, and compared to the original commits in master.

👍 brunoerg approved a pull request: "[29.x] 33106 backport and final changes for rc2"
(https://github.com/bitcoin/bitcoin/pull/33226#pullrequestreview-3138505376)
crACK 0034dcfba9dc599449e7569ed1b30e58d4f4434f

🤔 w0xlt reviewed a pull request: "cli: Handle arguments that can be either JSON or string"
(https://github.com/bitcoin/bitcoin/pull/33230#pullrequestreview-3138552075)
Approach ACK

📝 w0xlt opened a pull request: "net: Prevent node from binding to the same `CService`"
(https://github.com/bitcoin/bitcoin/pull/33231)
Currently, if the node starts with repeated `-bind` options (e.g. `./build/bin/bitcoind -listen -bind=0.0.0.0 -bind=0.0.0.0`), the user will receive a misleading message followed by the node shutdown:

```
[net:error] Unable to bind to 0.0.0.0:8333 on this computer. Bitcoin Core is probably already running.
[error] Unable to bind to 0.0.0.0:8333 on this computer. Bitcoin Core is probably already running.
```

And the user might spend some time looking for a `bitcoind` process or what appl
...

💬 davidgumberg commented on pull request "wallet: Remove isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2289457295)
> This is the current behavior anyways.

That's surprising!

Shouldn't we do something like:

```cpp
entry.pushKV("spendable", pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
```

instead?

Otherwise RPC docs should be more like @pablomartin4btc [below](https://github.com/bitcoin/bitcoin/pull/32523/commits/6a7aa015747e2634fe5a4b2f7fa0d104eb75c796#r2274128180) can be done in a follow-up.