👍 hebasto approved a pull request: "depends: fix libmultiprocess build on aarch64"
(https://github.com/bitcoin/bitcoin/pull/28846#pullrequestreview-1765128120)
ACK 6293a3f5a2b61e4429c6016cc941e1ee3dc22eb3, tested on Fedora 37, aarch64.

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1415650951)
Sure. Will add the following comment:
```
/*change_output_size=*/ 31, // unused value, use p2wpkh output size (wallet default change type)
/*change_spend_size=*/ 68, // unused value, use p2wpkh input size (high-r signature)
```

💬 fanquake commented on pull request "depends: fix libmultiprocess build on aarch64":
(https://github.com/bitcoin/bitcoin/pull/28846#issuecomment-1840838481)
> ACK https://github.com/bitcoin/bitcoin/commit/6293a3f5a2b61e4429c6016cc941e1ee3dc22eb3, tested on Fedora 37, aarch64.

The changes here don't link after #28856:
```bash
/usr/bin/ld: cannot find -lcapnp-rpc: No such file or directory
/usr/bin/ld: cannot find -lcapnp: No such file or directory
/usr/bin/ld: cannot find -lkj-async: No such file or directory
/usr/bin/ld: cannot find -lkj: No such file or directory
collect2: error: ld returned 1 exit status
```
because by switching to CMak
...

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1415654787)
Pushed it. Thanks josi.

💬 Sjors commented on pull request "build: use macOS 14 SDK (Xcode 15.0)":
(https://github.com/bitcoin/bitcoin/pull/28622#issuecomment-1840847022)
Why did the Boost issue stop CI, but not the Guix build?

💬 fanquake commented on pull request "build: use macOS 14 SDK (Xcode 15.0)":
(https://github.com/bitcoin/bitcoin/pull/28622#issuecomment-1840848907)
Because the CI explicitly passes --enable-external-signer.

💬 Okponko commented on pull request "build: use macOS 14 SDK (Xcode 15.0)":
(https://github.com/bitcoin/bitcoin/pull/28622#issuecomment-1840853183)
![Uploading IMG_3704.jpeg…]()

💬 Okponko commented on pull request "build: use macOS 14 SDK (Xcode 15.0)":
(https://github.com/bitcoin/bitcoin/pull/28622#issuecomment-1840853720)
The Shop right here

💬 maflcko commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1415690508)
Clang has an `-Wunreachable-code`. Maybe someone can integrate this into the build system by default?

📝 0xB10C opened a pull request: "rpc: addpeeraddress tried return error on failure"
(https://github.com/bitcoin/bitcoin/pull/28998)
When trying to add an address to the IP address manager tried table, it's first added to the new table and then moved to the tried table. Previously, adding a conflicting address to the address manager's tried table with test-only `addpeeraddress tried=true` RPC would return `{ "success": true }`. However, the address would not be added to the tried table, but would remain in the new table. This caused, e.g., issue #28964.

This is fixed by new returning `{ "success": false, "error": "..." }`
...

💬 Sjors commented on pull request "Replace Boost.Process with cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1840891563)
The code changes for external signing look pretty minimal, so that's nice. I have no informed opinion on the difference between Boost Process and cpp-process. Except that the latter seems self contained.

> Which means it should be an entirely external dependency we just check for in configure…

In theory that makes sense to me as well, e.g. a fork under the bitcoin-core project, with functionality (mostly) limited to what we need. However in practice it means having to package it and get in
...

👍 ryanofsky approved a pull request: "depends: fix libmultiprocess build on aarch64"
(https://github.com/bitcoin/bitcoin/pull/28846#pullrequestreview-1765213631)
Code review ACK 6293a3f5a2b61e4429c6016cc941e1ee3dc22eb3. No changes since last review other than rebasing after #28856 merge

💬 0xB10C commented on issue "rpc_net.py intemittent failure / assert_equal(len(getrawaddrman[table_name]), len(table_info["entries"]))":
(https://github.com/bitcoin/bitcoin/issues/28964#issuecomment-1840892172)
Improving `addpeeraddress tried=true` failure behavior in #28998

💬 maflcko commented on pull request "rpc: addpeeraddress tried return error on failure":
(https://github.com/bitcoin/bitcoin/pull/28998#discussion_r1415701162)
nit: no need for those

```suggestion
"Add the address of a potential peer to an address manager table. This RPC is for testing only.",
```

💬 maflcko commented on pull request "rpc: addpeeraddress tried return error on failure":
(https://github.com/bitcoin/bitcoin/pull/28998#discussion_r1415708737)
Is a copy the right way, or does it need to be removed from the expected tried table?

Maybe add steps to reproduce the test code?

📝 maflcko opened a pull request: "build: Enable -Wunreachable-code"
(https://github.com/bitcoin/bitcoin/pull/28999)
It seems a bit confusing to write code after a `return`. This can even lead to bugs, or incorrect code, such as https://github.com/bitcoin/bitcoin/pull/28830/files#r1415372320 .

Fix all issues by enabling `-Wunreachable-code`.

This flag also enables `-Wunreachable-code-loop-increment`, according to https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code, so remove that.

💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r1415728492)
Changed `options` to a `std::set<std::string>&&`.

💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r1415729616)
Added a `HelpMessageSubOpt` function instead of a `bool` argument

👍 theStack approved a pull request: "test: Extends MEMPOOL msg functional test"
(https://github.com/bitcoin/bitcoin/pull/28485#pullrequestreview-1765277701)
ACK fb288f331277536c8b528c0eae44d81617fcbee6

💬 theStack commented on pull request "test: Extends MEMPOOL msg functional test":
(https://github.com/bitcoin/bitcoin/pull/28485#discussion_r1415732765)
nit (feel free to ignore): MiniWallet's `send_to` already returns both txid and wtxid, so one doesn't need the extra `getmempoolentry` RPC call.