fix(consensus-observer): verify commit epoch before clearing blocks - #378
Open
apenzk wants to merge 2 commits into
Open
fix(consensus-observer): verify commit epoch before clearing blocks#378apenzk wants to merge 2 commits into
apenzk wants to merge 2 commits into
Conversation
- seed payload and ordered-block caches at the root epoch, then deliver a commit notification for a different epoch - assert nothing is evicted from either cache and the root does not advance - fails against current ordering (caches cleared before the epoch check); committed in failing state ahead of the fix
- move the epoch-match check ahead of the payload and ordered-block cache eviction, so a commit callback for a different epoch no longer clears caches it should leave untouched - a wrong-epoch callback now returns early without evicting anything or advancing the root
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The observer has a guard meant to reject commit notifications from the wrong epoch — but it ran after the cache eviction it was supposed to protect. So the guard rejected the notification while the damage was already done.
handle_committed_blocks did this:
A commit notification for a different epoch than the root can legitimately arrive — a stale/orphaned callback racing with an epoch transition. When it did, step 1 wiped payloads and ordered blocks the observer still needed, and the observer stalled until it recovered via state sync. The wrong-epoch
returnblocked the root from advancing but never stopped the eviction.The fix is a one-line reorder: check the epoch first, so a wrong-epoch notification returns before touching anything. Same-epoch notifications behave exactly as before. Impact is limited to observer/follower nodes — no committed ledger state is affected.
How Has This Been Tested?
New test test_handle_committed_blocks_wrong_epoch_preserves_caches. The test is added in its own commit ahead of the fix, so it can be run against both states.
Failure (old behavior). Check out the test commit (test present, fix absent) — the test fails because the caches are emptied:
Success (with the fix). Return to the branch tip — the same test passes:
git checkout fix/consensus-observer-commit-callback-epoch-guard cargo test -p aptos-consensus --lib test_handle_committed_blocks_wrong_epoch_preserves_cachesType of Change