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

💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r1415734175)
I've reworked this differently; wanted to keep the help order for each command to match how options are declared, and avoid using `.at()` so that `-help` still mostly works even if there's bugs in how options are declared.

💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r1415741747)
That doesn't really seem much simpler to me, and constructing strings that way doesn't seem a good match for the `bilingual_str` that `Result` contains. So going to leave that out of this PR.

💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#discussion_r1415748137)
We have plenty of `auto foo = bar` initialisations, and it at least [used to be](https://stackoverflow.com/questions/31310167/auto-and-brace-initialization-in-c11-c14) less ambiguous to write things that way, so I don't think this nit's a good one.

💬 jonatack commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1840972128)
Concept ACK

👍 hebasto approved a pull request: "build: use macOS 14 SDK (Xcode 15.0)"
(https://github.com/bitcoin/bitcoin/pull/28622#pullrequestreview-1765390165)
ACK 8ea45e626e5a0482ee11d4376f961d8126ce7c7b.

I've got the same hashes for SDK on both platforms:
- macOS:
```
% shasum -a 256 Xcode-15.0-15A240d-extracted-SDK-with-libcxx-headers.tar.gz
c0c2e7bb92c1fee0c4e9f3a485e4530786732d6c6dd9e9f418c282aa6892f55d Xcode-15.0-15A240d-extracted-SDK-with-libcxx-headers.tar.gz
```
- Linux:
```
$ sha256sum Xcode-15.0-15A240d-extracted-SDK-with-libcxx-headers.tar.gz
c0c2e7bb92c1fee0c4e9f3a485e4530786732d6c6dd9e9f418c282aa6892f55d Xcode-15.0-15A240d
...

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1415812593)
commit message is old; will fix

💬 sr-gi commented on pull request "test: Extends MEMPOOL msg functional test":
(https://github.com/bitcoin/bitcoin/pull/28485#discussion_r1415812758)
I'm taking this