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

⚠️ martinus opened an issue: "Test `policyestimator_tests/BlockPolicyEstimates` failure"
(https://github.com/bitcoin/bitcoin/issues/29000)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

I have a reproducible test case failure on my machine. I've bisected it to commit 714523918ba2b853fc69bee6b04a33ba0c828bf5. Since this commit, the test case `policyestimator_tests/BlockPolicyEstimates`, but only when I use my builds script:

```sh
chrt -i 0 make -j32 check
```

`chrt` runs the make command in idle priority (I use that so I can smoothly browse while testing). I have a
...

💬 aureleoules commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1841125275)
I've been experimenting with benchmark comparisons between pulls and master on my test coverage tool corecheck (https://corecheck.dev/bitcoin/bitcoin/pulls/28674).
It seems that this pull request is negatively impacting a bunch of benchmarks:

Note that the benchmarks are being run on ARM64 CPUs, but I tested locally on an x86 and the performance loss is roughly the same.

💬 maflcko commented on issue "Test `policyestimator_tests/BlockPolicyEstimates` failure":
(https://github.com/bitcoin/bitcoin/issues/29000#issuecomment-1841149630)
You can fix it by adding `SyncWithValidationInterfaceQueue` in line 113, no?

📝 instagibbs opened a pull request: "Ephemeral Anchors"
(https://github.com/bitcoin/bitcoin/pull/29001)
Depends on https://github.com/bitcoin/bitcoin/pull/28948 and https://github.com/bitcoin/bitcoin/pull/28984

Replaces https://github.com/bitcoin/bitcoin/pull/26403 to refresh the conversation.

BIP text here: https://github.com/instagibbs/bips/blob/ephemeral_anchor/bip-ephemeralanchors.mediawiki

TODO:
1) figure out what precisely to do in a reorg when ephemeral transactions are trying to enter the mempool(and write a test)