From 2c0410b20893383ec6f1c4c8cd5134e420513e80 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 08:29:44 +0000 Subject: net: fail over a stalled block fetch to another peer The 15s retry spacing only helps when several peers announce the block: the queued retries are created by inv handling, so a block announced by a single peer that then goes quiet was re-requested from nobody, and the tip stayed frozen until somebody happened to re-announce (#33). Track every block getdata in mapBlockFetch, erase the entry when the body arrives, and let SendMessages re-issue a request that has gone unanswered for BLOCK_FETCH_RETRY_SECS to the next peer that has not had a turn this round, restarting the round once every connected peer has. Client peers are skipped -- they declared they cannot serve bodies. Tracking is capped at 8 in-flight blocks so initial download, where hundreds are legitimately outstanding to one peer, is not duplicated to everybody else, and an entry is dropped after 10 minutes so a hash that was announced and never existed cannot be chased forever. The failover log line is deliberately not behind a debug category: it firing means a peer announces blocks it does not serve, which is the pathology being diagnosed. --- src/main.cpp | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) (limited to 'src/main.cpp') diff --git a/src/main.cpp b/src/main.cpp index 7b3e555..d9fe713 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -64,6 +64,31 @@ CBlockIndex* pindexBest = NULL; map mapOrphanBlocks; multimap mapOrphanBlocksByPrev; +// Block bodies we have sent a getdata for and not yet received, so the +// failover pass in SendMessages can tell "requested and being served" from +// "requested and gone quiet" and re-issue the request to a different peer +// (#33). Keyed by block hash; setAsked is the peers already tried since the +// round last restarted, so the retry rotates instead of hammering the +// announcer that is not serving. Guarded by cs_main, which both the message +// handlers and SendMessages already hold. +// +// Capped small on purpose. At the tip there is one block in flight; during +// initial download there are legitimately hundreds outstanding to one peer, +// and rotating those would duplicate the whole download to every other peer. +// Anything past the cap is simply not tracked and behaves as before. +struct CBlockFetch +{ + int64 nTimeFirstAsked; // when the first getdata for this block went out + int64 nTimeAsked; // when the latest getdata went out + set setAsked; // peers tried since the round last restarted +}; +map mapBlockFetch; +static const int BLOCK_FETCH_TRACK_MAX = 8; +// A block nobody has served for this long is not a fetch problem: the hash may +// have been announced and never existed. Drop the entry rather than getdata +// forever; a re-announcement recreates it. +static const int BLOCK_FETCH_GIVEUP_SECS = 10 * 60; + map mapOrphanTransactions; multimap mapOrphanTransactionsByPrev; @@ -3268,6 +3293,12 @@ bool ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) CInv inv(MSG_BLOCK, pblock->GetHash()); pfrom->AddInventoryKnown(inv); + // The body is here, so the fetch is over whatever validation says + // next -- unlike the mapAlreadyAskedFor erase below, which stays + // conditional on acceptance so a peer feeding us garbage does not + // cancel the scheduled retries for the real thing. + mapBlockFetch.erase(inv.hash); + if (ProcessBlock(pfrom, pblock.release())) { mapAlreadyAskedFor.erase(inv); @@ -3396,12 +3427,86 @@ bool SendMessages(CNode* pto) const CInv& inv = (*pto->mapAskFor.begin()).second; if (LogAcceptsCategory("net")) printf("sending getdata: %s\n", inv.ToString().c_str()); if (!AlreadyHave(txdb, inv)) + { vAskFor.push_back(inv); + // Blocks get their delivery tracked so the failover pass + // below can chase the ones that go quiet. See mapBlockFetch + // for why anything past the cap is left untracked. + if (inv.type == MSG_BLOCK && + (mapBlockFetch.count(inv.hash) || mapBlockFetch.size() < BLOCK_FETCH_TRACK_MAX)) + { + CBlockFetch& fetch = mapBlockFetch[inv.hash]; + if (fetch.nTimeFirstAsked == 0) + fetch.nTimeFirstAsked = GetTime(); + fetch.nTimeAsked = GetTime(); + fetch.setAsked.insert(pto->addr); + } + } pto->mapAskFor.erase(pto->mapAskFor.begin()); } if (!vAskFor.empty()) pto->PushMessage("getdata", vAskFor); + + // + // Message: getdata (block fetch failover) + // + // A requested block that has not arrived in BLOCK_FETCH_RETRY_SECS is + // not still in transit -- a body is at most 1 MB and somebody + // demonstrably has it -- so ask somebody else instead of waiting on + // whoever was asked first (#33). Each peer the message handler visits + // takes at most one turn per block per round; once every connected + // peer has had its turn and a further interval passes with nobody + // new to ask, the round restarts. A peer that declared itself a + // client cannot serve block bodies and is no use as a target. + if (!pto->fClient) + { + vector vFailover; + int64 nNowSecs = GetTime(); + for (map::iterator mi = mapBlockFetch.begin(); mi != mapBlockFetch.end();) + { + CBlockFetch& fetch = (*mi).second; + CInv inv(MSG_BLOCK, (*mi).first); + if (AlreadyHave(txdb, inv)) + { + // Arrived by some other road -- our own miner, an orphan + // parked in mapOrphanBlocks. Nothing left to chase. + mapBlockFetch.erase(mi++); + continue; + } + if (nNowSecs - fetch.nTimeFirstAsked > BLOCK_FETCH_GIVEUP_SECS) + { + printf("block fetch: giving up on %s -- no peer served it in %d s\n", + inv.ToString().c_str(), (int)(nNowSecs - fetch.nTimeFirstAsked)); + mapBlockFetch.erase(mi++); + continue; + } + if (nNowSecs - fetch.nTimeAsked >= BLOCK_FETCH_RETRY_SECS) + { + // The handler loop visits every peer within ~100ms, so a + // request stale by two full intervals means nobody outside + // setAsked was there to ask. Start the round over. + if (nNowSecs - fetch.nTimeAsked >= 2 * BLOCK_FETCH_RETRY_SECS) + fetch.setAsked.clear(); + if (!fetch.setAsked.count(pto->addr)) + { + // Deliberately not behind a debug category: this line + // firing is a peer announcing blocks it does not + // serve, which is the pathology of #33. + printf("block fetch failover: %s unanswered for %d s, re-requesting from %s\n", + inv.ToString().c_str(), (int)(nNowSecs - fetch.nTimeAsked), + pto->addr.ToString().c_str()); + vFailover.push_back(inv); + fetch.nTimeAsked = nNowSecs; + fetch.setAsked.insert(pto->addr); + } + } + ++mi; + } + if (!vFailover.empty()) + pto->PushMessage("getdata", vFailover); + } + } return true; } -- cgit v1.2.3 From 2e20434e563b09c5209d0a1f6784d96ed8b18851 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 16:00:59 +0000 Subject: net: keep a block tracked until its body actually validates The mapBlockFetch entry was erased before ProcessBlock ran, on the reasoning that the body had arrived so the fetch was over either way. That reasoning does not survive the fact that GetHash() covers the 80-byte header and nothing else. Any peer that saw X announced holds X's header. It can answer our getdata with that header and a transaction list of its own invention; the reply still hashes to X, so the entry was erased, and CheckBlock then rejected the body on hashMerkleRoot and stored nothing. AlreadyHave stayed false, the mapAskFor entry had already been consumed when the getdata went out, and nothing recreated the tracking -- so X was never requested from anybody again unless some peer happened to re-announce it. That is the #33 stall, reachable on demand for one message. The erase now sits inside the success branch, beside the mapAlreadyAskedFor erase it belongs with. ProcessBlock returns true in exactly two cases, block accepted and block parked as an orphan, and both store it where AlreadyHave will find it, so one erase covers both and neither needs different treatment. Of the false cases, the two already-have ones are cleared by the sweep's own AlreadyHave check on its next pass, and the rest are rejections that must keep the entry. A mismatched merkle root is also now grounds to drop the peer. Nothing honest produces it: peer traffic rides the .btf channel, which is XChaCha20-Poly1305, so a body that arrives at all arrived intact, and the mismatch is the peer's own construction. Checking it in the handler also rejects a forged body before CheckBlock spends a RandomX hash on it. The unconditional log line fires once per connection, because ProcessMessages keeps draining the receive buffer after fDisconnect is set and a peer that packs many forged bodies into one buffer must not get a line for each. --- src/main.cpp | 50 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 5 deletions(-) (limited to 'src/main.cpp') diff --git a/src/main.cpp b/src/main.cpp index d9fe713..8926d57 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3293,16 +3293,56 @@ bool ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) CInv inv(MSG_BLOCK, pblock->GetHash()); pfrom->AddInventoryKnown(inv); - // The body is here, so the fetch is over whatever validation says - // next -- unlike the mapAlreadyAskedFor erase below, which stays - // conditional on acceptance so a peer feeding us garbage does not - // cancel the scheduled retries for the real thing. - mapBlockFetch.erase(inv.hash); + // GetHash() covers the 80-byte header and nothing else, so the hash + // coming out right does not make this the block we asked for. Every + // peer that saw X announced holds X's header, and can answer our + // getdata with that header bolted onto a transaction list of its own + // invention -- the reply still hashes to X. hashMerkleRoot is the only + // thing binding the header to the body, so check it here, before the + // mapBlockFetch entry is touched and before CheckBlock spends a + // RandomX hash on a body already known to be forged. + // + // There is no honest way to arrive here. Peer traffic rides the .btf + // channel, which is XChaCha20-Poly1305, so a body that arrives at all + // arrived intact: a mismatch is the peer's own construction, not line + // noise. Drop the peer and leave the fetch outstanding so the failover + // sweep asks somebody else for the real X, rather than recording X as + // delivered on the strength of a header anyone could copy. + if (pblock->hashMerkleRoot != pblock->BuildMerkleTree()) + { + if (LogAcceptsCategory("net")) + printf("peer %s served a forged body for %s\n", + pfrom->addr.ToString().c_str(), inv.ToString().c_str()); + // One line per connection: ProcessMessages keeps draining the + // receive buffer after fDisconnect is set, so a peer that packs + // many forged bodies into one buffer must not get one log line + // for each of them. + if (!pfrom->fDisconnect) + printf("block fetch: a peer answered the request for %s with a body that is not " + "that block -- dropping it\n", inv.hash.ToString().substr(0,14).c_str()); + pfrom->fDisconnect = true; + return error("ProcessMessage() : forged block body for %s", + inv.hash.ToString().substr(0,14).c_str()); + } if (ProcessBlock(pfrom, pblock.release())) { mapAlreadyAskedFor.erase(inv); + // The body arrived and validated, so the fetch really is over: + // the block is now in mapBlockIndex, or parked in mapOrphanBlocks, + // and either way AlreadyHave answers for it from here on. + // + // This has to stay inside the success branch, next to the erase + // above. When validation rejects a body nothing is stored, + // AlreadyHave stays false, and the mapAskFor entry was already + // consumed when the getdata went out -- so erasing the tracking + // here would leave the block requested from nobody at all until + // some peer happened to re-announce it. That is exactly the stall + // of #33, and erasing before ProcessBlock made it reachable on + // demand. + mapBlockFetch.erase(inv.hash); + // A peer that just handed us a block is demonstrably at least at // that block's height. Without this, nStartingHeight is a snapshot // taken once during the version handshake and never touched again, -- cgit v1.2.3 From a215505c9e6b63883fa6058a29cc5dc7b22acd94 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 16:06:42 +0000 Subject: net: make room in the block fetch table instead of refusing new blocks Nothing on the path from inv to insertion can tell a real block hash from thirty-two random bytes. AlreadyHave only asks whether we already hold the block, and with no header there is no work to check. So one inv carrying eight hashes that never existed filled all eight slots, and because a full table refused new entries outright, no genuinely new block was tracked again until the give-up timer expired ten minutes later. One message per ten minutes from a single connection switched failover off for the whole node -- not eight wasted slots, an off-switch. A full table now evicts. Eviction on its own would just be the same attack in a new shape, so the victim is chosen by occupancy: a peer may only take a slot from a peer holding strictly more slots than itself, and otherwise recycles one of its own oldest. A peer that has filled the table is the heaviest holder by definition, so its own entries are the only ones it can displace; a peer announcing its first block holds nothing, so it always evicts from the heaviest. A genuine announcement is therefore never refused a slot, whatever an attacker holds or across however many connections it holds it. Entries carry the announcing peer for this, set once on creation -- the failover sweep asks other peers later and none of them chose to announce the hash, so none of them should be charged for it. The two timestamps also get member initialisers. The insertion path uses operator[] and tests nTimeFirstAsked == 0 to spot a fresh entry, which worked on value-initialisation alone, but only until somebody gave the struct a constructor -- and this commit gives it a member that wants one. --- src/main.cpp | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 97 insertions(+), 13 deletions(-) (limited to 'src/main.cpp') diff --git a/src/main.cpp b/src/main.cpp index 8926d57..9ab1f44 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -75,12 +75,24 @@ multimap mapOrphanBlocksByPrev; // Capped small on purpose. At the tip there is one block in flight; during // initial download there are legitimately hundreds outstanding to one peer, // and rotating those would duplicate the whole download to every other peer. -// Anything past the cap is simply not tracked and behaves as before. +// +// The cap is a shared resource and nothing on the path from inv to insertion +// can tell a real block hash from 32 random bytes -- AlreadyHave only asks +// whether we already hold the block, and there is no header to check work +// against. So the table does not refuse a new hash when it is full; it makes +// room. See MakeRoomInBlockFetch for the rule, which is what stops one peer +// filling all eight slots and switching failover off for everybody. +// +// The initialisers matter: the insertion path uses operator[] and then tests +// nTimeFirstAsked == 0 to detect a fresh entry. That worked on +// value-initialisation alone, but only for as long as nobody gave this struct +// a constructor. struct CBlockFetch { - int64 nTimeFirstAsked; // when the first getdata for this block went out - int64 nTimeAsked; // when the latest getdata went out - set setAsked; // peers tried since the round last restarted + int64 nTimeFirstAsked = 0; // when the first getdata for this block went out + int64 nTimeAsked = 0; // when the latest getdata went out + CAddress addrAnnounced; // peer whose announcement put this hash here + set setAsked; // peers tried since the round last restarted }; map mapBlockFetch; static const int BLOCK_FETCH_TRACK_MAX = 8; @@ -89,6 +101,62 @@ static const int BLOCK_FETCH_TRACK_MAX = 8; // forever; a re-announcement recreates it. static const int BLOCK_FETCH_GIVEUP_SECS = 10 * 60; +// Drop one entry so a hash announced by addrNew can be tracked. +// +// Refusing the insertion instead was a remote off-switch for the whole +// feature. An inv carrying eight hashes that never existed filled every slot +// -- nothing between inv and here can tell those from real blocks -- and no +// genuinely new block was tracked again until the give-up timer expired ten +// minutes later, re-armable for one message per ten minutes from a single +// connection. +// +// So evict instead, by occupancy: a peer may only take a slot from a peer +// holding strictly more slots than itself, and otherwise recycles one of its +// own. That is what keeps eviction from becoming the same attack in a new +// shape. A peer that has filled the table is by definition the heaviest +// holder, so its own entries are the only ones it can displace, while a peer +// announcing its first block holds nothing and therefore always evicts from +// the heaviest -- a genuine announcement is never refused a slot, whatever the +// attacker holds or however many connections it holds it from. The one case +// that costs a real fetch is eight distinct peers holding one entry each, +// where a ninth takes the oldest; that is a busy healthy node, not an attack, +// and losing the oldest of eight beats refusing the ninth. +static void MakeRoomInBlockFetch(const CAddress& addrNew) +{ + if (mapBlockFetch.empty()) + return; + + map mapHeld; + for (map::iterator mi = mapBlockFetch.begin(); mi != mapBlockFetch.end(); ++mi) + mapHeld[(*mi).second.addrAnnounced]++; + + int nMine = mapHeld.count(addrNew) ? mapHeld[addrNew] : 0; + + int nMax = 0; + CAddress addrVictim; + for (map::iterator mi = mapHeld.begin(); mi != mapHeld.end(); ++mi) + if ((*mi).second > nMax) + { + nMax = (*mi).second; + addrVictim = (*mi).first; + } + + // Nobody is holding more than this peer, so it recycles its own oldest + // rather than taking a slot off an equal. + if (nMax <= nMine) + addrVictim = addrNew; + + map::iterator miVictim = mapBlockFetch.end(); + for (map::iterator mi = mapBlockFetch.begin(); mi != mapBlockFetch.end(); ++mi) + if ((*mi).second.addrAnnounced == addrVictim && + (miVictim == mapBlockFetch.end() || + (*mi).second.nTimeFirstAsked < (*miVictim).second.nTimeFirstAsked)) + miVictim = mi; + + if (miVictim != mapBlockFetch.end()) + mapBlockFetch.erase(miVictim); +} + map mapOrphanTransactions; multimap mapOrphanTransactionsByPrev; @@ -3470,16 +3538,32 @@ bool SendMessages(CNode* pto) { vAskFor.push_back(inv); // Blocks get their delivery tracked so the failover pass - // below can chase the ones that go quiet. See mapBlockFetch - // for why anything past the cap is left untracked. - if (inv.type == MSG_BLOCK && - (mapBlockFetch.count(inv.hash) || mapBlockFetch.size() < BLOCK_FETCH_TRACK_MAX)) + // below can chase the ones that go quiet. + if (inv.type == MSG_BLOCK) { - CBlockFetch& fetch = mapBlockFetch[inv.hash]; - if (fetch.nTimeFirstAsked == 0) - fetch.nTimeFirstAsked = GetTime(); - fetch.nTimeAsked = GetTime(); - fetch.setAsked.insert(pto->addr); + // Full, and this is a hash we are not already tracking: + // make room rather than drop it on the floor, or one peer + // announcing eight hashes turns failover off for everyone. + if (!mapBlockFetch.count(inv.hash) && + mapBlockFetch.size() >= BLOCK_FETCH_TRACK_MAX) + MakeRoomInBlockFetch(pto->addr); + + if (mapBlockFetch.count(inv.hash) || + mapBlockFetch.size() < BLOCK_FETCH_TRACK_MAX) + { + CBlockFetch& fetch = mapBlockFetch[inv.hash]; + if (fetch.nTimeFirstAsked == 0) + { + fetch.nTimeFirstAsked = GetTime(); + // Who is answerable for this hash existing. Set + // once, on creation: the failover sweep asks other + // peers later, and none of them chose to announce + // it, so none of them should be charged for it. + fetch.addrAnnounced = pto->addr; + } + fetch.nTimeAsked = GetTime(); + fetch.setAsked.insert(pto->addr); + } } } pto->mapAskFor.erase(pto->mapAskFor.begin()); -- 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/main.cpp') 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 From 42c840036f35a52beec27422d92457e7e530bdb1 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 16:12:13 +0000 Subject: net: stop the failover log naming a peer on every re-request The line was unconditional on the argument that failover firing is the pathology of #33 and nobody could otherwise see it. That much is right. Printing a peer address every time it fires is not: a peer decides how often the line is written, by announcing hashes it will not serve, and an address is the one thing the .btf transport otherwise takes trouble not to spread around. Sustained, that was tens of lines a minute, each naming a peer, driven entirely from outside. Split it. The per-peer detail -- which block, how long, which peer is being tried next -- moves behind the net category, where the rest of the getdata logging already is. What stays unconditional is a count with no addresses in it, over a window no peer can shorten: the first failover after a quiet spell still prints immediately, because the change from "blocks arrive" to "blocks are announced and not served" is the event worth seeing, and everything after it is summarised once a minute. Worst case is now one line per minute regardless of what a peer does, instead of one per re-request. --- src/main.cpp | 44 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-) (limited to 'src/main.cpp') diff --git a/src/main.cpp b/src/main.cpp index be89416..a51b5ae 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -101,6 +101,21 @@ static const int BLOCK_FETCH_TRACK_MAX = 8; // forever; a re-announcement recreates it. static const int BLOCK_FETCH_GIVEUP_SECS = 10 * 60; +// Failover firing at all is worth saying out loud: it means peers are +// announcing blocks they then do not serve, which is the pathology of #33 and +// the thing nobody could otherwise see. Naming the peer on every re-request is +// a different matter. A peer can drive that line as often as it likes by +// announcing hashes it will not serve, and what it prints is a peer address, +// which is the one thing the .btf transport otherwise takes trouble not to +// spread around. So the per-peer detail sits behind the net category and the +// unconditional line is a count over a window no peer can shorten: the first +// failover after a quiet spell prints at once, because the change from "blocks +// arrive" to "blocks are announced and not served" is the event worth seeing, +// and everything after it is summarised once a minute. +static const int BLOCK_FETCH_REPORT_SECS = 60; +static int64 nBlockFetchWindowStart = 0; +static int nBlockFetchFailovers = 0; + // 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 @@ -3637,12 +3652,29 @@ bool SendMessages(CNode* pto) fetch.setAsked.clear(); if (!fetch.setAsked.count(pto->addr)) { - // Deliberately not behind a debug category: this line - // firing is a peer announcing blocks it does not - // serve, which is the pathology of #33. - printf("block fetch failover: %s unanswered for %d s, re-requesting from %s\n", - inv.ToString().c_str(), (int)(nNowSecs - fetch.nTimeAsked), - pto->addr.ToString().c_str()); + // Which block, how long, and which peer we are trying + // next. Behind the category because it names a peer + // and a peer decides how often it is written. + if (LogAcceptsCategory("net")) + printf("block fetch failover: %s unanswered for %d s, re-requesting from %s\n", + inv.ToString().c_str(), (int)(nNowSecs - fetch.nTimeAsked), + pto->addr.ToString().c_str()); + + // The unconditional half: no addresses, and at most + // one line per BLOCK_FETCH_REPORT_SECS however hard a + // peer tries. See the counters for the reasoning. + nBlockFetchFailovers++; + if (nNowSecs - nBlockFetchWindowStart >= BLOCK_FETCH_REPORT_SECS) + { + printf("block fetch: %d block request%s went unanswered and %s been " + "re-asked of other peers (%d tracked)\n", + nBlockFetchFailovers, nBlockFetchFailovers == 1 ? "" : "s", + nBlockFetchFailovers == 1 ? "has" : "have", + (int)mapBlockFetch.size()); + nBlockFetchWindowStart = nNowSecs; + nBlockFetchFailovers = 0; + } + vFailover.push_back(inv); fetch.nTimeAsked = nNowSecs; fetch.setAsked.insert(pto->addr); -- cgit v1.2.3