🤔 vasild reviewed a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1769801940)
Concept ACK. I acknowledge this PR resolves the problem. But the situation is somewhat convoluted in `master` and I am not sure this PR makes it any better. There are multiple ways to determine if we are stale:

1. `ChainstateManager::IsInitialBlockDownload()`: uses `-maxtipage` (default 24h). Once it considers out of IBD, it stays like that regardless of the tip age (why? surely it is possible to lag behind again).

2. `g_initial_block_download_completed`
2.1. Set by `Chainstate::Activat
...

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1418747776)
nit:
```suggestion
* outbound connection slots or for us to wish to prioritize keeping
```

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1420215938)
Tying together the "extra block-relay-only peers" and the "desirable service flags" seems unnecessary and maybe even wrong, because the conditions should be different. Flipping back to `false` is needed for "desirable service flags", but not for "extra block-relay-only peers" because then this code will be executed more than once:

https://github.com/bitcoin/bitcoin/blob/1d9da8da309d1dbf9aef15eb8dc43b4a2dc3d309/src/net_processing.cpp#L5268-L5269

Which seems innocent, but is confusing and lo
...

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1420201385)
naming: "close to tip" is misleading because there is no "tip" involved here. Just the block time is compared to the current time. `IsBlockRecent()` would be better. Or maybe even `size_t BlockAge(const CBlockIndex& index) { return (GetAdjustedTime() - index.Time()) / consensus.PowTargetSpacing(); }`

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1418751438)
What is meant by "the version of the peer" here?

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1420254312)
I find it confusing to have one condition to set the flag to `true` and a different condition to set it to `false`. I mean this:

```cpp
if (A) flag = true;
if (B) flag = false;
```
because, for example, it may happen that `B` is `true` and the flag is `true` (if the first `if` was not executed yet).

In this case it can be simpler:
```cpp
if (C) flag = true;
if (!C) flag = false;
// or just:
flag = C;
```
and C should be just "our highest block is older than 48h" (regardless of w
...

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1420242098)
Seems like `!fInitialDownload && ` can be removed? If `IsBlockCloseToTip()` is `true` then that block is 3h20min or younger. In this case `fInitialDownload` will always be `false` because it uses 24h, right?

💬 fanquake commented on pull request "fuzz: p2p: Detect peer deadlocks":
(https://github.com/bitcoin/bitcoin/pull/29009#issuecomment-1846978003)
cc also @mzumsande @sipa

👍 fanquake approved a pull request: "build: Require C++20 compiler"
(https://github.com/bitcoin/bitcoin/pull/28349#pullrequestreview-1772145687)
ACK fa6e50d6c79633e22ad4cfc75f56aaa40112ecbb

🚀 fanquake merged a pull request: "wallet: batch all individual spkms setup db writes in a single db txn"
(https://github.com/bitcoin/bitcoin/pull/28894)

🚀 fanquake merged a pull request: "test: Extends MEMPOOL msg functional test"
(https://github.com/bitcoin/bitcoin/pull/28485)

🚀 fanquake merged a pull request: "test: fix v2 transport intermittent test failure (#29002)"
(https://github.com/bitcoin/bitcoin/pull/29006)

✅ fanquake closed an issue: "Intermittent test failure in p2p_v2_transport"
(https://github.com/bitcoin/bitcoin/issues/29002)

✅ fanquake closed a pull request: "fuzz: p2p: Detect peer deadlocks"
(https://github.com/bitcoin/bitcoin/pull/29009)

💬 martinus commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1420319394)
Hm, as far as I know there's no easy way... I once found a bug like that where a deterministic random generator was used, and the unit test was asserting the exact output of an algorithm given a fixed but random input. On Windows the result was different because there the compiler had chosen a different evaluation order.

💬 ismaelsadeeq commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1420321030)
Updated, thanks

💬 ismaelsadeeq commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1420321687)
Fixed in https://github.com/bitcoin/bitcoin/commit/fab46cc454f94b33dda6bc68a7234b439310f358 thank you @martinus

💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1847041044)
> Did you have steps to reproduce outside of guix?

Not yet. Will get some, and open an issue upstream.

💬 maflcko commented on issue "fuzz: Left over tmp files when fuzzing with afl++":
(https://github.com/bitcoin/bitcoin/issues/28811#issuecomment-1847041403)
Not sure what to do here. There are some options:

* Forcing a static directory path per fuzzing task (Identified by target name + the CPU it runs on, or the "worker ID"), which will be cleared before the fuzz process starts.
* Detecting a low storage and then aborting the fuzzing campaign with a verbose error message, educating about the timeout?
* (something else?)

I think an early abort makes the most sense, because with frequent timeouts, it doesn't make sense to continue the fuzzing
...

📝 maflcko reopened a pull request: "fuzz: p2p: Detect peer deadlocks"
(https://github.com/bitcoin/bitcoin/pull/29009)
It may be possible that a peer connection will deadlock, due to software bugs such as https://github.com/bitcoin/bitcoin/pull/18808.

Fix this by detecting them in the fuzz target.

Can be tested by introducing a bug such as:

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 1067341495..97495a13df 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2436,3 +2436,3 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::ato
...