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:
diff --git a/src/net.cpp b/src/net.cpp
index 48977aeadf..1de1bda9a8 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1957,7 +1957,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
conn_type = ConnectionType::BLOCK_RELAY;
} else if (GetTryNewOutboundPeer()) {
// OUTBOUND_FULL_RELAY
- } else if (nTime > nNextExtraBlockRelay && m_start_extra_block_relay_peers) {
+ } else if (nTime > nNextExtraBlockRelay && m_msgproc->CanDirectFetch()) {
// Periodically connect to a peer (using regular outbound selection
// methodology from addrman) and stay connected long enough to sync
// headers, but not much else.
diff --git a/src/net.h b/src/net.h
index 58a5b36918..c836161f83 100644
--- a/src/net.h
+++ b/src/net.h
@@ -635,6 +635,7 @@ public:
virtual bool SendMessages(CNode* pnode) = 0;
virtual void InitializeNode(CNode* pnode) = 0;
virtual void FinalizeNode(NodeId id, bool& update_connection_time) = 0;
+ virtual bool CanDirectFetch() const = 0;
protected:
/**
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index ad40d67a97..ef47d00e73 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -883,6 +883,11 @@ void RequestTx(CNodeState* state, const GenTxid& gtxid, std::chrono::microsecond
} // namespace
+bool PeerManager::CanDirectFetch() const
+{
+ return ::CanDirectFetch(m_chainparams.GetConsensus());
+}
+
// This function is used for testing the stale tip eviction logic, see
// denialofservice_tests.cpp
void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds)
@@ -1956,7 +1961,7 @@ void PeerManager::ProcessHeadersMessage(CNode& pfrom, const std::vector<CBlockHe
m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETHEADERS, ::ChainActive().GetLocator(pindexLast), uint256()));
}
- bool fCanDirectFetch = CanDirectFetch(m_chainparams.GetConsensus());
+ bool fCanDirectFetch = CanDirectFetch();
// If this set of headers is valid and ends in a block with at least as
// much work as our tip, download as much as possible.
if (fCanDirectFetch && pindexLast->IsValid(BLOCK_VALID_TREE) && ::ChainActive().Tip()->nChainWork <= pindexLast->nChainWork) {
@@ -3261,7 +3266,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
}
// If we're not close to tip yet, give up and let parallel block fetch work its magic
- if (!fAlreadyInFlight && !CanDirectFetch(m_chainparams.GetConsensus()))
+ if (!fAlreadyInFlight && !CanDirectFetch())
return;
if (IsWitnessEnabled(pindex->pprev, m_chainparams.GetConsensus()) && !nodestate->fSupportsDesiredCmpctVersion) {
@@ -4073,7 +4078,7 @@ void PeerManager::CheckForStaleTipAndEvictPeers()
m_stale_tip_check_time = time_in_seconds + STALE_CHECK_INTERVAL;
}
- if (!m_initial_sync_finished && CanDirectFetch(m_chainparams.GetConsensus())) {
+ if (!m_initial_sync_finished && CanDirectFetch()) {
m_connman.StartExtraBlockRelayPeers();
m_initial_sync_finished = true;
}
diff --git a/src/net_processing.h b/src/net_processing.h
index 6e3e032831..88bf7ff2a6 100644
--- a/src/net_processing.h
+++ b/src/net_processing.h
@@ -93,6 +93,11 @@ public:
*/
void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message);
+ /**
+ * Return whether our tip block's time is close enough to current time that we can directly fetch.
+ */
+ bool CanDirectFetch() const;
+
private:
/**
* 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.
