refactor(ethexe-consensus): fix some bad places in batch aggregation logic#5366
refactor(ethexe-consensus): fix some bad places in batch aggregation logic#5366grishasobol wants to merge 10 commits intomasterfrom
Conversation
|
Review changes with Changed Files
|
Mirror the shape of aggregate_chain_commitment — caller hands in the batch filler and the helper takes care of fetching the codes queue, wrapping each validated code as a commitment, and stopping when the filler rejects further entries. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…struct The BatchFiller trait and BatchFillerNoLimits unlimited impl were introduced speculatively — in practice every caller wants the size budget, so drop the abstraction and keep only the concrete struct, renamed from BatchFillerLimiter back to BatchFiller. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- aggregate_code_commitments_for_block: happy path skips pending codes; missing codes queue surfaces as an error. - BatchFiller: size budget rejects commitments once exhausted; include_chain_commitment merges transitions and advances head. - validate_chain_commitment: HeadAnnounceNotComputed rejection path (the branch that fired when a peer races an announce ahead of local execution). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both aggregate_chain_commitment and validate_chain_commitment walked parent pointers into a chronologically-ordered VecDeque. Fold that into one utility so the two sides can't drift apart on traversal semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
grishasobol
changed the title
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 refactors the batch aggregation logic within the ethexe consensus module. The primary goal is to optimize batch creation by avoiding unnecessary on-chain commitments for empty batches while ensuring that chain commitments are still submitted when the chain deepness threshold is exceeded. It introduces a centralized configuration structure, improves the validation of chain and code commitments, and adds regression tests to verify the new skip-commit behavior. 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 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 counter productive. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the batch commitment logic within ethexe-consensus, simplifying the BatchFiller and centralizing commitment aggregation and validation. Key changes include renaming BatchLimits to BatchManagerConfig, introducing logic to skip batch creation for short empty chains to reduce on-chain noise, and improving error reporting using anyhow contexts. Feedback suggests enhancing robustness by replacing expect calls with proper error handling in utility functions and adding iteration limits to prevent potential infinite loops when traversing the announce chain.
| debug_assert_eq!( | ||
| db.announce(head_announce_hash) | ||
| .expect("Announce not found") | ||
| .block_hash, | ||
| at_block, | ||
| "Head announce is not from `at_block`" | ||
| ); |
There was a problem hiding this comment.
Avoid using expect for error handling. If an operation can fail, prefer returning a Result and handle the error at the call site. While this is used within a debug assertion, using expect in a library-like utility can be risky if the state is not truly unreachable.
References
- Avoid using expect for error handling. If an operation can fail, prefer returning a Result and handle the error at the call site.
| pub fn collect_pending_announces<DB: AnnounceStorageRO>( | ||
| db: &DB, | ||
| head_announce_hash: HashOf<Announce>, | ||
| last_committed_announce_hash: HashOf<Announce>, | ||
| announce_hash: HashOf<Announce>, | ||
| ) -> Result<Vec<HashOf<Announce>>> { | ||
| let mut announces = Vec::new(); | ||
| let mut current_announce = announce_hash; | ||
|
|
||
| // Maybe remove this loop to prevent infinite searching | ||
| while current_announce != last_committed_announce_hash { | ||
| if !db.announce_meta(current_announce).computed { | ||
| // All announces till last committed must be computed. | ||
| // Even fast-sync guarantees that. | ||
| bail!("Not computed announce in chain {current_announce:?}") | ||
| } | ||
|
|
||
| announces.push(current_announce); | ||
| current_announce = db | ||
| .announce(current_announce) | ||
| .ok_or_else(|| anyhow!("Computed announce {current_announce:?} body not found in db"))? | ||
| ) -> Result<VecDeque<HashOf<Announce>>> { | ||
| let mut announces = VecDeque::new(); | ||
| let mut current = head_announce_hash; | ||
| while current != last_committed_announce_hash { | ||
| announces.push_front(current); | ||
| current = db | ||
| .announce(current) | ||
| .with_context(|| format!("Announce {current:?} not found in db"))? | ||
| .parent; | ||
| } | ||
|
|
||
| Ok(announces.into_iter().rev().collect()) | ||
| Ok(announces) | ||
| } |
There was a problem hiding this comment.
The loop while current != last_committed_announce_hash in collect_pending_announces could theoretically be infinite if the chain is broken or if last_committed_announce_hash is not an ancestor of head_announce_hash. Although the blockchain structure (hashes) should prevent cycles, adding a safety limit on the number of iterations (e.g., based on a maximum expected chain depth) would make this more robust against database corruption or unexpected states.
grishasobol
added
A3-gotissues
1 participant
No description provided.