refactor(ethexe/db): move announces from BlockMeta to AnnounceStorage

[playX18]

Resolves #4945

@gear-tech/dev

[semanticdiff-com]

Review changes with  

Changed Files

File	Status
ethexe/consensus/src/validator/batch/manager.rs	83% smaller
ethexe/compute/src/tests.rs	73% smaller
ethexe/consensus/src/announces.rs	70% smaller
ethexe/service/src/tests/mod.rs	56% smaller
ethexe/network/src/db_sync/responses.rs	54% smaller
ethexe/rpc/src/utils.rs	49% smaller
ethexe/network/src/db_sync/mod.rs	37% smaller
ethexe/db/src/verifier.rs	35% smaller
ethexe/consensus/src/validator/initial.rs	30% smaller
ethexe/common/src/mock.rs	23% smaller
ethexe/db/src/migrations/v0.rs	18% smaller
ethexe/db/src/migrations/v2.rs	10% smaller
ethexe/common/src/db.rs	3% smaller
ethexe/db/src/database.rs	2% smaller
ethexe/rpc/src/apis/mod.rs	1% smaller
ethexe/common/src/utils.rs	1% smaller
ethexe/db/src/iterator.rs	0% smaller
ethexe/db/src/migrations/mod.rs	Unsupported file format
ethexe/db/src/migrations/v1.rs	0% smaller
ethexe/db/src/migrations/v3.rs	0% smaller

[gemini-code-assist]

Summary of Changes

Hello @playX18, 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 refactors the storage mechanism for 'announces' data by moving it out of the BlockMeta structure and into a dedicated storage solution. This change addresses technical debt by enhancing modularity and clarifying data ownership within the database layer. The 'announces' are now managed through specific read and write interfaces on the AnnounceStorage trait, which improves the overall organization and maintainability of the codebase.

Highlights

• Data Relocation: The 'announces' field, previously part of the BlockMeta struct, has been removed to improve data organization.
• Dedicated Announce Storage: New methods (block_announces, set_block_announces, mutate_block_announces) have been added to the AnnounceStorageRO and AnnounceStorageRW traits, providing a dedicated interface for managing block-specific announces.
• New Database Key: A new database key, Key::BlockAnnounces, has been introduced in ethexe/db/src/database.rs to store block announces independently, separate from BlockMeta.
• Codebase Update: All existing references and operations on BlockMeta.announces across the codebase have been updated to utilize the new AnnounceStorage interfaces, ensuring consistency with the refactored data structure.

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.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

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 refactors the database schema to move the announces field from BlockMeta into its own AnnounceStorage. The changes are extensive, touching many parts of the codebase that interact with block announces. The refactoring is well-executed, with the new database access patterns correctly implemented in most places. I've identified a critical issue regarding error handling in the database layer that could lead to a panic, and a logic change in a test file that should be addressed for correctness. Overall, this is a solid refactoring that improves the database design.

[grishasobol]

Requires a migration

[grishasobol]

@claude opus review

[claude]

Claude finished @grishasobol's task in 3m 55s —— View job

---

Claude's PR Review

Summary

This PR moves the announces field out of BlockMeta into a dedicated BlockAnnounces key (discriminant 13), introducing a new v3 migration for existing databases. The refactor is mechanical and well‑executed: 20 files updated, a new Key::BlockAnnounces variant added, four new trait methods (block_announces, set_block_announces, mutate_block_announces) plumbed through RawDatabase and Database, and all call sites (consensus, compute, network, rpc, verifier, iterator, tests) migrated from meta.announces to the new API.

Overall assessment: LGTM in design; one correctness concern about the migration worth addressing before merge, plus a small test-hygiene nit.

---

severity: high — v3 migration drops Some(empty_set) and replaces it with None, changing semantics

File: ethexe/db/src/migrations/v3.rs lines 44–48

if let Some(announces) = &old_data.meta.announces
    && !announces.is_empty()
{
    db.set_block_announces(block_hash, announces.clone());
}

In the old model, meta.announces had three distinct states:

• None → announces have not been propagated yet
• Some(empty_set) → propagation ran but produced no base announces
• Some(non_empty_set) → propagated with these announces

The migration intentionally skips Some(empty_set), which collapses the first two states into None in the new world. This matters because propagate_announces in ethexe/consensus/src/announces.rs treats the None case as "not propagated yet":

• line 147 (collect_unannounced_blocks): if self.block_announces(current_block).is_some() { break; } — after migration, an ex‑Some(empty_set) block no longer terminates the backward walk, so propagation could restart further back than intended.
• line 226 debug_assert!(db.block_announces(block.hash).is_none(), "Block {} should not have announces propagated yet", ...) — benign in release but will fire in debug on rerun.
• lines 248–256: db.block_announces(parent_hash).ok_or_else(|| anyhow!("Parent block({}) announces are missing"))? — if the migration turns a parent's empty set into None, propagation of the child now errors out where it previously succeeded.

Note the new set_block_announces callsite in announces.rs at line 282 still writes new_base_announces even when empty, so new writes can legitimately store Some(empty_set). Only the migration drops that state.

Suggested fix — preserve Some(empty_set) by removing the emptiness guard:

if let Some(announces) = &old_data.meta.announces {
    db.set_block_announces(block_hash, announces.clone());
}

If the intent was a space optimization, please add a comment explaining why the None/Some(empty_set) collapse is safe for the propagation paths cited above. Otherwise this is a real migration regression for already-persisted databases.

Fix this →

severity: low — Gemini's .expect() concern on block_announces is consistent with the rest of the file

File: ethexe/db/src/database.rs line 430

BTreeSet::<HashOf<Announce>>::decode(&mut data.as_slice())
    .expect("Failed to decode data into `BTreeSet<HashOf<Announce>>`")

Gemini flagged this as panic-prone. Worth noting that this file uses the same .expect("Failed to decode data into ...") idiom in 28 places (including announce_meta, announce_schedule, announce_outcome, block_small_data). Softening only this one call would be inconsistent. If the team wants robust-to-corruption reads it should be a follow-up that converts them all to Result. No change requested here.

severity: low — Gemini's mock.rs concern does not apply to the current code

File: ethexe/common/src/mock.rs lines 691–697

The concern in Gemini's review was about mutate_block_announces vs set_block_announces, but the current code already uses set_block_announces:

if let Some(announces) = announces {
    db.set_block_announces(hash, announces);
}

So the "insert-into-existing" vs "set" semantics Gemini warned about don't apply. No action needed — grishasobol already explicitly said this edge case isn't worth handling for test setup.

severity: low — odd empty mutate_block_meta call in a network test

File: ethexe/network/src/db_sync/mod.rs around lines 1359

right_db.mutate_block_meta(H256::zero(), |_meta| {});

Calling mutate_block_meta with an empty closure purely to materialize a default BlockMeta entry for H256::zero() is surprising. If this is needed to satisfy the sync protocol (block must have a BlockMeta entry in addition to BlockAnnounces), a short comment would help future readers; otherwise the line could be dropped.

Things that look correct and are worth calling out

• New Key::BlockAnnounces(H256) = 13 slots in without collision; prefix handling in Key::to_bytes is updated. Discriminant 13 is unused, no conflict.
• v3 migration correctly uses iter_prefix on the BlockSmallData prefix (H256::from_low_u64_be(0)) and filters by key length 2 * size_of::<H256>() — only BlockSmallData entries are touched, no collision with other key variants.
• Re-encoding the BlockSmallData without the announces field (BlockMeta no longer has it) is handled correctly in-place via db.kv.put(&k, new_data.encode()).
• Migration chain check assert!(crate::VERSION == super::v3::VERSION, ...) in v1.rs and v2.rs is updated consistently; the compile-time type-hash in v2.rs is regenerated.
• ensure_types_unchanged in ethexe/common/src/db.rs updates EXPECTED_TYPE_INFO_HASH to reflect the new BlockMeta layout — good.
• propagate_announces debug_assert and error paths preserve old invariants for newly‑written data (see migration note above for existing data).
• take_block_announces (flagged by grishasobol on an earlier revision) is no longer present.

---

Feel free to ping me again after addressing the migration point.

• Read style guide and CLAUDE.md
• Analyze PR diff vs origin/master
• Review key files (db.rs, database.rs, migrations/v3.rs, announces.rs, mock.rs)
• Check previous reviewer feedback resolution
• Post review feedback
  • Branch: ap-move-anounces

[playX18]

@codex fix

[chatgpt-codex-connector]

To use Codex here, create an environment for this repo.

[playX18]

@codex fix

[chatgpt-codex-connector]

Summary

• Fixed the v2 -> v3 migration to preserve Some(empty_set) announces by removing the && !announces.is_empty() guard, so any existing Some(...) announce state is migrated into BlockAnnounces consistently. ethexe/db/src/migrations/v3.rsL43-L46
• Committed the change on branch work with commit a9b52ba (fix(ethexe-db): preserve empty announces in v3 migration) and created a follow-up PR via the make_pr tool.

Testing

• ❌ rustfmt --check ethexe/db/src/migrations/v3.rs (failed initially because edition was not specified; rustfmt defaulted incompatibly for this file’s async syntax).
• ✅ rustfmt --edition 2024 --check ethexe/db/src/migrations/v3.rs
• ⚠️ cargo test -p ethexe-db migrations::v3::tests::ensure_migration_types -- --exact (started successfully but was interrupted before completion due long compile/runtime in this session).

View task →

[grishasobol]

work is here #5244