Fix windowed sync stalling against a peer on a competing branch
Reported symptom: node A (4656 blocks, last 10 mined on its own fork) syncing
from node B (5646 blocks) crawled for minutes while B re-served the whole chain
several times over. A's log was thousands of "Queued orphan BLOCK_DATA" lines
interleaved with "timed out fetching block N, giving up", sliding forward 8
blocks at a time and never reorging.
== Root cause ==
The sync loop had no channel for FETCH_BLOCK replies. It inferred that a
requested block had arrived purely from the chain growing:
if ((uint64_t)Chain_Size(chain) > h) { /* block h was applied */ }
But the reply is handled on the peer's io thread by Node_ParseAndAcceptBlock,
which correctly identifies a block whose prevHash points at the peer's branch
and files it in the orphan pool -- the chain does not grow. So a block that
ARRIVED AND WAS CORRECTLY ORPHANED was indistinguishable from a lost packet.
Every one of the 990 blocks therefore cost MAX_SYNC_RETRIES re-requests plus a
SYNC_REQUEST_TIMEOUT_MS stall before the window slid on and did it again. That
is both the crawl and the repeated serving on the peer.
Neither existing fork probe could rescue it:
* The divergence check sits downstream of a SUCCESSFUL append -- it only runs
after Chain_GetBlockCopy(chain, h) succeeds. When the fork begins at the
very first requested height nothing ever lands, so RequestForkWindow was
never reached from there.
* The no-progress fallback only runs after the entire window range drains,
i.e. after timing out through all 990 blocks, and is capped at 3 rounds.
== Delivery receipts ==
Node_NoteBlockDelivered / Node_TakeBlockDelivery / Node_ResetBlockDeliveries,
backed by a 512-slot ring with its own mutex. Recorded from the BLOCK_DATA
handler only -- BLOCK_DATA is sent solely in reply to a FETCH_BLOCK, so a
receipt means "the peer answered", independently of whether the block could
join our chain. A receipt for a height already present is refreshed in place,
so a retried request cannot leave a stale one for the next window to consume.
Receipts carry a status rather than a bool:
NODE_DELIVERY_APPENDED joined our chain
NODE_DELIVERY_DUPLICATE we already held this exact block -- common ground
NODE_DELIVERY_ORPHANED competing branch; now pooled
NODE_DELIVERY_REJECTED failed validation
DUPLICATE is what lets a backwards fork walk terminate; a plain "appended"
boolean cannot tell "we already have this" from "this is on their branch",
which is precisely the signal the walk needs.
The sync loop now treats any non-APPENDED delivery as the divergence signal and
probes immediately, instead of retrying something retrying cannot fix. Bounded
by MAX_FORK_PROBE_ROUNDS -- without that the window resets to the same height
and re-probes forever -- and the counter resets on real progress so a branch
that forks again further on can still be followed.
== RequestForkWindow is now an actual walk ==
It previously fired REORG_FETCH_DEPTH (128) requests and slept a flat
SYNC_REQUEST_TIMEOUT_MS. Two problems:
* The sleep was a race against the peer's serving rate. At the ~10 blocks/s
observed in testing, a 128-block window cannot land in 5s, so the
OrphanPool_AttemptAttach that followed ran against a half-filled pool and
reported a failure that was not real.
* It always asked for the full depth. On a shallow fork the overwhelming
majority came back as duplicates that were discarded without even entering
the pool -- a wasted full block send each.
It now descends in batches of MAX_PARALLEL_FETCHES, waits on that batch's
receipts rather than guessing, and stops at the first height the peer returns
that we already hold: that block is the fork point and everything below it is
shared. It returns whether common ground was found, so callers no longer
attempt an attach that cannot possibly link.
Measured on the reported scenario: 16 requests instead of 129, locating the
fork point at height 4645 (fork = blocks 4646..4655, exactly the 10 mined).
== IBD_TIP_AGE_BLOCKS 500 -> 20 ==
The reorg penalty is served by LOCAL chain growth, so a node that does not mine
can never serve it -- its tip does not move, and the only way it could grow is
by adopting the branch the penalty is gating. The IBD exemption is that node's
only route back, so at 500 block times it had to sit stalled for ~12.5 hours.
20 block times is ~30 minutes at a 90s target, far beyond normal Poisson block
spacing (a gap that long has probability ~e^-20), so a node genuinely following
the tip will not trip it.
This is what flipped the live test from "initialSync=no" to "initialSync=yes"
and let the reorg be adopted at all. Noted in the constant's comment: the
exemption is all-or-nothing, so lowering it further widens that hole.
== Reorg failures are now legible ==
Chain_ReplaceBranch broke silently for fork-point-beyond-tip, work computation
failure, insufficient work, snapshot failure and candidate copy failure alike,
so "not adopted" could not be diagnosed from a log. Each now says which. The
not-heavier case reports candidate vs incumbent block counts, because the
common cause is a branch that is still arriving -- a partial branch is
genuinely lighter -- and that reads very differently from a peer actually on a
weaker chain.
The sync loop's "Reorg candidate not adopted (lighter branch, or still serving
its reorg penalty)" is replaced by "Reorg not completed on this pass; branch
stays pooled for retry". The old wording was actively misleading: live testing
showed it firing on reorgs that the 1Hz maintenance thread completed a second
later.
== Verification ==
Against a real peer at 5646 blocks, from a real local chain of 4656 with a
10-block fork:
reported receipts only all changes
wall-clock minutes 123s 116s
timed-out fetches 1 per block 0 0
fork-walk requests n/a 129 16
reorg completed never maintenance first pass
final height 4656 5646 5646
fullverify - Chain OK Chain OK
The 116s is not a meaningful speedup over 123s and should not be read as one:
990 blocks at ~10 blocks/s is ~100s, so both are bound by the peer's serving
throughput rather than by anything in the sync loop. The substantive wins are
the 129 -> 16 fork-walk reduction, the reorg completing deterministically
instead of by luck, and logs that no longer report failure on success.
Also exercised: a synthetic partitioned fork (10 vs 60 blocks) confirming zero
timeouts and bounded probe rounds when the branch is correctly refused by the
penalty.
== Known limits, unchanged ==
Chain_ReplaceBranch adopts whatever is pooled at attach time (18 blocks in the
live run); the remainder arrives through normal windowed sync afterwards. That
bounds a single reorg by MAX_ORPHAN_BLOCKS.
This commit is contained in:
+20
-2
@@ -842,7 +842,9 @@ bool Chain_ReplaceBranch(blockchain_t* chain,
|
||||
do {
|
||||
const size_t tipCount = DynArr_size(chain->blocks);
|
||||
if (forkHeight > tipCount) {
|
||||
break; // fork point is beyond our chain; nothing to replace
|
||||
printf("Chain_ReplaceBranch: fork point %zu is beyond our tip %zu; nothing to replace\n",
|
||||
forkHeight, tipCount);
|
||||
break;
|
||||
}
|
||||
|
||||
if (!Chain_BranchIsLinkedLocked(chain, forkHeight, newBlocks, count)) {
|
||||
@@ -883,10 +885,17 @@ bool Chain_ReplaceBranch(blockchain_t* chain,
|
||||
uint256_t candidateWork;
|
||||
if (!Chain_ComputeWorkRange(chain, forkHeight, tipCount, &incumbentWork) ||
|
||||
!Chain_ComputeBranchWork(newBlocks, count, &candidateWork)) {
|
||||
printf("Chain_ReplaceBranch: could not compute work for the branch at height %zu\n", forkHeight);
|
||||
break;
|
||||
}
|
||||
if (uint256_cmp(&candidateWork, &incumbentWork) <= 0) {
|
||||
break; // not heavier; keep what we have
|
||||
// Very often this just means the branch is still arriving -- a partial branch is
|
||||
// genuinely lighter than what it would replace. Report the block counts so that case
|
||||
// is distinguishable from a peer that really is on a weaker chain.
|
||||
printf("Chain_ReplaceBranch: candidate at height %zu is not heavier "
|
||||
"(%zu candidate block(s) vs %zu incumbent); keeping our chain\n",
|
||||
forkHeight, count, tipCount - forkHeight);
|
||||
break;
|
||||
}
|
||||
|
||||
// Snapshot what we are about to discard so a failed apply can be undone. The in-memory
|
||||
@@ -896,6 +905,8 @@ bool Chain_ReplaceBranch(blockchain_t* chain,
|
||||
if (snapshotCount > 0) {
|
||||
snapshot = (block_t**)calloc(snapshotCount, sizeof(block_t*));
|
||||
if (!snapshot) {
|
||||
printf("Chain_ReplaceBranch: out of memory snapshotting %zu block(s); chain unchanged\n",
|
||||
snapshotCount);
|
||||
break;
|
||||
}
|
||||
|
||||
@@ -915,6 +926,10 @@ bool Chain_ReplaceBranch(blockchain_t* chain,
|
||||
}
|
||||
}
|
||||
if (!snapshotOk) {
|
||||
// Refusing here is the point: without a complete snapshot a failed apply could not
|
||||
// be undone, so we would rather not start than risk a half-replaced chain.
|
||||
printf("Chain_ReplaceBranch: could not snapshot the blocks being replaced at height %zu; "
|
||||
"refusing the reorg rather than risk an unrecoverable apply\n", forkHeight);
|
||||
break;
|
||||
}
|
||||
}
|
||||
@@ -923,6 +938,7 @@ bool Chain_ReplaceBranch(blockchain_t* chain,
|
||||
// rollback -- the caller keeps ownership of what it passed in, whatever happens here.
|
||||
candidate = (block_t**)calloc(count, sizeof(block_t*));
|
||||
if (!candidate) {
|
||||
printf("Chain_ReplaceBranch: out of memory copying %zu candidate block(s); chain unchanged\n", count);
|
||||
break;
|
||||
}
|
||||
bool copiedAll = true;
|
||||
@@ -934,6 +950,8 @@ bool Chain_ReplaceBranch(blockchain_t* chain,
|
||||
}
|
||||
}
|
||||
if (!copiedAll) {
|
||||
printf("Chain_ReplaceBranch: could not copy the candidate branch at height %zu; chain unchanged\n",
|
||||
forkHeight);
|
||||
break;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user