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

✅ instagibbs closed a pull request: "policy: Ephemeral anchors"
(https://github.com/bitcoin/bitcoin/pull/26403)

💬 sr-gi commented on pull request "test: Extends MEMPOOL msg functional test":
(https://github.com/bitcoin/bitcoin/pull/28485#discussion_r1415936588)
I don't think it is. Did a bunch of testing locally and it may be a remnant of the old approach taken for this test, which does not longer apply.

I've reversed some of the calls to make the diff as minimal as possible

💬 maflcko commented on pull request "test: Extends MEMPOOL msg functional test":
(https://github.com/bitcoin/bitcoin/pull/28485#issuecomment-1841170942)
lgtm ACK 59e86afbcdfb9dbf52a6580246233ee501c51475

💬 theStack commented on pull request "script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks)":
(https://github.com/bitcoin/bitcoin/pull/28923#issuecomment-1841192171)
> Does this reduce our safety against memory corruption or similar?

I don't think so, at least I don't see how verifying a created signature twice in a row has any benefit over doing it only once.

⚠️ maflcko opened an issue: "Intermittent test failure in p2p_v2_transport"
(https://github.com/bitcoin/bitcoin/issues/29002)
https://drahtbot.space/temp_scratch/p2p_v2_transport_129.tar.xz

```
test 2023-12-05T14:06:56.900000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == num_peers + 1)
'''
test 2023-12-05T14:06:56.902000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):

...

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1841206307)
re-ACK [3a062b2](https://github.com/bitcoin/bitcoin/pull/28765/commits/3a062b2bdc6dc787b967947872f55131522cd2ac) the diff is mainly moving the removal of TODOs between commits.

I've noticed that the co-authorship of 3a062b2bdc6dc787b967947872f55131522cd2ac was dropped, which may have been unintended.

Also, looks like this is failing CI, but it may be unrelated.