fix(consensus): recover from Finalize-stall when a FinalizeBlock guard aborts apply#817
fix(consensus): recover from Finalize-stall when a FinalizeBlock guard aborts apply#817satyakwok wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…d aborts apply A FinalizeBlock guard (split-brain / hash-mismatch / add_block error) can break out without applying block N, parking the engine at height N phase=Finalize while the chain stays at N-1. need_new_round cannot reset it (bft.height N > current_height N-1) and Finalize has no timeout, so the engine spins on "phase timeout finalize" until a peer's block N arrives via gossip and advances the chain — minutes when the mesh is degraded. Detect the engine parked in Finalize one height ahead of the chain past a 5s threshold and reset it via new_height() to re-run consensus for the unapplied height. Safe: the bc.height() >= height skip-guard in the FinalizeBlock arms no-ops a re-finalize if the block already landed (no double-apply), and a re-proposal still needs 2/3 precommits, so safety is preserved. Adds BftEngine::phase_elapsed() for the stall detection.
phase_elapsed feeds the finalize-stall recovery in main.rs. Add a deterministic (no-sleep) unit test asserting it is tiny right after construction and after new_height, which also satisfies the codecov patch gate (the recovery block in main.rs is under the bin/ codecov-ignore).
satyakwok
force-pushed
the
fix/finalize-stall-recovery
branch
from
a3df02e to
3487641
Compare
📝 WalkthroughWalkthroughA finalize-stall liveness recovery mechanism is added to the BFT consensus engine. A new public Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/sentrix/src/main.rs`:
- Around line 1970-1987: The finalize-stall recovery path calls
bft.new_height(next_height, total_active_stake) while already at that height,
which bypasses the normal new-height initialization logic that would clear
proposed_block, last_signed_proposal, rebroadcast state, and advance past
last_sign_guard::current_state(). Since validators in Finalize phase may have
already signed prevote/precommit at the current (height, round), resetting
without proper cleanup risks equivocation (signing twice at the same height,
round, step with different LastSignBytes). Instead of directly calling
new_height on the bft_engine, either route recovery through the existing
initialization/resume logic path, or minimally capture the stalled round,
explicitly clear the transient proposal state fields, advance the sign guard
past any previously signed/stalled round, and only then proceed with resetting
the engine to ensure no new sign attempts occur at the same (height, round,
step) as previous signatures.
In `@crates/sentrix-bft/src/engine.rs`:
- Around line 3507-3518: The test function
phase_elapsed_small_after_construct_and_new_height does not follow the
established naming convention used throughout the codebase. Rename the function
to test_phase_elapsed_small_after_construct_and_new_height to add the test_
prefix that is consistently used for all other test functions in this file like
test_new_engine and test_advance_round. This ensures consistency with the
codebase naming standards.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 344dfee7-b2e1-43a9-85ba-7a83cdc128ad
📒 Files selected for processing (2)
bin/sentrix/src/main.rscrates/sentrix-bft/src/engine.rs
| if !need_new_round | ||
| && let Some(bft) = bft_engine.as_mut() | ||
| && bft.height() > current_height | ||
| && bft.phase() == BftPhase::Finalize | ||
| && bft.phase_elapsed() >= FINALIZE_STALL_RESET | ||
| { | ||
| tracing::warn!( | ||
| target: "bft::engine", | ||
| "BFT finalize-stall recovery: engine parked at height {} \ | ||
| round {} phase=Finalize for {:?} while chain at {} — \ | ||
| resetting to re-run consensus for the unapplied height", | ||
| bft.height(), | ||
| bft.round(), | ||
| bft.phase_elapsed(), | ||
| current_height, | ||
| ); | ||
| bft.new_height(next_height, total_active_stake); | ||
| } |
There was a problem hiding this comment.
Do not reset the same height without reusing the signed-round resume path.
This watchdog calls new_height(next_height, ...) while the engine is already at that same unapplied height, but it bypasses the normal new-height block that advances past last_sign_guard::current_state() and clears proposed_block / last_signed_proposal / rebroadcast state. In Finalize, this validator may already have signed prevote/precommit for the current (height, round): with the guard enabled, the re-run can get stuck on guard refusals; with it disabled, the reset can permit another sign attempt at the same (height, round, step).
Please route recovery through the existing initialization/resume logic, or minimally capture the stalled round, clear the transient proposal state, and advance the reset engine past any previously signed/stalled round before any new proposal/vote can be signed. As per coding guidelines, bin/sentrix/src/main.rs is “CONSENSUS-CRITICAL” and equivocation prevention requires “never sign two messages at the same (height, round, step) with different LastSignBytes.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/sentrix/src/main.rs` around lines 1970 - 1987, The finalize-stall
recovery path calls bft.new_height(next_height, total_active_stake) while
already at that height, which bypasses the normal new-height initialization
logic that would clear proposed_block, last_signed_proposal, rebroadcast state,
and advance past last_sign_guard::current_state(). Since validators in Finalize
phase may have already signed prevote/precommit at the current (height, round),
resetting without proper cleanup risks equivocation (signing twice at the same
height, round, step with different LastSignBytes). Instead of directly calling
new_height on the bft_engine, either route recovery through the existing
initialization/resume logic path, or minimally capture the stalled round,
explicitly clear the transient proposal state fields, advance the sign guard
past any previously signed/stalled round, and only then proceed with resetting
the engine to ensure no new sign attempts occur at the same (height, round,
step) as previous signatures.
Source: Coding guidelines
|
|
||
| #[test] | ||
| fn phase_elapsed_small_after_construct_and_new_height() { | ||
| // phase_elapsed feeds the finalize-stall recovery in main.rs: it reports | ||
| // how long the engine has sat in the current phase. Construction and | ||
| // new_height both re-arm phase_start, so right after either it must be | ||
| // tiny. No sleep, so the test stays deterministic. | ||
| let mut engine = BftEngine::new(100, "0xval".into(), 1000); | ||
| assert!(engine.phase_elapsed() < std::time::Duration::from_secs(1)); | ||
| engine.new_height(101, 1000); | ||
| assert!(engine.phase_elapsed() < std::time::Duration::from_secs(1)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Minor: test name should follow codebase convention.
Most test functions in this file use the test_ prefix (e.g., test_new_engine, test_advance_round, test_new_height). Consider renaming to test_phase_elapsed_small_after_construct_and_new_height for consistency.
The test logic is sound: deterministic, well-commented, and the 1-second threshold provides an appropriate safety margin for the sanity check.
♻️ Proposed renaming for consistency
- fn phase_elapsed_small_after_construct_and_new_height() {
+ fn test_phase_elapsed_small_after_construct_and_new_height() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sentrix-bft/src/engine.rs` around lines 3507 - 3518, The test function
phase_elapsed_small_after_construct_and_new_height does not follow the
established naming convention used throughout the codebase. Rename the function
to test_phase_elapsed_small_after_construct_and_new_height to add the test_
prefix that is consistently used for all other test functions in this file like
test_new_engine and test_advance_round. This ensures consistency with the
codebase naming standards.
95ba9c6 to
3487641
Compare
1 participant
Problem
When the BFT engine reaches
FinalizeBlockfor height N, the driver normally applies block N, which advancescurrent_heightto N and letsneed_new_round(bft.height() <= current_height) start the next round. But theFinalizeBlockarms are guard-heavy — split-brain, hash-mismatch, andadd_blockerrors canbreakout without applying block N. The engine is then parked at height N,phase=Finalize, while the chain stays at N-1:need_new_roundcan't reset it (bft.height()(N) > current_height(N-1))stepreturns a zero-duration →BftAction::Wait)So the engine spins on
phase timeout finalizeindefinitely, only un-sticking when a peer's block N arrives via gossip and advances the chain. When the gossip mesh is degraded that wait is minutes — the burst-then-stall cadence.Fix
At the
need_new_roundsite, detect the stuck state — engineheight > current_heightandphase == Finalizeand stuck longer thanFINALIZE_STALL_RESET(5s) — and callnew_height(next_height, …)to re-run consensus for the unapplied height.Adds
BftEngine::phase_elapsed()(the engine already tracksphase_start) for the detection.Safety
bc.height() >= heightskip-guard in the FinalizeBlock arms no-ops a re-finalize if the block already landed via gossip.Finalize+ ahead-of-chain gate only matches a genuine stall (normal finalize completes in ms).Not done / caveats
Summary by CodeRabbit
Bug Fixes
Tests