💬 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

💬 sr-gi commented on pull request "test: Extends MEMPOOL msg functional test":
(https://github.com/bitcoin/bitcoin/pull/28485#issuecomment-1841026089)
Addressed comments. Should be re-review ready

💬 BrandonOdiwuor commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1415830461)
fixed

💬 BrandonOdiwuor commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1415830781)
fixed

💬 BrandonOdiwuor commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1415831064)
fixed

💬 BrandonOdiwuor commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1415831346)
fixed

💬 Sjors commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#discussion_r1415835758)
cc @achow101

👋 BrandonOdiwuor's pull request is ready for review: "doc: explain what the wallet password does"
(https://github.com/bitcoin/bitcoin/pull/28974)

🤔 maflcko reviewed a pull request: "test: Extends MEMPOOL msg functional test"
(https://github.com/bitcoin/bitcoin/pull/28485#pullrequestreview-1765436169)
lgtm ACK 3f2da64a45f79033c29d08ee24147ca124a26c22

💬 maflcko commented on pull request "test: Extends MEMPOOL msg functional test":
(https://github.com/bitcoin/bitcoin/pull/28485#discussion_r1415836863)
nit: Can remove this line, now that you use wait_for_tx ?

💬 maflcko commented on pull request "test: Extends MEMPOOL msg functional test":
(https://github.com/bitcoin/bitcoin/pull/28485#discussion_r1415837675)
nit: Is this change from `send_message(msg_mempool())` to `send_and_ping(msg_mempool())` required?

💬 maflcko commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#discussion_r1415844552)
@Sjors The previous code was correct as well. However, this refactoring change is required if the warning is enabled.

👍 stickies-v approved a pull request: "build: Enable -Wunreachable-code"
(https://github.com/bitcoin/bitcoin/pull/28999#pullrequestreview-1765440924)
ACK fa8adbe7c17b16cf7ecd16eb9f3f1792e9da2876

At least on my machine, this does't catch https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1415690508 though. I tried changing the `Assert` (and `inline_assertion_check`) definition/implementation but can't seem to get that to work. Would be pretty helpful to fix, but even without it, this is still an improvement?

💬 stickies-v commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#discussion_r1415840290)
This doesn't functionally change anything except for removing dead code _if_ there's an earlier return statement.

💬 maflcko commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1841067566)
> At least on my machine, this does't catch

You'll have to use `clang`, because GCC does not have those warnings.

👍 ryanofsky approved a pull request: "wallet: Migrate entire address book entries to watchonly and solvables too"
(https://github.com/bitcoin/bitcoin/pull/28610#pullrequestreview-1765450748)
Code review ACK 02665016a5870c9fcba049b849a4086b14d08255. Nice cleanup and test!

💬 ryanofsky commented on pull request "wallet: Migrate entire address book entries to watchonly and solvables too":
(https://github.com/bitcoin/bitcoin/pull/28610#discussion_r1415847310)
In commit "wallet: Migrate entire address book entries" (02665016a5870c9fcba049b849a4086b14d08255)

This is so much more straightforward, thank you for cleaning it up. But while you are getting rid of confusing GetLabel code here, I think you should do it below too:

```diff
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4007,10 +4007,8 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
WalletBatch batch{wallet.GetDatabase()};
f
...

💬 stickies-v commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1841076279)
> You'll have to use `clang`, because GCC does not have those warnings.

I'm using clang, and it also caught the walletdb issue you fixed. It also catches e.g. `error.reset()` dead code I added. Just not the Assert.

```cpp
// ...and construct a CTxMemPool from it
std::optional<bilingual_str> error{};
return CTxMemPool{mempool_opts, error};
error.reset(); // just to add unreachable code
Assert(!error);
```

```
% make
Making all in src
GEN obj/build.h

...

💬 maflcko commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1841113048)
> Just not the Assert.

I thought I tested this, but looks like you are right. Any code from macros won't trigger the warning.