fix(core): store gap snapshot before writeHeadBlock to close crash window - #2476
fix(core): store gap snapshot before writeHeadBlock to close crash window#2476gzliudan wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes a crash/consensus-drop window around XDPoS gap-block snapshot creation by ensuring the masternode snapshot is generated and stored before head markers are persisted (both on normal canonical inserts and during reorgs).
Changes:
- Move gap-block masternode snapshot generation to occur before
writeHeadBlockinwriteBlockWithState. - Apply the same “snapshot-first” ordering in the reorg head-advancement loop.
- Introduce
updateM1ForBlock(block, statedb)to compute candidates/stakes directly from the committed state trie (avoiding dependence onbc.CurrentBlock()/bc.CurrentHeader()).
b1fdf32 to
d93b6cd
Compare
2e605b3 to
5b1992b
Compare
53683ba to
82052dc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
core/blockchain_xdpos_gap_test.go:245
- Blocks produced by
GenerateChainWithGenesisare finalized but not sealed (it never callsengine.Seal). For XDPoS V1, snapshot creation applies headers and callsecrecover, which requires a valid signature inheader.Extra. After switchinggapTestExtrato return a placeholder, the test chain generator should sign the header’sengine.SigHashand copy it into the last 65 bytes ofExtra(at least for the empty blocks these tests generate), otherwise snapshot updates may fail with signature recovery errors.
_, chain, _ := GenerateChainWithGenesis(gspec, engine, blocks, func(i int, block *BlockGen) {
block.SetExtra(gapTestExtra(vanity))
if gen != nil {
gen(i, block)
}
core/blockchain_xdpos_gap_test.go:230
gapTestExtraclaims to return a “well formed seal”, but it currently signs a constantcommon.Hash{}. V1 signer recovery (ecrecover) expects the signature to be over the header’s consensus signing hash, so this helper does not produce a valid seal for any real header and can cause snapshot generation to fail unexpectedly.
This issue also appears on line 241 of the same file.
sig, err := crypto.Sign(common.Hash{}.Bytes(), gapTestSealKey)
if err != nil {
panic(err)
}
return append(extra, sig...)
82052dc to
8af9fe6
Compare
…ndow
UpdateMasternodes (called via UpdateM1) ran after writeHeadBlock, leaving a
window in which the persisted head pointed at a gap block while no matching
snapshot existed in the database. A kill or restart during the lengthy
per-candidate EVM calls in UpdateM1 made that snapshot permanently missing, and
on the next boot the node failed with "Cannot find snapshot from last gap block"
on every block of the following epoch, silently dropping out of consensus.
Add storeGapSnapshot(header), which derives candidates and stakes from the
committed state trie of the given block the same way downloader.generateSnapshot
does, instead of depending on bc.CurrentBlock() or bc.CurrentHeader(). The
canonical path calls it before writeHeadBlock; the reorg path derives the
snapshots of the whole new chain up front, so a failure aborts the reorg before
it mutates anything instead of leaving it half applied.
Both paths report a failure as an error instead of log.Crit: the head is left on
a block that has a snapshot and the import is retried later, rather than taking
the node down.
UpdateM1 becomes a thin wrapper that resolves the current head and its state and
delegates to updateM1ForBlock. It no longer queries the validator contract
through bc.GetClient(), which drops the IPC dependency. The snapshot is
unchanged: getCandidateCap() returns validatorsState[_candidate].cap, the very
slot StateDB.GetCandidateCap reads, and the old "latest" resolved to the same gap
block because the call ran after writeHeadBlock under chainmu.
This closes the crash window only, two holes remain and both need getSnapshot to
rebuild a missing snapshot on demand:
- Fast sync leaves no gap snapshots below the pivot, because
commitFastSyncData bypasses writeBlockWithState.
- A reorg only gives up on the new head when the masternode set cannot be
derived. An older block of the new chain whose tries are already pruned is
skipped with a warning, because aborting there would strand the node on the
old chain forever, so that block keeps the very hole this change prevents.
Tests record the raw database write order through an ethdb wrapper and assert
that the gap snapshot is persisted before the head markers on the canonical path,
on the reorg path and under the v2 engine, that neither path advances the head
when the masternode set cannot be derived, that an aborted reorg leaves the old
chain untouched, and that UpdateM1 works without a client.
8af9fe6 to
64e8505
Compare
|
replaced by #2507 |
Proposed changes
UpdateMasternodes (called via UpdateM1) ran after writeHeadBlock, leaving a
window in which the persisted head pointed at a gap block while no matching
snapshot existed in the database. A kill or restart during the lengthy
per-candidate EVM calls in UpdateM1 made that snapshot permanently missing, and
on the next boot the node failed with "Cannot find snapshot from last gap block"
on every block of the following epoch, silently dropping out of consensus.
Add storeGapSnapshot(header), which derives candidates and stakes from the
committed state trie of the given block the same way downloader.generateSnapshot
does, instead of depending on bc.CurrentBlock() or bc.CurrentHeader(). The
canonical path calls it before writeHeadBlock; the reorg path derives the
snapshots of the whole new chain up front, so a failure aborts the reorg before
it mutates anything instead of leaving it half applied.
Both paths report a failure as an error instead of log.Crit: the head is left on
a block that has a snapshot and the import is retried later, rather than taking
the node down.
UpdateM1 becomes a thin wrapper that resolves the current head and its state and
delegates to updateM1ForBlock. It no longer queries the validator contract
through bc.GetClient(), which drops the IPC dependency. The snapshot is
unchanged: getCandidateCap() returns validatorsState[_candidate].cap, the very
slot StateDB.GetCandidateCap reads, and the old "latest" resolved to the same gap
block because the call ran after writeHeadBlock under chainmu.
This closes the crash window only, two holes remain and both need getSnapshot to
rebuild a missing snapshot on demand:
commitFastSyncData bypasses writeBlockWithState.
derived. An older block of the new chain whose tries are already pruned is
skipped with a warning, because aborting there would strand the node on the
old chain forever, so that block keeps the very hole this change prevents.
Tests record the raw database write order through an ethdb wrapper and assert
that the gap snapshot is persisted before the head markers on the canonical path,
on the reorg path and under the v2 engine, that neither path advances the head
when the masternode set cannot be derived, that an aborted reorg leaves the old
chain untouched, and that UpdateM1 works without a client.
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that