💬 sipa commented on pull request "fuzz: Avoid timeout in bitdeque":
(https://github.com/bitcoin/bitcoin/pull/29012#issuecomment-1843087428)
utACK fad1903b8a85506378101c1f857ba47b4a058fb4

💬 dergoegge commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1417498148)
> So, with the current code, having 4 peers would never trigger it, right? Because median will always be 0.

Yes and this sort of (on accident) mimics how/when we trigger the warning on master at 5 or more samples: https://github.com/bitcoin/bitcoin/blob/dde7ac5c704688c8a9af29bd07e5ae8114824ce7/src/timedata.cpp#L77

💬 dergoegge commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1417498497)
Done

💬 dergoegge commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1417504050)
I see your point but I think I would like to keep discussion about improving the warning condition to a future PR. The current adjusted time mechanism has the same flaw and is even worse as it stops sampling entirely after 200 samples.

Maybe we can collect some stats on how often outbound connections rotate and how many samples from which connections are actually useful to inform such a change.

---

I will add the release note.

💬 achow101 commented on pull request "test: Fix test by checking the actual exception instance":
(https://github.com/bitcoin/bitcoin/pull/28989#issuecomment-1843111898)
ACK 55e3dc3e03510e97caba1547a82e3e022b0bbd42

📝 ismaelsadeeq opened a pull request: "test: doc: follow-up #28368"
(https://github.com/bitcoin/bitcoin/pull/29013)
This is a simple PR that does two things
1. Fixes #29000 by waiting for the fee estimator to catch up after `removeForBlock` calls before calling `estimateFee` in the `BlockPolicyEstimates` unit test.

2. Addressed some outstanding review comments from #28368
- Updated `NewMempoolTransactionInfo::m_from_disconnected_block` to `NewMempoolTransactionInfo::m_mempool_limit_bypassed` which now correctly indicates what the boolean does.
- Changed input of `processTransaction`'s tx_info `m_su
...

💬 instagibbs commented on pull request "test: Extends MEMPOOL msg functional test":
(https://github.com/bitcoin/bitcoin/pull/28485#discussion_r1417479419)
If I comment out these lines the test still passes. Seems wrong based on text below at https://github.com/bitcoin/bitcoin/pull/28485/files#diff-c5c320cd909288d7cf2d82632c6bafcb9085413bfddf5edf361f288dbd76e0cbR153 ?

Based on my look at the code, in SendMessages we set m_last_inv_sequence = m_mempool.GetSequence() immediately on connection, even if we had no INVs to send. This means new connections can ask for anything historical, regardless of mempool message?

🤔 instagibbs reviewed a pull request: "test: Extends MEMPOOL msg functional test"
(https://github.com/bitcoin/bitcoin/pull/28485#pullrequestreview-1767820295)
I haven't done much review of https://github.com/bitcoin/bitcoin/pull/27675 , but I can't make the test case fail sensibly by commenting out the mempool message, and by the reading of the code.

🚀 achow101 merged a pull request: "test: Fix test by checking the actual exception instance"
(https://github.com/bitcoin/bitcoin/pull/28989)

💬 maflcko commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1417521141)
nit: use the format that clang-tidy understands and that is used elsewhere in the code?

```suggestion
/*m_mempool_limit_bypassed=*/ false,
```

💬 achow101 commented on pull request "rpc: encryptwallet help, mention HD seed rotation and backup requirement":
(https://github.com/bitcoin/bitcoin/pull/28980#issuecomment-1843125339)
ACK ca09415e630f0f7de9160cab234bd5ba6968ff2d

🚀 achow101 merged a pull request: "rpc: encryptwallet help, mention HD seed rotation and backup requirement"
(https://github.com/bitcoin/bitcoin/pull/28980)

🤔 hebasto reviewed a pull request: "build: Introduce internal kernel library"
(https://github.com/bitcoin/bitcoin/pull/28690#pullrequestreview-1767977106)
Approach ACK 2086d1d4c3f40cce34647995ead4bff22967ffd8.

Still reviewing the 4th commit (983d0978a973a12c3128b2c8e13b73ed08155e67).

🤔 sr-gi reviewed a pull request: "p2p: Fill reconciliation sets (Erlay)"
(https://github.com/bitcoin/bitcoin/pull/28765#pullrequestreview-1767980730)
tACK [dca7990](https://github.com/bitcoin/bitcoin/pull/28765/commits/dca7990c3b8b8ecb963c068b4a8a215e0c2b3cd0)

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1417583518)
Ups, I forgot to add this.

nit: Shouldn't this also be `Wtxid&` for consistency with the rest of the interface?

This is non-blocking, but I realized when reviewing the last changes to the tests

💬 dergoegge commented on pull request "wip: Split fuzz binary":
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1843194507)
> Are you sure on this? I don't know how afl or fuzz-introspector detect reachable code paths, but I'd be surprised if the static analysis can follow a function pointer hidden in a map.

Yea maybe not... should be quite easy to avoid the map.

I'll try to setup a fuzz-introspector instance at some point, I have a feeling it'll still be a while before we can use oss-fuzz's instance.

💬 theuni commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1843211066)
Hmm, playing around with this code on godbolt, it hardly ever compiles down to bswaps. I'm not sure what changed from when I was initially investigating. Will have a look.

💬 dergoegge commented on pull request "fuzz: p2p: Detect peer deadlocks":
(https://github.com/bitcoin/bitcoin/pull/29009#issuecomment-1843215697)
> Happy to review a pull request, or happy to include any patch here, that compiles, if someone writes it.

feel free to pick https://github.com/dergoegge/bitcoin/commit/9f265d88253ed464413dea5614fa13dea0d8cfd5

👍 dergoegge approved a pull request: "fuzz: Avoid timeout in bitdeque"
(https://github.com/bitcoin/bitcoin/pull/29012#pullrequestreview-1768029847)
utACK fad1903b8a85506378101c1f857ba47b4a058fb4

💬 achow101 commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#issuecomment-1843222722)
ACK 3ea54e5db7d53da5afa321e1800c29aa269dd3b3