Repository navigation
fix(node-core): rewind a fork found while registering an unfinalized block - #3052
Conversation
…block Since subquery#3016, registering a block whose parent hash doesn't match the last stored block reports a fork from above the finalized height: the new block, or the last backfilled one. `getLastCorrectFinalizedBlock` then compared it with the stored blocks at or below the finalized height and asserted that their heights were equal. Whenever such a stored block existed, the assertion failed and the node exited instead of rewinding. The search now walks the chain down to each stored block's height before comparing hashes. The rewind target is still a stored block at or below the finalized height, so it is on the canonical chain whichever header the walk starts from.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Fork recovery and regression coverage packages/node-core/src/indexer/unfinalizedBlocks.service.ts, packages/node-core/src/indexer/unfinalizedBlocks.service.spec.ts, packages/node-core/src/indexer/project.service.spec.ts, packages/node-core/CHANGELOG.md |
The processing path checks for a stored fork after registration. A detected fork returns the last correct finalized block; a registration-only fork returns the closest stored record at or below finalized height, or the finalized header if no such record exists. Tests cover finalized blocks and backfill mismatches involving stored fork blocks. The project test reverses the supplied header order, and the changelog records the fix. |
Priority:
Estimated code review effort: 2 (Simple) | ~12 minutes
Change: Bug fix
Possibly related PRs
- subquery/subql#3016: Introduced backfill and fork signaling in the same service path.
Suggested reviewers: ianhe8x
Merge Risk: 
ed061
-
No actionable issue is established for this change; it is mergeable after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: rewinding a fork detected while registering an unfinalized block. Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 3 files. (1 skipped: 1 unsupported.)
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
ianhe8x
left a comment
ianhe8x
left a comment
There was a problem hiding this comment.
Thanks for tracking this down. The bug is real: with main's unfinalizedBlocks.service.ts, both new tests fail with Expect best header and checking header to be at the same height. The fork reported by registerUnfinalizedBlock (#3016) is above the stored blocks at or below the finalized height, and getLastCorrectFinalizedBlock was written for hasForked, which always passes a header at their height.
I'd prefer a different fix, for two reasons.
1. RPC cost. #3016 deliberately avoids walking back from the head by parent hash. This PR adds that walk: one getHeaderForHash per height, from the new block down to the finalized height, on every fork. On Substrate each one fetches a full block. With EVM finality lag (about 64-100 blocks), or during a GRANDPA stall, that is 100-200+ sequential requests per fork. Unlike hasForked, it has no UNFINALIZED_THRESHOLD shortcut.
2. The rewind target can be a fork block. The walk follows the parent hashes of whichever header the caller passes in. When the backfill loop finds a fork, it passes the stored previousHeader, which can itself be on the fork:
- Stored 111x and 112x branch off canonical 110. Finality moves to the canonical 111c, then the next indexed block is 115c.
- Backfill at 113 sees parent 112c != stored 112x and returns
forkHeader= stored 112x. - The walk fetches 111x, matches the stored 111x, and rewinds to a fork block. The unfinalized list is then reset, so the 111x data is never rechecked.
- On
main, the assert fires in this case instead, the node exits, andinit/hasForkedrewinds correctly on restart.
A related case already exists on main: if finality moves to 113c, the stored 112x is compared with itself and becomes the rewind target. The current tests can't catch either case, because the mock getHeaderForHash always returns the canonical parent.
Suggestion: treat the fork reported at registration as a signal only, and let hasForked decide where to rewind. hasForked already runs for every block without a registration fork. It only walks from the finalized height down to the highest stored block at or below it, which is usually the finalized block itself, so normally it needs no extra request.
async processUnfinalizedBlockHeader(header?: Header): Promise<Header | undefined> {
const registrationFork = header ? await this.registerUnfinalizedBlock(header) : undefined;
const forkedHeader = await this.hasForked();
if (forkedHeader) {
// A stored block at or below the finalized height is off the finalized chain: same path as before #3016
return this.getLastCorrectFinalizedBlock(forkedHeader);
}
if (registrationFork) {
// The fork is above the finalized height, and hasForked just verified the highest stored block at or below it
return this.getClosestRecord(this.finalizedBlockNumber) ?? this.finalizedHeader;
}
await this.deleteFinalizedBlock();
}getLastCorrectFinalizedBlockonly ever gets a level header fromhasForked, so the assert holds and no walk from the head is needed.- The header returned by the backfill is no longer used as a reference, which also fixes the self-match case above.
- Open question: when there is no stored block at or below the finalized height, should it rewind to
finalizedHeader(as sketched) or keep the existinglastCheckedBlockHeightfallback?
For tests, please keep your two new cases, and add one where the walk would start from a stored fork block, as in the scenario above. That needs the mock getHeaderForHash to return fork parents for fork hashes.
…tion fork A fork reported while registering can come from above the finalized height or from a stored fork block, so walking back from it can cost a request per block and can land on the fork. The stored blocks at or below the finalized height are now checked against the finalized chain instead, as for any other block, and the highest one that matches is the rewind target, or the finalized block when there is none. Orders the unfinalized blocks in a project service test as they are stored.
0620e90 to
ed06134
Compare
|
Thanks @ianhe8x , that's a better fix, and you're right about the backfill case. I added a test for it and it fails on the old version. I've pushed your approach as a separate commit so you can see what changed. A few notes:
I've updated the description to match. |
|
Thanks for the update. I checked it locally and it looks good to me |
…ock (#437) * Find the fork point when cached blocks have a gap below the forked block getLastCorrectFinalizedBlock stepped one chain block per cached block, so it compared cached blocks with chain blocks at the wrong heights when there was a gap between the forked block and the cached blocks. That happens when node-core stops a backfill at a fork, and with subquery#3052 the walk starts from the new block above that gap. No cached block matched, so it rewound to a POI block or blockForkReindex blocks back. The walk now goes back to the height above each cached block before comparing. Without a gap it takes the same steps and requests as before. The spec's mock API gains getBestBlockHeight, which node-core 19 calls when registering a block, and a genesis parent hash, and the blockForkReindex test expects the header that getHeaderForHeight now returns. * Update @subql/node-core to 19.4.0
Description
Since 19.2.0 (#3016), the node exits instead of rewinding when it finds a fork while registering an unfinalized block:
We hit it on Polymesh testnet within minutes of following head with unfinalized blocks enabled. On restart,
initrewinds correctly, so the data comes out right, but the node exits once for most forks. Polymesh mainnet sees about 6 forks an hour.What happens
registerUnfinalizedBlockreports a fork when a new block's parent hash doesn't match the last stored block, or when the backfill finds a break. It returns the new header or the last stored/backfilled one, andprocessUnfinalizedBlockHeaderpasses that togetLastCorrectFinalizedBlock. That compares it with the stored blocks at or below the finalized height and asserts they're at the same height. It always was for the header fromhasForked, but the one from registration is usually higher, so the assert fires.For example: a stored canonical 111, a fork 112, finality moves to 111, then the canonical 113 arrives. 113's parent isn't the stored 112, so the fork is reported from 113 and compared with the stored 111.
The fix
A fork found while registering now only tells us we need to rewind.
hasForkedalready runs for every block, so it decides where to:hasForkedfinds a stored block at or below the finalized height that isn't on the finalized chain, it's the same path as before fix: handle non-sequential unfinalized blocks with backfill #3016.getLastCorrectFinalizedBlockonly ever gets a header fromhasForked, at the same height as the stored block, so the assert holds.hasForkedhas just checked the highest stored block at or below it, so we rewind to that block. If there isn't one, we rewind to the finalized header.The first version of this PR walked the reported header down by parent hash instead. Ian pointed out that it costs a request per block and can rewind to a fork block when the backfill reports one, so I've switched to this approach.
Tests
New cases in
unfinalizedBlocks.service.spec.ts:The first three fail on
mainwith the assertion, and the fourth rewinds to 112x. The mockgetHeaderForHashnow returns fork headers for fork hashes, so the last two can follow the fork's parents.Two existing tests changed:
project.service.spec.tsfails onmainwith the same assertion. Its unfinalized blocks were listed newest first, which the service never stores, sogetClosestRecordpicked the wrong one. I've put them in ascending order.Type of change
Checklist
Summary by CodeRabbit