💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3208041679)
> ### RFM?

I've been keeping my [review summary](https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3200849464) comment up to date through this discussion and I am more confident this is ready to merge now, and plan to do this shortly.

I've just added the following new sections since my last update:

>### Review
>- janb84: current ack, high level review with questions and considerations of tradeoffs

> ### Concerns
> #### conflation of different multiprocess features
>
>*
...

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

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3208120136)
Updated 3a34e006918490eb968c7b3312693e8a27fa3fde -> bce88ae28ab2cd12f32aead1fbf47153c50c3b05 ([kernelApi_59](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_59) -> [kernelApi_60](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_60), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_59..kernelApi_60))

* Fixed conflict with #33078 and got rid of the separate `BlockPointer` type.

📝 achow101 opened a pull request: "cli: Handle arguments that can be either JSON or string"
(https://github.com/bitcoin/bitcoin/pull/33230)
There are some RPCs where the argument can be either JSON that needs to be parsed, or a string that we can pass straight through. However, `bitcoin-cli` would always parse those arguments as JSON which makes for some cumbersome argument passing when using those RPCs. Notably, `hash_or_height` in `getblockstats` and `gettxoutsetinfo` do this, and results in a more cumbersome command of `bitcoin-cli getblockstats '"<hash>"'`. Otherwise, using a normal invocation of `bitcoin-cli getblockstats <hash
...

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