feat(ethexe-consensus): in case of equal outcome prefer base announces

[grishasobol]

No description provided.

[semanticdiff-com]

Review changes with  

Changed Files

File	Status
ethexe/consensus/src/validator/batch/manager.rs	50% smaller
ethexe/rpc/src/apis/mod.rs	44% smaller
ethexe/consensus/src/validator/producer.rs	23% smaller
ethexe/consensus/src/validator/subordinate.rs	22% smaller
ethexe/service/src/tests/mod.rs	12% smaller
ethexe/consensus/src/connect/mod.rs	8% smaller
ethexe/consensus/src/announces.rs	4% smaller
CLAUDE.md	Unsupported file format
ethexe/common/src/hash.rs	0% smaller
ethexe/common/src/primitives.rs	0% smaller
ethexe/consensus/src/lib.rs	0% smaller
ethexe/consensus/src/validator/mod.rs	0% smaller
ethexe/service/src/lib.rs	Unsupported file format
ethexe/service/src/tests/utils/events.rs	0% smaller

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! 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 refines the consensus mechanism in the ethexe layer by prioritizing base announces when outcomes are identical, which optimizes batch commitment processes. Additionally, it streamlines the network layer by replacing custom connection limit logic with standard libp2p capabilities and updates testing utilities to support these changes.

Highlights

• Consensus Logic Improvement: Implemented a preference for base announces when outcomes are equal, ensuring more efficient batch commitment handling.
• Network Connection Management: Removed custom connection limit logic in favor of improved built-in libp2p connection management, simplifying the network stack.
• Testing Infrastructure: Updated test suites to accommodate the new consensus logic, including new test cases for batch commitment expiry and announce selection.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a 'base-announce-priority' mechanism to the ethexe consensus layer, ensuring that base announces are preferred when outcomes are identical to non-base siblings. This change aims to maintain consistent batch commitment expiry even during idle periods. The implementation includes updates to the producer and subordinate state machines to handle concurrent computation of base and non-base announces, along with new utility functions in the announces module. While the logic correctly addresses the described goal, several critical issues were identified in the review: a performance-degrading debug block was left in the validator context, a change to the network connection manager bypasses the backoff mechanism for outbound connections, and there are potential integer underflow risks in the consensus logic. Additionally, some redundant database lookups and complex logic could be simplified for better maintainability.

[grishasobol]

@claude opus review

[claude]

Claude finished @grishasobol's task in 1m 28s —— View job

---

Code Review: feat(ethexe-consensus): base announce priority

• Read PR diff and changed files
• Review core logic changes (announces.rs, producer.rs, subordinate.rs)
• Review supporting changes (common, connect, batch manager, RPC, service)
• Review tests
• Post review findings

---

Summary

This PR adds a preference for base announces when they have the same outcome as non-base siblings. The goal is to maintain a chain of base announces during idle blocks (no user messages), ensuring batch commitment expiry equals commitment_delay_limit rather than degrading to 1. The implementation touches three consensus participants (Producer, Subordinate, ConnectService) to eagerly compute base announces alongside producer announces, then prefer the base when outcomes match.

The overall design is sound and well-tested. A few issues below, mostly around edge cases in the new logic.

---

Findings

severity: medium — Duplicated base-preference logic in best_announce and block_best_announce

File: ethexe/consensus/src/announces.rs lines 659-688 and 736-757

block_best_announce manually finds base/not-base announces and applies the preference. But best_announce (called internally by best_parent_announce, which is called by block_best_announce) also does the same base-preference check at lines 740-757. This means for the parent selection, the base preference is applied, and then block_best_announce applies it again at the current block level. While this is correct (the two checks are at different block levels), having the same conceptual logic in two places with slightly different APIs is fragile. The best_announce function both selects the best by points AND applies base-preference, mixing two responsibilities. Consider documenting clearly that best_announce always returns a base-preferred result, so callers of block_best_announce understand both levels are covered.

---

severity: medium — find_block_announce returns only the first match

File: ethexe/consensus/src/announces.rs lines 218-243

find_block_announce returns the first announce matching the predicate. In block_best_announce (line 667), it searches for a not-base announce with parent == best_parent. If there are multiple not-base announces with the same parent, only the first is considered. This is likely fine in practice (usually one producer per block), but the function name and signature don't communicate this "first match" behavior. Consider renaming or documenting this.

---

severity: medium — Producer does not emit BlockComputationComplete in the WaitForBaseComputed → WaitForTimer → produce_announce path

File: ethexe/consensus/src/validator/producer.rs lines 91-101 and 183-194

When the Producer is in WaitForBaseComputed and receives the base computation result, it transitions to WaitForTimer. After the timer fires, it calls produce_announce, which creates and computes the producer's own announce. After that computation completes (WaitingAnnounceComputed branch, line 103), BlockComputationComplete is emitted. So the event is correctly emitted — but only after a second computation round. This two-step path is correct but worth documenting more explicitly in the state machine comments, as it's not obvious from reading the state enum alone.

---

severity: medium — Subordinate doesn't apply base-preference when choosing announce for commitment

File: ethexe/consensus/src/validator/subordinate.rs lines 76-113

The Subordinate computes both the base and non-base announces, emits BlockComputationComplete, then transitions to Participant. But unlike the Producer (which checks announces_have_equal_outcomes in process_computed_announce), the Subordinate doesn't select between the two computed announces — it just waits for both to finish and moves on. The actual base-preference selection happens later in BatchCommitmentManager::create_batch_commitment via block_best_announce. This is correct but the asymmetry with Producer is worth noting. The Producer explicitly chooses the best announce before creating the batch commitment, while the Subordinate relies on the batch manager to make that choice.

---

severity: low — Large block of commented-out test code

File: ethexe/service/src/tests/mod.rs lines ~3450-3525

The catch_up_test_case function has a large block of commented-out code with a TODO: "after changes in best announce selection, Bob is able to catch up Alice. We should create more complex test case to cover the scenario when Bob is not able to catch up Alice." This is a significant amount of dead test code. Consider removing it and tracking the TODO in an issue instead, or implementing the replacement test in this PR.

---

severity: low — invalid_code test timeout doubled without explanation

File: ethexe/service/src/tests/mod.rs line 74

Timeout changed from 30s to 60s. Also the test now uses wait_for_code_validation instead of the previous wait_for() pattern. This seems like a separate behavioral change bundled into this PR without explanation.

---

severity: low — #[allow(unused)] on find_announce_rejected

File: ethexe/service/src/tests/utils/events.rs line 299

This #[allow(unused)] suggests the function lost its callers as part of this PR (the catch_up_test_case commenting out). If the test code is being reworked, consider removing the function entirely or keeping it without the allow if it will be used by the replacement test.

---

Overall Assessment

The core design — eagerly computing base announces and preferring them when outcomes match — is well-reasoned and addresses a real issue with batch commitment expiry degradation during idle periods. The new batch_commitment_expiry_after_idle_blocks integration test directly validates the key invariant. The unit tests in announces.rs cover the three important cases (same outcome → prefer base, different outcome → keep non-base, not computed → keep non-base).

The main concerns are about code clarity rather than correctness: the duplicated base-preference logic across best_announce and block_best_announce, and the large commented-out test block. The functional logic appears correct.

---

[ecol-master]

can you please replace it to data: T, because value sounds like transaction value or something that have digital type.

Also in announces.rs file it would be easy to read:

Current announce.value from first view is not clearly describe the logic

let not_base_announce_hash = db.find_block_announce(block_hash, |announce| {
    announce.value.parent == best_parent && !announce.value.is_base()
})?;

[ecol-master]

What is it?

[grishasobol]

this is old test case, which is not correct now but and must be fixed by todo above

[ecol-master]

I think here must be ancestor_depth_limit.

Current name is misleading

[grishasobol]

@claude fix

[claude]

Claude finished @grishasobol's task in 2m 43s —— View job

---

Fixed review feedback

• Analyze review comments to identify fixes needed
• Rename WithHashOf.value → WithHashOf.data (ethexe/common/src/hash.rs:223)
• Update all usages of .value on WithHashOf instances in announces.rs, producer.rs, subordinate.rs
• Rename limit → ancestor_depth_limit parameter in best_announce (ethexe/consensus/src/announces.rs:695)
• Commit and push changes

Changes made

ethexe/common/src/hash.rs — renamed pub value: T to pub data: T in WithHashOf<T> struct

ethexe/consensus/src/announces.rs:

• Updated WithHashOf { value: announce } construction → { data: announce }
• Updated all announce.value.{is_base,parent} → announce.data.{is_base,parent} (3 call sites)
• Renamed limit: u32 → ancestor_depth_limit: u32 in best_announce signature and body

ethexe/consensus/src/validator/producer.rs — updated 3 .value accesses on WithHashOf<Announce> → .data

ethexe/consensus/src/validator/subordinate.rs — updated 2 .value accesses on WithHashOf<Announce> → .data
|