dcrubro/skalacoin
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Let a node that is behind adopt the peer's branch, and fix two bugs that only appear once a reorg actually applies

Browse Source 
Reported: a node at height N+10 on its own short fork, with the peer at N+500,
could never sync. It located the fork point correctly, pooled the branch, and
then refused it forever. At 90s blocks a node is 960 blocks behind after a day
offline, so this is the normal case, not an edge case.

== Why it could never recover ==

The reorg delay was served by LOCAL chain growth alone:

    elapsed = tipHeight - observedTip

That only makes sense for a node whose tip is advancing. A node that is behind
has a frozen tip precisely because it is rejecting the branch, so elapsed stays
0 forever while penalty(5) = 42. Both escape hatches also fail: mining out of it
means extending a fork nobody accepts, and the IBD exemption never fires for a
node that is mining, because mining keeps its tip fresh.

Being 490 blocks behind is being behind, not a reorg contest. Refusing to adopt
protects nothing while the node falls a further 40 blocks behind every hour.

The delay is now satisfiable by EITHER side making progress:

    localGrowth = tipHeight - observedTip
    branchLead  = candidateTip - tipHeight        (0 if not ahead)
    elapsed     = max(localGrowth, branchLead)

A branch already extending `penalty` blocks past our tip has demonstrated
exactly what the delay asks for, and every one of those blocks carries PoW we
validated ourselves. Requiring us to independently produce the same amount is
demanding the same proof twice. Only VALIDATED blocks count -- a peer's
advertised height is not evidence and never reaches this code.

It degrades correctly in both directions: a mining node's tip advances, so an
attacker must outpace it by penalty blocks; a node that is only observing
follows the heaviest chain, which is what an observer should do.

== The window has to keep pulling the branch ==

Every forked delivery reset nextReq back to our own tip, so the window
re-requested the same eight heights forever and the pool never grew past the
window size -- branchLead could not rise even in principle. The backward walk
now runs ONCE to establish linkage, then the window marches forward pooling the
branch, retrying adoption per window-full with a final attempt when it drains.

== sync force ==

Operator override that skips the delay for one sync, for a node whose chain is
known to be the wrong one. Threaded explicitly (Chain_ReplaceBranch gains
bypassPenalty, OrphanPool_AttemptAttachForced) rather than through a global, so
nothing a peer sends can reach it. Linkage, work comparison and atomicity still
apply -- it waives only the waiting, and says so loudly in the log.

== Bug found in testing: PoW is branch-relative ==

A block's epoch seed is the last block of the previous epoch ON ITS OWN BRANCH.
Validating a competing branch's block against OUR epoch seed does not merely
fail to resolve: when the chains diverge before the boundary it resolves to the
WRONG seed and rejects a perfectly valid block. Any fork spanning an epoch
boundary was therefore impossible to assemble -- the branch could not grow past
the boundary block, so branchLead stalled one short of it.

The receive path now does self-contained checks only (Block_HasValidStructure:
merkle, transactions, vote, non-empty). Proof of work moved to
Chain_AddBlockLocked, at the point a block joins the chain, where the branch
context is real -- the rollback has put its ancestors in place by then. That is
where the invariant belongs and it removes a duplicate check rather than adding
one. Needs Chain_DagParamsForHeightLocked, because Chain_AddBlockLocked already
holds chainLock for writing and the lock is not recursive.

Consequence worth knowing: the orphan pool can now hold blocks whose work has
not been verified, bounded by MAX_ORPHAN_BLOCKS (512). Each still had to pass
merkle and full transaction/signature validation, and none can reach the chain
unverified.

This bug also affected plain forward sync across block 350000; it was masked
because appending keeps the boundary block in the chain.

== Bug found in testing: stale DAG accepted as current ==

Block_PowHashHeavy matched on epoch index and size but not the seed. A DAG's
content is a function of (seed, size); the epoch index is a label for it. A
reorg is exactly the event that changes the seed while leaving index and size
untouched, so mid-apply the miner's context still held a DAG built from the
PRE-reorg seed, the guard passed, and a valid block was hashed against the wrong
lanes. g_dagSeed now records what each DAG was generated from and both
Block_EnsureAutolykos2Dag and Block_PowHashHeavy compare it.

== Bug found in testing: double free on the failed-apply path ==

SIGABRT in the allocator: free_tiny_botch -> DynArr_destroy -> Block_Destroy ->
Chain_FreeBlockArray -> Chain_ReplaceBranch.

DynArr_push_back stores the struct BY VALUE, so the chain's element and the
caller's block_t share one transactions pointer. Three places free that array
through the chain's copy -- Chain_ClearBlocks, Chain_RollbackToHeightLocked and
Chain_SaveToFile -- and each NULLs only the chain's side, leaving any caller
wrapper dangling. Whether a caller then had to use free() or Block_Destroy() was
a convention carried in comments at every call site plus a consumed-count passed
into Chain_FreeBlockArray. Chain_ReplaceBranch reset that count to 0 after
rolling back a failed apply, which told the cleanup to Block_Destroy exactly the
blocks whose arrays the rollback had just freed.

Rather than fix the count, the aliasing is now safe by construction:
Chain_AddBlockLocked clears the CALLER's transactions pointer immediately after
the push. Since DynArr_destroy(NULL) is a no-op, free(wrapper) and
Block_Destroy(wrapper) become equivalent and both safe regardless of what later
frees the chain's copy. The consumed-count parameter and both counters are gone
-- the thing that could be got wrong no longer exists -- and all call sites are
unified on Block_Destroy.

Placement is deliberate: immediately after the push, not at the end on success.
The ledger pass can fail with the block already in the chain, returning false to
a caller that destroys its wrapper on failure -- OrphanPool_ExtendTip does
exactly that, a third live instance not yet triggered.

Two follow-ons the refactor forced, both improvements anyway: MineAndAppendBlock
read the coinbase for its log line after the add (hoisted above it), and the
success log printed the caller's block rather than the chain's copy.

== Also ==

The deferral line is rate-limited. The maintenance thread retries pooled
branches once a second and elapsed only changes when something moves, so it
printed an identical line every second -- forever, on a node that is not mining.
It now reports each distinct situation once.

== Verification ==

Synthetic fork, node A 5 deep, node B ~60 ahead, EPOCH_LENGTH=8 so the branch
crosses three epoch boundaries:

  branchLead climbs 8 -> 16 -> 28 -> 34, crosses penalty(5)=42
  Adopted competing branch of 50 block(s) at fork height 20
  sync complete: localHeight=70
  Chain OK

Repeated under AddressSanitizer: adopted 47 blocks, 0 ASan errors on both nodes.
This matters because the refactor rewrites the exact cleanup path the SIGABRT
came from, and a double free that no longer aborts would otherwise pass silently.

Unit suites pass, including a new assertion "heavy path refuses a DAG built from
a different seed" -- the direct regression for the stale-DAG bug.

Harness note: each node needs its OWN wallet. With a shared one both pay the
same coinbase address, produce identical merkle roots, and at easy difficulty
mine byte-identical blocks -- the fork test silently became a catch-up test.

== Still untested ==

The restore-after-failed-apply path is no longer naturally reachable now that
the two bugs above are fixed, so it needs deliberate corruption to exercise.
Test B (branch only slightly ahead must still DEFER), test C (sync force), and a
TSan pass over the changed paths are outstanding.
This commit is contained in:
dcrubro
2026-08-02 19:36:09 +02:00
parent 01c44731ef
commit 5eaf0b699c
8 changed files with 349 additions and 115 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/block/block.c
+37 -17
View File
@@ -16,6 +16,10 @@
static Autolykos2Context* g_autolykos2Ctx = NULL;
static pthread_mutex_t g_powCtxLock = PTHREAD_MUTEX_INITIALIZER;
static uint64_t g_dagEpoch = 0;
// The seed the current DAG was generated from. Matching on epoch index and size is NOT enough: a
// reorg replaces the block an epoch's seed is derived from while leaving the epoch index and size
// unchanged, so a stale DAG would still look current and silently hash against the wrong lanes.
static uint8_t g_dagSeed[32];
static bool g_dagReady = false;
// Caller must hold `g_powCtxLock`.
@@ -50,9 +54,11 @@ bool Block_EnsureAutolykos2Dag(uint64_t epochIndex, size_t dagBytes, const uint8
pthread_mutex_lock(&g_powCtxLock);
// Already built for this epoch at this size: generation is seconds of work, so never redo it.
// Already built from exactly this seed at this size: generation is seconds of work, never redo
// it. The seed has to be part of the test -- see g_dagSeed.
if (g_dagReady && g_autolykos2Ctx && g_dagEpoch == epochIndex &&
Autolykos2_DagSize(g_autolykos2Ctx) == dagBytes) {
Autolykos2_DagSize(g_autolykos2Ctx) == dagBytes &&
memcmp(g_dagSeed, seed32, 32) == 0) {
pthread_mutex_unlock(&g_powCtxLock);
return true;
}
@@ -70,6 +76,7 @@ bool Block_EnsureAutolykos2Dag(uint64_t epochIndex, size_t dagBytes, const uint8
const bool ok = Autolykos2_DagAllocate(ctx, dagBytes) && Autolykos2_DagGenerate(ctx, seed32);
if (ok) {
g_dagEpoch = epochIndex;
memcpy(g_dagSeed, seed32, 32);
g_dagReady = true;
}
@@ -77,17 +84,22 @@ bool Block_EnsureAutolykos2Dag(uint64_t epochIndex, size_t dagBytes, const uint8
return ok;
}
bool Block_PowHashHeavy(const block_t* block, uint64_t epochIndex, size_t dagBytes, uint8_t outHash[32]) {
if (!block || !outHash) {
bool Block_PowHashHeavy(const block_t* block, uint64_t epochIndex, size_t dagBytes,
const uint8_t seed32[32], uint8_t outHash[32]) {
if (!block || !seed32 || !outHash) {
return false;
}
pthread_mutex_lock(&g_powCtxLock);
// Checking the epoch and size here, rather than trusting the caller to have built the right
// DAG, is what makes this impossible to misuse: an unbuilt or stale DAG yields false and the
// caller falls back to deriving the lanes from the seed.
// Verifying the SEED here, not just the epoch and size, is what makes this impossible to
// misuse. A reorg changes the block an epoch's seed is derived from while the epoch index and
// size stay put, so an epoch+size check alone happily accepts a DAG built from the pre-reorg
// seed and returns a hash for the wrong lanes -- which shows up as a valid block failing PoW
// while a branch is being applied. A mismatch yields false and the caller derives the lanes
// from the seed instead.
const bool usable = g_dagReady && g_autolykos2Ctx && g_dagEpoch == epochIndex &&
Autolykos2_DagSize(g_autolykos2Ctx) == dagBytes;
Autolykos2_DagSize(g_autolykos2Ctx) == dagBytes &&
memcmp(g_dagSeed, seed32, 32) == 0;
const bool ok = usable &&
Autolykos2_Hash(
g_autolykos2Ctx,
@@ -254,7 +266,7 @@ bool Block_HasValidProofOfWorkWithParams(const block_t* block, uint64_t epochInd
// from the epoch seed. The two produce identical hashes, so which one runs is invisible to
// consensus; only speed differs.
uint8_t hash[32];
if (!Block_PowHashHeavy(block, epochIndex, dagBytes, hash) &&
if (!Block_PowHashHeavy(block, epochIndex, dagBytes, seed32, hash) &&
!Block_PowHashLight(block, dagBytes, seed32, hash)) {
// Fail CLOSED. This used to hand back a zeroed hash on any failure and compare that to the
// target -- and zero is below every target, so a DAG that was missing, mis-sized or failed
@@ -383,16 +395,24 @@ bool Block_ValidateCoinbaseAndFees(const block_t* block, uint64_t expectedCoinba
return true;
}
bool Block_IsFullyValid(const block_t* block, blockchain_t* chain) {
bool merkleValid = false;
uint8_t calculatedMerkleRoot[32];
if (block && block->transactions) {
Block_CalculateMerkleRoot(block, calculatedMerkleRoot);
merkleValid = (memcmp(calculatedMerkleRoot, block->header.merkleRoot, 32) == 0);
bool Block_HasValidStructure(const block_t* block) {
if (!block || !block->transactions) {
return false;
}
return Block_HasValidVote(block) && Block_HasValidProofOfWork(block, chain) &&
Block_AllTransactionsValid(block) && DynArr_size(block->transactions) > 0 && merkleValid;
uint8_t calculatedMerkleRoot[32];
Block_CalculateMerkleRoot(block, calculatedMerkleRoot);
if (memcmp(calculatedMerkleRoot, block->header.merkleRoot, 32) != 0) {
return false;
}
return Block_HasValidVote(block) &&
Block_AllTransactionsValid(block) &&
DynArr_size(block->transactions) > 0;
}
bool Block_IsFullyValid(const block_t* block, blockchain_t* chain) {
return Block_HasValidStructure(block) && Block_HasValidProofOfWork(block, chain);
}
void Block_Destroy(block_t* block) {