if you close your laptop or disconnect from the network though then yes you’re right that we’ll use these occasional peers to help us catch up, which is not the intent.
If this is the intent (to stop making these short-lived connections if we’ve fallen behind the tip), then I think we can achieve that fairly easily by removing this caching variable, making CanDirectFetch() a public method on PeerManager and calling that function whenever we need to test if we should make an additional block-relay-only connection:
0diff --git a/src/net.cpp b/src/net.cpp
1index 48977aeadf..1de1bda9a8 100644
2--- a/src/net.cpp
3+++ b/src/net.cpp
4@@ -1957,7 +1957,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
5 conn_type = ConnectionType::BLOCK_RELAY;
6 } else if (GetTryNewOutboundPeer()) {
7 // OUTBOUND_FULL_RELAY
8- } else if (nTime > nNextExtraBlockRelay && m_start_extra_block_relay_peers) {
9+ } else if (nTime > nNextExtraBlockRelay && m_msgproc->CanDirectFetch()) {
10 // Periodically connect to a peer (using regular outbound selection
11 // methodology from addrman) and stay connected long enough to sync
12 // headers, but not much else.
13diff --git a/src/net.h b/src/net.h
14index 58a5b36918..c836161f83 100644
15--- a/src/net.h
16+++ b/src/net.h
17@@ -635,6 +635,7 @@ public:
18 virtual bool SendMessages(CNode* pnode) = 0;
19 virtual void InitializeNode(CNode* pnode) = 0;
20 virtual void FinalizeNode(NodeId id, bool& update_connection_time) = 0;
21+ virtual bool CanDirectFetch() const = 0;
22
23 protected:
24 /**
25diff --git a/src/net_processing.cpp b/src/net_processing.cpp
26index ad40d67a97..ef47d00e73 100644
27--- a/src/net_processing.cpp
28+++ b/src/net_processing.cpp
29@@ -883,6 +883,11 @@ void RequestTx(CNodeState* state, const GenTxid& gtxid, std::chrono::microsecond
30
31 } // namespace
32
33+bool PeerManager::CanDirectFetch() const
34+{
35+ return ::CanDirectFetch(m_chainparams.GetConsensus());
36+}
37+
38 // This function is used for testing the stale tip eviction logic, see
39 // denialofservice_tests.cpp
40 void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds)
41@@ -1956,7 +1961,7 @@ void PeerManager::ProcessHeadersMessage(CNode& pfrom, const std::vector<CBlockHe
42 m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETHEADERS, ::ChainActive().GetLocator(pindexLast), uint256()));
43 }
44
45- bool fCanDirectFetch = CanDirectFetch(m_chainparams.GetConsensus());
46+ bool fCanDirectFetch = CanDirectFetch();
47 // If this set of headers is valid and ends in a block with at least as
48 // much work as our tip, download as much as possible.
49 if (fCanDirectFetch && pindexLast->IsValid(BLOCK_VALID_TREE) && ::ChainActive().Tip()->nChainWork <= pindexLast->nChainWork) {
50@@ -3261,7 +3266,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
51 }
52
53 // If we're not close to tip yet, give up and let parallel block fetch work its magic
54- if (!fAlreadyInFlight && !CanDirectFetch(m_chainparams.GetConsensus()))
55+ if (!fAlreadyInFlight && !CanDirectFetch())
56 return;
57
58 if (IsWitnessEnabled(pindex->pprev, m_chainparams.GetConsensus()) && !nodestate->fSupportsDesiredCmpctVersion) {
59@@ -4073,7 +4078,7 @@ void PeerManager::CheckForStaleTipAndEvictPeers()
60 m_stale_tip_check_time = time_in_seconds + STALE_CHECK_INTERVAL;
61 }
62
63- if (!m_initial_sync_finished && CanDirectFetch(m_chainparams.GetConsensus())) {
64+ if (!m_initial_sync_finished && CanDirectFetch()) {
65 m_connman.StartExtraBlockRelayPeers();
66 m_initial_sync_finished = true;
67 }
68diff --git a/src/net_processing.h b/src/net_processing.h
69index 6e3e032831..88bf7ff2a6 100644
70--- a/src/net_processing.h
71+++ b/src/net_processing.h
72@@ -93,6 +93,11 @@ public:
73 */
74 void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message);
75
76+ /**
77+ * Return whether our tip block's time is close enough to current time that we can directly fetch.
78+ */
79+ bool CanDirectFetch() const;
80+
81 private:
82 /**
83 * Potentially mark a node discouraged based on the contents of a BlockValidationState object
That approach may be preferable for a couple of reasons:
- Placing the logic that checks/sets the condition under which we’ll make additional block-relay-only peers in the same place that it makes those connections makes it much easier to reason about those state transitions. Currently
m_start_extra_block_relay_peers is set based on a loop in net_processing and then read on a timer in net.
- Caching the state internally makes it more difficult to test all the code paths. If the start_extra_block_relay_peers condition is set by a call into PeerManager, then that interface can be mocked and code paths can be hit more easily in unit/fuzz testing.
