From 73e77c3bc259a4c8e0a4cc5c376f1a9545439bdd Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 08:29:44 +0000 Subject: net: retry block fetches every 15s, not every two minutes AskFor keys its retry spacing off mapAlreadyAskedFor, which is global to the inv, so the two-minute gap does more than pace re-requests to one peer: it strings the getdatas for one block across its announcers two minutes apart. When the first peer asked does not serve the body, the second candidate is not asked for 120s and the ninth for 960s -- the observed 16-minute tip stall of #33, during which the pool kept broadcasting jobs against the stale tip and 5 of 12 found blocks orphaned. A block body is at most 1 MB and somebody demonstrably has it -- they announced it -- so on this timescale "not here yet" means "not coming from that peer". Blocks now retry on BLOCK_FETCH_RETRY_SECS (15s); transactions keep the two-minute pacing, which for them is the point. --- src/net.h | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) (limited to 'src/net.h') diff --git a/src/net.h b/src/net.h index 9f3c7e4..2e4d8cb 100644 --- a/src/net.h +++ b/src/net.h @@ -89,6 +89,15 @@ static const int BTF_RECV_TIMEOUT_SECS = 30 * 60; static const int BTF_SEND_STALL_SECS = 10 * 60; static const int BTF_HANDSHAKE_GRACE_SECS = 60; +// How long a requested block gets before it is asked for again, in seconds. +// Used twice: CNode::AskFor spaces the queued per-announcer block retries this +// far apart instead of the two minutes transactions get, and the failover +// sweep in main.cpp re-issues a getdata to a different peer once a sent +// request has gone unanswered this long. A block body is at most 1 MB and +// somebody demonstrably has it -- they announced it -- so on this timescale +// "not here yet" means "not coming from that peer", not "still in transit". +static const int BLOCK_FETCH_RETRY_SECS = 15; + // A serialized object cannot be larger than MAX_SIZE, so no valid network // message needs the old 256 MB header allowance. Keep the wire cap aligned with // what the deserializer can accept. How long a peer may sit on a partial @@ -795,8 +804,16 @@ public: static int64 nLastTime; nLastTime = nNow = max(nNow, ++nLastTime); - // Each retry is 2 minutes after the last - nRequestTime = max(nRequestTime + 2 * 60 * 1000000, nNow); + // Each retry is 2 minutes after the last -- and because nRequestTime + // is shared across peers, the same spacing is what strings requests + // for one inv across its announcers. For transactions that is pacing; + // for a new block it was the stall of issue #33: nine peers announce + // the block, the first one asked does not serve it, and the ninth + // candidate is not even asked until sixteen minutes in, with the pool + // mining on the stale tip the whole time. Blocks retry on the fetch + // timescale instead. + int64 nRetryUsecs = (inv.type == MSG_BLOCK ? BLOCK_FETCH_RETRY_SECS : 2 * 60) * 1000000LL; + nRequestTime = max(nRequestTime + nRetryUsecs, nNow); mapAskFor.insert(make_pair(nRequestTime, inv)); } -- cgit v1.2.3 From cd6edfc2c234db71d9b68f96702ad08a56cc3127 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 16:10:24 +0000 Subject: net: only use the 15s block retry spacing at the tip nRequestTime lives in mapAlreadyAskedFor, which is global to the inv, so the spacing AskFor applies is not per-peer pacing: it is what strings the getdatas for one block across its announcers. Shortening it to 15s was right at the tip and wrong everywhere else, and the eight-entry cap that made the failover table safe for catch-up does not cover this at all -- it is a different mechanism. At two minutes a second announcer's queued request almost always fired after the block had arrived, so AlreadyHave dropped it. At fifteen seconds, with a peer working through a 500-deep queue and block N legitimately minutes away, the second announcer is asked at +15s, the third at +30s, and the bulk download arrives once per announcer -- up to eight times over, with nothing bounding it. So the fast spacing is now conditional on tracking the tip, measured off the age of our own best block. Not off a peer-reported height: nStartingHeight is optional in the version message and every node released so far sends the short form, so it is -1 for essentially every peer, and where it does get filled in later it comes from the height of a block that peer served us, which during catch-up tracks our own height and would answer "at the tip" for the entire download. Block timestamps are the one clock a peer cannot move, being bounded above by CheckBlock and below by AcceptBlock's median-time rule. Thirty block intervals is the threshold: far longer than any stall worth failing over from, far shorter than catching up. Failover at the tip does not rest on this in any case -- mapBlockFetch re-issues a stalled getdata to the next peer on its own 15s timer, and that is what actually answers #33. --- src/main.cpp | 23 +++++++++++++++++++++++ src/net.h | 30 +++++++++++++++++++++++++++--- 2 files changed, 50 insertions(+), 3 deletions(-) (limited to 'src/net.h') diff --git a/src/main.cpp b/src/main.cpp index 9ab1f44..be89416 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -101,6 +101,29 @@ static const int BLOCK_FETCH_TRACK_MAX = 8; // forever; a re-announcement recreates it. static const int BLOCK_FETCH_GIVEUP_SECS = 10 * 60; +// How stale our own best block may be before we stop calling this the tip. +// Blocks target two minutes, so this is thirty intervals: far longer than any +// stall worth failing over from -- the one in #33 ran sixteen minutes -- and +// far shorter than catching up, which starts hours or days back. +static const int BLOCK_FETCH_TIP_AGE_SECS = 60 * 60; + +// Whether this node is tracking the tip rather than still catching up. +// +// Measured off the age of our own best block, not off any height a peer told +// us. nStartingHeight is optional in the version message and every node +// released so far sends the short form, so it is -1 for essentially every +// peer; and where it is filled in later it is filled in from the height of a +// block that peer served us, which during catch-up tracks our own height and +// would report "at the tip" for the whole download. Block timestamps are the +// one clock here that peers cannot move: CheckBlock refuses anything more than +// two hours ahead and AcceptBlock refuses anything at or before the previous +// median, so the only way to make this read "at the tip" falsely is to mine, +// and all that buys is the 15s spacing this code had unconditionally anyway. +bool IsBlockFetchNearTip() +{ + return pindexBest && (int64)pindexBest->nTime > GetTime() - BLOCK_FETCH_TIP_AGE_SECS; +} + // Drop one entry so a hash announced by addrNew can be tracked. // // Refusing the insertion instead was a remote off-switch for the whole diff --git a/src/net.h b/src/net.h index 2e4d8cb..64f061f 100644 --- a/src/net.h +++ b/src/net.h @@ -23,6 +23,12 @@ enum +// Defined in main.cpp. True while this node is tracking the tip rather than +// still catching up, measured off the age of our own best block -- see there +// for why that and not a peer-reported height. Declared here because net.h is +// parsed before main.h and CNode::AskFor needs it. +bool IsBlockFetchNearTip(); + bool GetMyExternalIP(unsigned int& ipRet); CNode* ConnectNodeBtf(const string& strBtfAddr); CNode* ConnectNodeBtfResolved(const string& strBtfAddr, const string& strMeeting, @@ -810,9 +816,27 @@ public: // for a new block it was the stall of issue #33: nine peers announce // the block, the first one asked does not serve it, and the ninth // candidate is not even asked until sixteen minutes in, with the pool - // mining on the stale tip the whole time. Blocks retry on the fetch - // timescale instead. - int64 nRetryUsecs = (inv.type == MSG_BLOCK ? BLOCK_FETCH_RETRY_SECS : 2 * 60) * 1000000LL; + // mining on the stale tip the whole time. + // + // Blocks retry on the fetch timescale, but only while we are at the + // tip. That is where the stall costs something -- the pool keeps + // handing out jobs against a dead tip -- and it is also where the + // spacing is nearly free: one block is in flight, and its body has + // almost always arrived before the second announcer's turn comes + // round, so AlreadyHave drops the duplicate before a getdata is sent. + // + // While catching up neither half holds. A peer working through a + // 500-deep queue can be minutes away from block N without anything + // being wrong, so at 15s the second announcer is asked at +15s, the + // third at +30s, and the bulk download arrives once per announcer -- + // up to eight times over. Nothing caps that: the eight-entry cap is on + // mapBlockFetch, which is a different mechanism from this spacing. + // Two minutes is the right number there, and it is what this line did + // before. Failover at the tip does not depend on this being 15s in any + // case: mapBlockFetch re-issues a stalled getdata to the next peer on + // its own timer, which is what actually fixes #33. + bool fFast = (inv.type == MSG_BLOCK && IsBlockFetchNearTip()); + int64 nRetryUsecs = (fFast ? BLOCK_FETCH_RETRY_SECS : 2 * 60) * 1000000LL; nRequestTime = max(nRequestTime + nRetryUsecs, nNow); mapAskFor.insert(make_pair(nRequestTime, inv)); } -- cgit v1.2.3