fix(drive-abci): execute a different block at the same height/round instead of serving the stale context - #4462
Conversation
…nstead of serving the stale context process_proposal keeps its block execution context keyed by height and round only. When a second, different block arrived for the same height and round it either answered with the cached result of the block it held (proposer path: the app hash of our own proposal was returned for the network's block) or refused with an ABCI error. Tenderdash turns the first into a mustValidate panic and the second into a mustEnsureProcess panic, and its WAL replay recreates the same situation on every restart. This is exactly what happens to a validator that hands over from block sync to consensus while still behind: it proposes at a historical height, and the block the network really committed at that height and round then arrives through consensus catch-up (dashpay/tenderdash#1413). Key the cache by block hash: the same block is still served from the proposer cache / re-run as before; a different block at the same height and round drops the stale context and is executed. Two strategy tests pin both behaviours. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
✅ Final review complete — no blockers (commit a5cab13) |
|
Warning Review limit reachedNext included review available in 54 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe proposal handler now compares block hashes before using cached proposer results. Matching hashes reuse cached results. Different hashes discard the existing execution context and execute the requested block. New strategy tests cover both collision cases. ChangesProposal collision handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change makes process-proposal handling execute a different block received at the same height and round instead of reusing stale results or returning an exception; targeted tests and broader checks are reported passing, so no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Tenderdash
participant process_proposal
participant BlockExecutionContext
Tenderdash->>process_proposal: Submit proposal at height and round
process_proposal->>BlockExecutionContext: Compare incoming hash with cached hash
alt Hash matches
BlockExecutionContext-->>process_proposal: Return cached proposer results
process_proposal-->>Tenderdash: Accept with cached result
else Hash differs
process_proposal->>BlockExecutionContext: Drop stale execution context
process_proposal->>BlockExecutionContext: Execute requested proposal
BlockExecutionContext-->>process_proposal: Return independent app hash
process_proposal-->>Tenderdash: Accept with new result
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4462 +/- ##
============================================
- Coverage 87.35% 87.01% -0.34%
============================================
Files 2698 2735 +37
Lines 344653 349535 +4882
============================================
+ Hits 301074 304163 +3089
- Misses 43579 45372 +1793
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The new hash comparison correctly handles collisions after a block hash has been recorded, but the immediately post-PrepareProposal cache remains unbound and can still return a stale app hash for a different block at the same height and round. The regression test establishes the local block hash before submitting the competing block, so it does not cover this remaining path.
Source: reviewer evidence from codex-general, codex-security-auditor, and codex-rust-quality (exact backend model IDs were not included in the supplied projection); final verifier is Anthropic Claude Agent SDK (exact model ID not exposed); orchestration-only and not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/abci/handler/process_proposal.rs`:
- [BLOCKING] packages/rs-drive-abci/src/abci/handler/process_proposal.rs:109-149: Hashless proposer cache can still answer a different block
`PrepareProposal` stores `proposer_results` while leaving `block_hash` as `None`. The first `ProcessProposal` request at that round is therefore accepted from this cache whenever only the retained transaction count and Core chain-lock update match. The code does not compare the transaction bytes or other execution inputs such as block time, proposer, Core height, quorum, and app version. A competing empty block can consequently receive the prepared block's app hash, after which its own hash is attached to the stale context. If the two blocks execute differently, Tenderdash receives an app hash that does not belong to the requested block and can enter the same persistent failure this PR is intended to prevent. The new test first processes the local proposal through `mimic_execute_block`, which populates `block_hash`, so it bypasses this branch. Before reusing a hashless proposer result, compare the complete execution-relevant proposal identity, including exact transaction bytes; otherwise discard the context and execute the incoming request.
In `packages/rs-drive-abci/tests/strategy_tests/test_cases/process_proposal_collision_tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/tests/strategy_tests/test_cases/process_proposal_collision_tests.rs:96-98: Convert milliseconds to nanoseconds with the correct scale
`Timestamp.nanos` is expressed in nanoseconds, but the millisecond remainder is multiplied by 1,000 instead of 1,000,000. The current test timestamps have no fractional-second remainder, so the existing assertions are unaffected, but a future fractional offset would silently lose most or all of that offset and could weaken timing-sensitive proposal tests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The hash-aware branch now correctly discards a stale execution context and executes a different block at the same height and round. However, the hashless context left immediately after PrepareProposal is still matched using only transaction count and the Core chain-lock update, so the first competing ProcessProposal can receive the prepared block's stale app hash; this leaves the PR's core failure mode unresolved.
Source: Codex general, security-auditor, and rust-quality reviewer backend: gpt-5.6-sol; final verifier backend: Anthropic Claude Agent SDK (exact model ID not exposed). Orchestration-only and not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/abci/handler/process_proposal.rs`:
- [BLOCKING] packages/rs-drive-abci/src/abci/handler/process_proposal.rs:109-149: Hashless proposer cache can still answer a different block
`prepare_proposal` executes the local candidate with `block_hash: None` and stores its response in `proposer_results`. On the first `ProcessProposal`, this branch reuses that response when only the retained transaction count and `core_chain_lock_update` match; it does not compare the accepted transaction bytes, block time, proposer, Core height, proposed app version, validator quorum, or validate the incoming consensus app version. These fields are consumed by `BlockProposal` and `run_block_proposal_v0` and can change execution or make the proposal invalid. A competing request with the same transaction count and chain-lock update can therefore receive the prepared candidate's app hash, after which lines 134-141 attach the competing request's hash to that stale context. This directly preserves the wrong-AppHash failure the PR is intended to eliminate. The regression test calls `mimic_execute_block` for the local proposal first, and that helper invokes `ProcessProposal` and records its hash before the competing request, so the test exercises only the corrected hash-present branch. Retain and compare a normalized identity covering every execution-relevant PrepareProposal/ProcessProposal input, including byte-for-byte accepted transactions, before serving this cache; otherwise discard the context and execute the incoming request.
…ntent The hash-aware branch discards a stale execution context and executes a different block at the same height and round, but the context left behind immediately after PrepareProposal has no block hash to compare against — Tenderdash computes one only once the response is in. That context was matched on transaction count and the core chain lock update alone, so the first competing ProcessProposal at the same height and round could still be served the prepared block's app hash. Height was never compared at all. Decide identity on the execution inputs instead: the ordered transaction bytes, block time, proposer, core chain locked height, core chain lock update, proposed app version, validator set quorum hash, consensus app version and height. Anything BlockProposal discards on conversion — proposed_last_commit, misbehavior, next_validators_hash and the consensus block version — cannot reach execution and is left out. A mismatch on any of them takes the same way out the hash-mismatch case already took: discard the context and execute the block we were asked about. The proposed app version and the quorum hash were not retained anywhere, so proposer_results now carries them alongside the response. It is written only by prepare_proposal and read only by process_proposal, so every other construction site keeps its `proposer_results: None` unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
They pin a consensus-halt regression, run in under a second combined, and their binary is already built by the PR phase. Excluded from the push-only chain-simulation phase to keep the codecov flags disjoint. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
At exact head a5cab13, the hashless proposer-cache path now compares every input retained and consumed by BlockProposal execution, including exact accepted transaction bytes, before reusing the prepared result; mismatches discard the stale context and execute the incoming proposal. The prior blocking finding is fixed, and validation passed with all 5 targeted collision tests, cargo fmt, and drive-abci clippy; no in-scope issues remain.
Source: Codex reviewers: gpt-5.6-sol; final verifier: Anthropic Claude Agent SDK (exact model ID not exposed). Orchestration-only and not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
2 participants
Issue being fixed or feature implemented
App-side half of dashpay/tenderdash#1413.
process_proposalkeeps its block execution context keyed by height and round only. When a second, different block arrives for the same height and round, it does one of two wrong things:block_hashset,proposer_resultscached): it returns the cached result for any request hash — i.e. the app hash of our own proposal is served for the network's block;Err(BadRequest("received a process proposal request twice with different hash")), which reaches Tenderdash as an ABCI exception.Tenderdash turns the first into a
mustValidatepanic (wrong Block.Header.AppHash …) and the second into amustEnsureProcesspanic, and its WAL replay re-creates the same situation on every restart, so the node never comes back without discarding the Tenderdash data.This is exactly what happens to a validator that hands over from block sync to consensus while still behind: it proposes at a historical height (it is in that era's quorum), and the block the network really committed at that height and round then reaches it through consensus catch-up — same height, same round, different block. Observed on a mainnet evonode re-syncing from scratch (stall at 24174, own proposal at
h=24175 r=1, the collision at 24176 where it was the genuine round-0 proposer).What was done?
process_proposal: key the cache by block hash. Same hash → unchanged (proposer cache served; non-proposer re-run with the existing warning). Different hash at the same height/round → log a warning, drop the stale context, and execute the block we were asked about. Never serve the other block's result, never error.test_cases/process_proposal_collision_tests.rs:process_proposal_for_a_different_block_must_not_be_served_from_the_proposer_cache— PrepareProposal + ProcessProposal of our own block at (H, 0), then ProcessProposal of a different block at (H, 0): the answer must be that block's app hash (checked against a clean execution of the same request). Before the fix:Acceptwith our own block's app hash.process_proposal_for_a_second_different_block_in_the_same_round_must_execute— two different blocks at (H, 0), neither ours: the second must execute. Before the fix: the ABCIBadRequestabove.With this change the collision is harmless: the genuine block executes and commits and the node keeps catching up. The Tenderdash side (not proposing while behind, not evicting the catch-up peer) is tracked in dashpay/tenderdash#1413 and dashpay/tenderdash#1414.
How Has This Been Tested?
cargo test -p drive-abci --test strategy_tests process_proposal_collision— both tests red on the base, green with the fix.cargo test -p drive-abci --test strategy_tests— 95 passed, 0 failed, 4 ignored (pre-existing).cargo test -p drive-abci --lib abci::handler— 52 passed.cargo clippy -p drive-abci --tests --features mocksclean;cargo fmt --all -- --checkclean.Breaking Changes
None. Valid flows (same block re-sent, next round, next height) are unchanged; only the "different block at the same height/round" case changes, from a wrong answer / error to execution.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests