Skip to content

[2.0.x] fix(core): count rollback depth in blocks, and restart instead of ignoring a deep rollback (backport of #1331) - #1341

Open
satran004 wants to merge 3 commits into
release/2.0.xfrom
backport/2.0.x/1331-rollback-depth-in-blocks
Open

satran004 wants to merge 3 commits into
release/2.0.xfrom
backport/2.0.x/1331-rollback-depth-in-blocks

Conversation

@satran004

Copy link
Copy Markdown
Member

Backport of #1331 to release/2.0.x. Tracked in #1339.

What it fixes: see #1331. The backport changes the same code in the same way.

How it was made: The 2 commits of #1331, no changes. Commits are cherry-picked with -x, and the changed lines were compared one by one with #1331.

Checks: 113 components/core tests pass.

Fault-injection run: the same scenarios as #1328 are running on release/2.0.x with all the backports. The results will be posted here and in #1339.

🤖 Generated with Claude Code

satran004 and others added 2 commits November 7, 2026 14:59
…instead of ignoring a deep rollback

onRollback ignored a rollback when the cursor was more than 2160 slots ahead of
the rollback point. 2160 is k, the security parameter, which counts blocks, not
slots. On mainnet and preprod that refused every rollback deeper than about 36
minutes. After an ignored rollback the session went on and wrote the blocks that
follow the rollback point on top of newer data.

- The limit is now 3k/f slots (129600 on mainnet and preprod): k blocks always
  fit in that window, so no valid rollback is refused.
- A rollback deeper than that is refused and the sync restarts from the cursor,
  instead of carrying on with that session.
- No cursor yet: nothing to roll back (was a NullPointerException).

Part of #1328.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit b9c38ad)
…fallback

Review: 3k/f slots refused valid one-block rollbacks across a gap on sparse
devnets (k=10, f=1 gives 30 slots), and allowed rollbacks of up to ~3k blocks
on mainnet. Look up the rollback point's block number in the cursor table and
compare with k. Fall back to 3k/f slots only when the point has no cursor row
(range-fetch batches write one row per batch; old rows are cleaned up).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 3d7f3d3)
@satran004 satran004 added fault-injection Found or verified by the auto-recovery fault-injection test 2.0.x Fix or backport for the release/2.0.x stable line labels Nov 7, 2026

Copy link
Copy Markdown

Initial automated review · Aster

Hi @satran004, thanks for the PR! 🙏 This is an automatic first check by an AI reviewer. It does not replace maintainer review and may be wrong.

Reviewed commit: 6365dece91a314ca0ccfce173cb6c2db2e988646

Suggestions:

Not tested: I reviewed the code and CI without running the build, tests, or a deep rollback.

@satran004

Copy link
Copy Markdown
Member Author

Fault-injection result on release/2.0.x

The same scenarios as #1328 ran on plain release/2.0.x and on release/2.0.x with all 8 backports (#1340–#1347). With the backports:

  • all 104 scenarios recovered on their own, with no manual restart (plain 2.0.x: 6 stuck);
  • no duplicate rows (plain 2.0.x: 31 scenarios);
  • only the 2 blocks that B04 skips on purpose are missing (plain 2.0.x: 9 blocks).

Full results: #1339.

This PR in the run: all 11 rollback scenarios at the tip (RB01–RB11) recovered on both builds. No scenario rolls back far enough to hit the deep-rollback path, so that part is covered by the unit tests only.

Copy link
Copy Markdown

reviewer1 — review of 6365dece91a314ca0ccfce173cb6c2db2e988646

No blocking defect found in this diff. I checked the rollback decision, cursor lookup, failure flag, restart event, and the new tests. Using stored block numbers when available fixes the slots-versus-blocks error, and stopping the session before requesting recovery is safer than continuing after a refused rollback.

Two follow-ups:

  • Exercise the refusal-to-recovery path in the combined build. The reported RB01–RB11 results explicitly never exceed 30 blocks. The unit tests verify that a deep rollback sets the error and emits an event, but do not establish that the restart finds a usable cursor and resumes with correct data. Include k+1 with a known cursor, an absent/pruned target beyond the fallback window, and a request arriving during [2.0.x] fix(core): retry a failed auto-restart instead of stopping for good (#959) (backport of #1330) #1340's debounce with the admin health checker disabled.
  • Describe 3k/f as a fallback heuristic, not the deepest possible valid rollback in slots. The exact block-count path correctly accepts a shallow rollback across a long slot gap. With sparse/pruned cursors, the same chain can instead take the refusal/restart path. Document that distinction and assert eventual recovery for it.

Review method: exact-head source/diff and unit-test inspection against the published fault-test evidence. I have not rerun the suite or a deep-rollback scenario.

The slot check is used only when the rollback point has no cursor row. It is not the deepest valid rollback in
slots: after a long run of empty slots a rollback of fewer than k blocks can be deeper. Say so in the javadoc, and
add a test that such a rollback takes the refuse-and-restart path instead of being applied.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MJrQUCWhJpLCQR5Asv7nDb
(cherry picked from commit 2348452)
@sonarqubecloud

sonarqubecloud Bot commented Nov 8, 2026

Copy link
Copy Markdown

@satran004

Copy link
Copy Markdown
Member Author

Thanks, reviewer1. Handled as follows:

  • The javadoc now calls 3k/f a fallback heuristic.
  • New test: a sparse rollback with no cursor row is refused and takes the restart path.

Commits: 93577e0 here, 2348452 on #1331.

The fixes were made in #1331 first and cherry-picked here. Each finding, its verdict and its test are in the reply on #1339. The fault suite is running again on the updated combined build.

@satran004

Copy link
Copy Markdown
Member Author

Verdict: APPROVED

reviewer1 — round 2. Reviewed 93577e0386a316fe8fae59933cb0fcd39fee2246, compared with 6365dece91a314ca0ccfce173cb6c2db2e988646.

No blocking findings remain.

Follow-up Status Reason
F1 — deep rollback recovery coverage Accepted follow-up The new sparse/no-cursor test proves refusal emits the restart request. A real >k rollback remains untested end to end, as recorded on #1339; I do not require it for this PR.
F2 — 3k/f wording Resolved The code now calls this a heuristic and describes the sparse-chain limitation.

Keep the distinction between testing refusal/restart signalling and testing the complete recovery sequence. The former is verified here.

Validation: seven module suites on isolated combined commit 121ee4599fcdc41226239b9195ce45daa47cfc9d: 366 passed, 1 skipped, 0 failures/errors (core: 219 passed, 1 skipped). This is combined-build validation, not a separate run on every backport.

Exact command
JAVA_HOME=/Users/satya/.sdkman/candidates/java/21.0.3-tem ./gradlew :components:core:test :components:common:test :stores:assets:test :stores:metadata:test :stores:script:test :stores:mir:test :stores:staking:test --no-daemon --console=plain

No implementation changes made by this review. The updated VM fault-suite results are still pending. This verdict covers the reviewed head only.

@satran004

Copy link
Copy Markdown
Member Author

Fault-injection rerun on 2.0.x with all eight backports at their approved heads (test/recovery-combined-2.0.x bbac1a25a): 104 of 104 scenarios recovered, with no duplicate rows and no unexpected missing blocks. The full report is on #1339: #1339 (comment)

For this PR:

  • All 11 rollback scenarios (RB01–RB11, rollbacks of 1, 3 and 30 blocks, combined with resets, kills and DB faults) recovered.
  • After each rollback, the block chain checked continuous apart from the block B04 skips on purpose.

The unpatched baseline is release/2.0.x (b2x), run on the same matrix.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.0.x Fix or backport for the release/2.0.x stable line fault-injection Found or verified by the auto-recovery fault-injection test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants