fix(vara.eth): batch-commitment refactor; correct validators rotation in malachite - #5598
fix(vara.eth): batch-commitment refactor; correct validators rotation in malachite#5598grishasobol wants to merge 16 commits into
Conversation
a33cfb3 to
c0ac26d
Compare
Make the consensus-fix branch compile, pass clippy and tests: - thread MbMeta.last_advanced_eb (now Option<H256>) and the new `finalized` flag through compute, externalities, init, prometheus - mark MBs finalized in process_mb_finalized; seed genesis MB - fix create_batch_commitment dropping the chain commitment - fix producer try_include_chain_commitment (wrong var / missing anchor) - fix is_strict_descendant_eth_block genesis-anchor equality - drop the `disable-tests` gate; update/prune unit + integration tests Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0165AUhSM5rfJgD7hMdyhs8X
…t_advanced_eb> `ensure_types_unchanged` guards DB SCALE type layouts; the MbMeta change (new `finalized` field, `last_advanced_eb` -> Option<H256>) alters it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0165AUhSM5rfJgD7hMdyhs8X
…ration - compute: treat a parent MB with no last_advanced_eb as an error (ParentMbLastAdvancedEbMissing) instead of defaulting to zero - externalities: return errors instead of panicking on a missing last_advanced_eb in process_mb_proposal / build / validate paths - db: add v1 -> v2 migration that re-encodes MbMeta (Option<last_advanced_eb>, new finalized flag backfilled from latest_finalized_mb_hash) and bumps version - consensus: unit-test is_strict_descendant_eth_block; drop stray `let _ = head` - compute test fixture seeds last_advanced_eb like process_mb_proposal Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0165AUhSM5rfJgD7hMdyhs8X
…cache flag validate_chain_commitment rejected a head MB unless its per-MB `MbMeta.finalized` cache bit was set. That bit is only written by `process_mb_finalized`, so a freshly-started validator that learns the prior chain's finality indirectly (sync / on-chain MBCommitted) has it unset on MBs that are in fact finalized — making it reject every batch referencing them. During a validator-set handover the new set can then never reach commitment threshold and consensus stalls (`tests::validators_election` timeout). Restore the BFT-safe reachability check (`is_finalized_locally`, reachable from `latest_finalized_mb_hash`) as the source of truth, keeping the `finalized` flag as a fast path. Found via codex investigation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0165AUhSM5rfJgD7hMdyhs8X
…tted anchor
A freshly-joined validator (e.g. right after a validator-set handover)
learns the on-chain `last_committed_mb` only from the `MBCommitted` event
(propagated into `BlockMeta.last_committed_mb`) and never computed that MB,
so it has no local `mb_compact_block` for it. The batch-commitment refactor
required that compact block up front, so such a validator skipped chain
commitment forever ("last committed MB is still not synced locally").
Both the producer (`try_include_chain_commitment`) and participant
(`validate_chain_commitment`) paths now walk the parent chain from the
finalized tip / head and terminate on the anchor *by hash*, never
dereferencing the anchor's own compact block:
- producer: running off the local chain before reaching the anchor is a
lenient skip (retried on a later block), not a hard error.
- participant: it becomes a `HeadMbNotStrictDescendantOfLatestCommittedMb`
rejection instead of a hard error, and the committed EB anchor is read
from `BlockMeta.last_committed_eb` (emitted alongside `MBCommitted` from
the same `ChainCommitment` in Router.sol) rather than from the committed
MB's `mb_meta`, which a fresh validator also lacks.
Adds regression tests covering an absent committed-anchor compact block on
both the producer and participant paths.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0147tTHt6e7mhirSAPwNxRc3
`validators_election` hung for the whole 60s test budget after the new validator set took over. Two stacked defects in the chain-commitment path: 1. Infinite loop / runtime wedge. The DB seeds a sentinel "zero MB" with a self-parent (`db init`), so `mb_compact_block(H256::zero())` returns `Some`, not `None`. When the new set's finalized chain did not descend from the on-chain `last_committed_mb`, the producer/participant parent walks reached the zero MB and spun forever on its self-parent, freezing the single-threaded runtime. Stop both walks at the zero sentinel (the pre-refactor `collect_*_predecessors` had this guard). 2. Empty chain commitment riding along. `create_batch_commitment` emitted a chain commitment with no transitions whenever the batch carried other commitments (e.g. code). That fires `MBCommitted`, pinning `last_committed_mb` to an MB which a freshly re-synced validator set (after a handover) does not have on its locally rebuilt chain — so it can never build a descendant chain commitment and stalls. Drop empty chain commitments unless a genuine checkpoint threshold is reached; other commitments still go through. This matches the master baseline, which never emits empty chain commitments. Adds a regression test that builds a finalized chain disconnected from the committed anchor and asserts the producer skips promptly instead of hanging. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0147tTHt6e7mhirSAPwNxRc3
Re-implements the parent-advance (AdvanceTillEthereumBlock) rule on top of - db init seeds the genesis era's on-chain validator set (a synced block must always have its era's validators in the db). - observer backfills db.validators(era) for every era spanned by a freshly synced chain, so a node joining mid-chain holds historical eras' sets. - new Externalities::validators_for_child_of resolves the era a parent MB advanced into; core app validator_set_for_height verifies each height's certificate against that era's set (falling back to the live shared set). - the on-chain address -> pub key pool is threaded into EthexeExternalities. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0147tTHt6e7mhirSAPwNxRc3
c0ac26d to
e82394f
Compare
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request finalizes the consensus batch-commitment refactor, ensuring robust propagation of metadata and finalized status across the system. It introduces necessary database schema updates, fixes critical logic in batch commitment creation, and enhances validator set resolution to support historical era validation. These changes improve system reliability and prepare the codebase for future cross-era membership handling. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a database migration (v1 to v2) where MbMeta gains a finalized flag and its last_advanced_eb field is changed to an Option<H256>. It refactors the batch commitment manager and filler to support appending multiple chain commitments, resolving potential validator-set handover stalls and sync issues. Additionally, it updates the consensus and sync layers to ensure era-correct validator sets are used for historical certificate verification. The review feedback highlights two critical issues: a compilation error in filler.rs due to calling checked_add directly on a NonZero type, and a potential panic in compute.rs where .expect() is used instead of gracefully returning the pre-defined ParentMbLastAdvancedEbMissing error.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…ra block is allowed when advancing to the new era
…sing
The variant was introduced for an `ok_or(...)?` path that was later
replaced by an `expect("invariant")`, since a missing `last_advanced_eb`
on a computed parent is a DB-invariant violation (process_mb_proposal
always sets it). With the only call site gone the variant is dead code.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0147tTHt6e7mhirSAPwNxRc3
The frozen v1 on-disk `MbMeta` layout describes the v1 database version, so it belongs in `migrations/v1.rs`. Moved it there as `v1::MbMeta` (dropping the now-redundant `V1` suffix) and reference it from the v1 -> v2 migration. No behavior change; the v2 layout stays in v2.rs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0147tTHt6e7mhirSAPwNxRc3
Finalizes the ethexe consensus batch-commitment refactor and fixes validator rotation across eras in the malachite layer. Compiles, clippy-clean, ethexe unit/integration tests pass.
1. Batch commitment
A batch is what validators agree on and submit to the Router. It bundles a few optional parts plus the chain progress:
Normal (non-empty) chain commitments already worked. This PR fixes the edge cases where building the chain commitment could stall or loop:
is_strict_descendant_eth_block— genesis-anchor equality fix.2. Validator rotation across eras
Time is split into eras, and each era has its own on-chain validator set. A batch is verified against the set of the era its block falls into. A node must therefore hold the validator set for every era it touches, not just the current one.
flowchart LR subgraph "era N" VN["validators(N)"] end subgraph "era N+1" VN1["validators(N+1)"] end subgraph "era N+2" VN2["validators(N+2)"] end VN --> VN1 --> VN2Fixed here:
3.
MbMeta+ DB migration (v1 → v2)MbMetagained afinalizedflag andlast_advanced_ebbecame optional:A v1 → v2 migration re-encodes every record:
last_advanced_ebis wrapped inSome(..), andfinalizedis backfilledtruefor every MB reachable from the latest finalized MB.Deliberately out of scope
Full cross-era membership — where a node that joined after several rotations can resolve and observe any historical era from a complete pubkey table (vote/propose only in elected eras, observe otherwise) — is a consensus feature, not a bug fix, and is left as a follow-up. The current resolver uses the node's own per-era set with a live-set fallback, which is enough because a node only runs consensus for eras it is elected to.
🤖 Generated with Claude Code