feat!(ethexe): transition of value claims to merkle root#5415
feat!(ethexe): transition of value claims to merkle root#5415StackOverflowExcept1on wants to merge 138 commits into
Conversation
StackOverflowExcept1on
added
scope: vara.eth
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 transitions the value claim mechanism in the ethexe system from using arrays to Merkle roots. This change optimizes the data structure for value claims, allowing for more efficient verification via Merkle proofs. Please note that this update introduces a breaking change for existing program states on Ethereum, though it does not affect the underlying database state. Highlights
Ignored Files
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
|
![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 transitions the ethexe value claim mechanism from a push-based model to a pull-based model using Merkle proofs. It introduces a BinaryMerkleTree Solidity library, updates the Mirror contract to store Merkle roots of claims, and adds a claimValue function for users to claim funds individually. Feedback was provided to ensure that failed Ether transfers in claimValue revert the transaction to allow for retries and prevent permanent fund locking. Additionally, a comment in the BinaryMerkleTree library was flagged as misleading, noting that commutative hashing would break compatibility with the Rust-side root calculation.
StackOverflowExcept1on
changed the title
…oundation) Restores the foundational types from commit 4138374: - ReplyInfo::to_hash() in gear-core - PromiseEmissionMode in ethexe-common primitives - CompactPromise / SignedCompactPromise + helpers in ethexe-common - Mock impls for Promise - InjectedStorage{RO,RW} extended with promise/compact_promise getters and setters - Database/RawDatabase impls for the new storage methods This is the foundation that downstream pieces (processor BoundPromiseSink, compute PromisePolicy wiring, modular RPC injected API, gossipsub compact-promise topic) build on. The marker comments for those follow-ups remain. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Apply +_+_+ rename markers across Rust + Solidity sources:
- `AnnouncesCommitted{,Event}` → `MBCommitted{,Event}` (Rust struct, enum variant, Solidity event)
- `LastAdvancedEthBlockCommitted{,Event}` → `EBCommitted{,Event}` (Rust struct, enum variant, Solidity event)
- `ANNOUNCES_COMMITTED` → `MB_COMMITTED`, `LAST_ADVANCED_ETH_BLOCK_COMMITTED` → `EB_COMMITTED` (constants)
- `announces_committed` → `mb_committed`, `last_advanced_eth_block_committed` → `eb_committed` (methods)
- `last_committed_advanced_eth_block` → `last_committed_eb` (field in BlockMeta / PreparedBlockData)
- `last_advanced_block` → `last_advanced_eb` (field in MbMeta)
- `latest_synced_block` → `latest_synced_eb`, `latest_prepared_block_hash` → `latest_prepared_eb_hash` (DBGlobals)
- `CompactBlock` → `CompactMB` (struct)
- camelCase counterparts in Solidity
Forge build regenerated ABI JSON for Router/Mirror/Middleware/POAMiddleware/WrappedVara.
`ensure_types_unchanged` hash bumped to reflect the type-name changes (SCALE encoding unchanged).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The module owns Malachite-sequencer application types (`Transaction`, `Transactions`, etc.) so `malachite` is the more accurate name. Updates all import paths and the `ensure_types_unchanged` hash for the new module path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The validator state more accurately is the "idle" state (waiting for chain head, then sync, then prepare before electing a role). Renames: - module `wait_for_eth_block` → `idle` - struct `WaitForEthBlock` → `Idle` - display tag `WAIT_FOR_ETH_BLOCK` → `IDLE` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The rate-stepping promise-latency experiment runner lives on a separate branch now; drop it from the main branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a `Migration` trait and `migrate()` driver that walks ascending schema versions in `migrations()`. The vector is empty for now — the old v1/v2/... entries had no live consumers and were removed earlier. Wire `migrate()` into `initialize_db()` so an on-disk version below `LATEST_VERSION` runs the framework instead of bailing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The handler now reads `mb_program_states(at)` and returns the set of `ActorId`s present there, instead of warning and returning an empty response. Added `MbStorageRO` to the `DbSyncDatabase` trait so the network layer can reach the MB stores. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The MB-driven path has no active consumers of db_sync responses: every incoming response was being logged-and-dropped, and every failure was just re-issued forever. Drop the `network_fetcher` future, the `Event::Fetching` variant, and their imports. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Renumbers the test list to mirror master's ordering for tests common to both branches; new MB-specific tests (`multiple_validators_ping`, `reorg_within_quarantine`, `reorg_deeper_than_quarantine`) slot in where their thematic neighbors live. Also drop the now-stale `Event::Fetching` variant from the `TestingEvent` mirror — it was already gone from the runtime enum. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`MalachiteService::new` now takes `Option<PublicKey>`: - `Some(pk)` → `NodeRole::Validator`, validator key extracted from the signer. - `None` → `NodeRole::FullNode`, ephemeral secret used only as the libp2p peer identity. The service's call site no longer skips Malachite when no validator key is configured, so connect/full nodes also join the gossip mesh and receive proposals + sync responses. Test harness call sites updated to pass `Some(pub_key)`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move `db.set_injected_transaction(tx)` into `InjectedTxMempool::insert` so the network and RPC handlers don't have to remember to do it themselves. The two service-side call sites lose the duplicated persistence calls. Persistence happens before pool insertion so a producer that picks the tx on the very next round is guaranteed to find it via `injected_getTransactions`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`ethexe check --computation` now walks `latest_finalized_mb_hash`
back through `CompactMB.parent` and asserts that every MB has its
cached `mb_program_states` / `mb_outcome` / `mb_schedule` records
plus `MbMeta { computed: true }`.
Re-execution through the processor (asserting cached records match
fresh execution) is intentionally left out — it requires loading
every code blob and reconstructing the runtime, which the CLI doesn't
have the context for.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…k entry points Add `StateDump::collect_from_mb_storage(storage, mb_hash, block_hash)` as the malachite-native entry point — state lives per-MB, so callers that already know which MB to dump shouldn't have to round-trip through `BlockMeta::last_committed_mb`. The existing `collect_from_storage(block_hash)` becomes a thin wrapper that derives the MB and forwards. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`WaitForUploadCode`, `WaitForProgramCreation`, and `WaitForReplyTo`
each had an inline `select! { sleep => evm_mine, … = wait => break }`
loop that duplicated the `KickingStream::find_map` kick mechanism.
Each `wait_for` now hands the `(provider, block_time*3)` kick to the
receiver via `set_kicks` before consuming it with
`filter_map_block_synced`; the resulting `KickingStream` triggers
`evm_mine` on idle automatically.
`KickExt` retains its `EventReceiver` impl so `WaitFor*` can mutate
kicks pre-conversion. Replaced the marker on `extend_malachite_endpoints`
with a TODO note — pulling endpoint setup forward into `TestEnv::new`
needs a wider tests refactor and is left for a follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Promotes `apis/injected.rs` to `apis/injected/` and splits it into: - `mod.rs` — module entry, public re-exports - `trait.rs` — JSON-RPC trait definition (`Injected`) - `promise_manager.rs` — `PromiseSubscriptionManager` owns the `tx_hash → oneshot::Sender` map plus `try_register_subscriber`, `cancel_registration`, and `dispatch_promise` - `relay.rs` — `TransactionsRelayer` does the per-validator fan-out and the single-recipient fallback when the era's validator vector isn't known yet - `spawner.rs` — `spawn_pending_subscriber` bridges a registered subscriber to a jsonrpsee subscription sink with timeout / cleanup - `server.rs` — `InjectedApi` orchestrates all of the above Adds the `injected_getTransactionPromise` method that reads `db.promise(tx_hash)` and `db.compact_promise(tx_hash)` and reconstructs the `SignedPromise` via `SignedCompactPromise::restore`. This is the user-facing payoff of the CompactPromise primitives added in the foundation commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces every `Option<mpsc::UnboundedSender<Promise>>` field / parameter in the processor with `Option<BoundPromiseSink>`. The sink wraps an `UnboundedSender<(H256, Promise)>` and pre-binds each send to the MB hash the executor is currently working on, so worker threads no longer have to thread the binding through manually. Ripples: - `host::threads::ThreadParams.promise_out_tx` → `promise_sink` - `clear_promise_out_tx` → `clear_promise_sink` - `CommonRunContext.promise_out_tx` → `promise_sink` - compute's per-MB channel now carries `(H256, Promise)`; the `MbPromisesStream` reads the hash from each message instead of storing it in a field - compute constructs `BoundPromiseSink::new(sender, target_hash)` for the target MB and passes `None` for predecessor MBs (unchanged semantics) - processor unit tests upgrade their channel to `(H256, Promise)` and wrap senders in `BoundPromiseSink::new(_, H256::zero())` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…h compute_mb Closes the remaining `+_+_+` marker on `MbComputeRequest`: - `MbComputeRequest` gains a `promise_policy: PromisePolicy` field - `ComputeSubService::receive_mb(mb_hash, policy)` records it - `compute()` constructs a `BoundPromiseSink` for the target MB only when `policy == Enabled`, preserving the rule that ancestor MBs always replay with promises disabled - `ComputeService` carries a `PromiseEmissionMode` (default `ConsensusDriven`) and offers `with_promise_mode(...)`. In `AlwaysEmit` it overrides the per-call policy to `Enabled` - service-side `compute_mb` call passes `PromisePolicy::Enabled` for `MalachiteEvent::BlockProposal` (consensus may still narrow it via the emission mode) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- `cargo fmt --all` across all touched files (line-wrap reorganisation only). - `Migration::from_version` → `source_version` (clippy: `wrong_self_convention`). - ProgramIds db-sync handler uses `BTreeMap::into_keys()` instead of `into_iter().map(|(k, _)| k)` (clippy: `iter_kv_map`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…IONS
Splits the migrations module back into the historical shape:
- `migration.rs` — `Migration` trait (now `Sync` so the slice can be
`&'static`). `migrate(&InitConfig, &RawDatabase)` returns a pinned
boxed future so `&dyn Migration` is dyn-compatible.
- `v5.rs` — schema-version anchor. Holds `pub const VERSION: u32 = 5;`
and nothing else. Future schema bumps add `v6.rs`, `v7.rs`, … each
with their own `VERSION` and (where applicable) a
`migration_from_v(N-1)` function.
- `mod.rs` — re-introduces the user-requested constants:
pub const OLDEST_SUPPORTED_VERSION: u32 = v5::VERSION;
pub const LATEST_VERSION: u32 = v5::VERSION;
pub const MIGRATIONS: &[&dyn Migration] = &[];
const _: () = assert!(
(LATEST_VERSION - OLDEST_SUPPORTED_VERSION) as usize == MIGRATIONS.len(),
"Wrong number of migrations available"
);
The const-assert guarantees every step in the supported version
range has a corresponding migration entry — adding a new schema
version without a migration step (or vice versa) now fails to
compile. Both anchor constants point at `v5::VERSION` for now;
splits into distinct OLDEST/LATEST anchors when the next migration
lands.
- `init.rs` — `migrate(&config, &db).await` (was `migrate(&db)`).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`EventReceiver<T>` no longer carries a `kicks: Option<…>` field nor implements `KickExt`. Kicks live exclusively on the `KickingStream<S>` wrapper, and `TestingEventReceiver` / `ObserverEventReceiver` are now type aliases for `KickingStream<EventReceiver<…>>`. Side effects: - `channel<T>(db, kicks)` returns `(EventSender, KickingStream<EventReceiver<T>>)`. - `KickingStream` gains `Debug + Clone` derives (the inner `EventReceiver` is `Debug + Clone`) and forwards `FusedStream` from its inner stream. - `TestingEventReceiver::find_map_with_db` and `ObserverEventReceiver:: filter_map_*` switch from `self.db`/`self.kicks` field access to `self.db()`/`self.take_kicks()` helpers on the wrapper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request transitions the ethexe value claim mechanism to a pull-based model using Merkle proofs, involving updates to state transitions, Solidity contracts, and the addition of proof-related RPC endpoints. Feedback identifies several critical issues: the consensus logic fails to store outgoing actions during the validation path, which would prevent non-producer nodes from serving proofs; the CLI's --watch functionality is broken due to commented-out code; and the node loader's claim tracking is currently disabled with placeholders. These regressions must be addressed to maintain system statistics and user-facing functionality.
| if let Ok(Some(BatchCommitment { | ||
| chain_commitment: Some(ChainCommitment { transitions, .. }), | ||
| .. | ||
| })) = batch_commitment.as_ref() | ||
| { | ||
| for StateTransition { | ||
| new_state_hash, | ||
| value_claims, | ||
| .. | ||
| } in transitions | ||
| { | ||
| let mut outgoing_actions = vec![]; | ||
|
|
||
| for value_claim in value_claims { | ||
| outgoing_actions.push(OutgoingAction::ValueClaim(value_claim.clone())); | ||
| } | ||
|
|
||
| let outgoing_actions: OutgoingActions = outgoing_actions.into(); | ||
| self.db | ||
| .set_outgoing_actions(*new_state_hash, outgoing_actions); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for storing outgoing actions in the database is currently only implemented in the batch creation path (create_batch_commitment). It must also be included in the validation path (validate_batch_commitment) to ensure that all validators store these actions. Without this, non-producer nodes will be unable to serve RPC requests for Merkle proofs, which breaks the pull-based claim model for users connected to those nodes.
…outgoing_action tests `Mirror._merkleRoots` is keyed per state hash and stores the merkle root of `value_claims` committed in that specific transition. The follow-up "force-fold" no-op submitted by these tests lands in an MB without claims, so `mirror.query().state_hash()` returns a stateHash whose merkleRoot is bytes32(0) and `processOutgoingAction` reverts with `OutgoingActionMerkleRootNotFound` whenever the claim and the no-op end up in different MBs. Use a `find_value_claim_proof` helper that walks the local Mirror.StateChanged stream, probes `db.outgoing_actions(stateHash)`, and returns the stateHash that actually carries the claim. The `mailbox` test additionally re-derives the surrounding `mb_hash` via `mb_outcome` because Router.MBCommitted is emitted after Mirror.StateChanged in the same eth block. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
StackOverflowExcept1on
force-pushed
the
av/ethexe-value-claims-merkle-root
branch
from
413ba5b to
f6d3dce
Compare
StackOverflowExcept1on
changed the base branch from
master
to
gsobol/ethexe/malachite-new
|
@claude opus review |
|
Claude finished @StackOverflowExcept1on's task in 10m 52s —— View job Code review — OpusReviewed against severity: critical — File:
Since the actions are needed by every node that exposes RPC, persistence has to happen wherever severity: high — File: For
Either gate the function so anything other than severity: high —
The whole non-trivial algorithm — position parity, the "last node in odd-width row" lift case, severity: medium — File: bool success = _transferEther(destination, value);
require(success, ValueClaimFailed(messageId, value));
severity: medium — comment for File: /**
* @dev `DISCRIMINANT_SIZE + MESSAGE_ID_SIZE + DESTINATION_SIZE + VALUE_SIZE` offset.
*/
uint256 internal constant OFFSET3 = 53;
Other observations (not findings)
|
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| pub async fn vara_eth_api(&self, ethereum: Ethereum) -> Option<VaraEthApi> { |
There was a problem hiding this comment.
We were planning to rewrite tests to SDK. I'd love to hear your thoughts on this. Since we're moving to RPC API for processOutgoingAction, we need method for creating SDK instance.
4 participants
Closes #5442
Closes #5463
Summary
Transition to merkle roots, but only for value claims for now
TODO
How to test
See tests that use merkle proofs to claim value
Notes
This will break state of programs on Ethereum, but will not break them in database
Checklist
type(scope): description)