Skip to content

feat: add LocalDataSource trait for block/bytecode fetching#135

Open
flyq wants to merge 8 commits into
mainfrom
liquan/adapt_mega_reth
Open

feat: add LocalDataSource trait for block/bytecode fetching#135
flyq wants to merge 8 commits into
mainfrom
liquan/adapt_mega_reth

Conversation

@flyq
Copy link
Copy Markdown
Member

@flyq flyq commented May 14, 2026

Summary

Introduces a LocalDataSource trait so the validator can pull blocks and contract bytecode from a host process (a full node embedding the validator) instead of always round-tripping through RPC. The witness path stays on RPC — only replayers produce witnesses, so that source is inherently remote — but blocks and bytecode are now fetched through the trait.

Motivated by embedding the validator into mega-reth, where both are already in-memory. Default behavior is unchanged: RpcClient implements LocalDataSource, so existing deployments keep using RPC.

Changes

New trait — crates/stateless-common/src/local_source.rs

  • LocalDataSource: object-safe async trait (Arc<dyn LocalDataSource>) with block_by_hash and codes_by_hashes.
  • LocalDataError: typed errors — Deadline, CodeVerification, BlockMissing, Provider. From<CodeFetchError> flattens CodeFetchError::Deadline into LocalDataError::Deadline so callers only match the deadline condition once.
  • Blanket impl for RpcClient preserves the RPC-only deployment path.
  • In-memory mock + 2 unit tests for the missing-block and partial-codes cases.

Validator wiring — bin/stateless-validator/src/{chain_sync,workers}.rs

  • ValidatorFetcher and ValidatorProcessor now hold Arc<dyn LocalDataSource> for block/bytecode fetches. ValidatorFetcher still keeps Arc<RpcClient> for the witness call.
  • Block fetch switched from by-number to by-hash (after get_block_hash), so a reorg between the hash lookup and the block fetch surfaces as a hash mismatch instead of silently swapping in a different block.
  • ValidatorProcessor matches all four LocalDataError variants: CodeVerification → non-retryable validation failure; BlockMissing / Provider → retryable; Deadlineunreachable! (we pass deadline=None).

Integration tests — bin/stateless-validator/tests/integration.rs

  • Mock RPC server now also handles eth_getBlockByHash (the new fetch path). shape_block helper deduplicates the full/hashes-only response shaping shared with eth_getBlockByNumber.
  • Test wiring updated to plumb Arc<dyn LocalDataSource> through both fetcher and processor.

Workspace

  • Adds async-trait = "0.1" (used by the new trait).

Test plan

  • cargo check --workspace passes
  • cargo test -p stateless-common local_source::tests — new unit tests pass
  • cargo test -p stateless-validator --test integration — by-hash block path + processor wiring exercised end-to-end
  • Manual: run the validator against a real RPC and confirm chain sync continues to work (default LocalDataSource = RpcClient path)

@claude
Copy link
Copy Markdown

claude Bot commented May 14, 2026

The PR currently has no labels, but based on the diff it should have the enhancement label.

This PR introduces a new LocalDataSource trait for block/bytecode fetching, wires it into the validator, and adds new unit and integration tests. That's a new feature addition, which matches the enhancement label description ("New feature or request"). The conventional-commit prefix feat: in the title also confirms this.

Deadline(#[from] RpcDeadlineExceeded),

#[error(transparent)]
CodeVerification(CodeFetchError),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeVerification(CodeFetchError) couples the public error type to the RPC implementation. An embedded full-node impl that detects a bytecode hash mismatch has no natural CodeFetchError to construct — it must fall through to Provider(...) and lose the "verification failure" semantics. Consider replacing the inner type with a plain string or Box<dyn Error + Send + Sync>:

Suggested change
CodeVerification(CodeFetchError),
#[error("contract verification failed: {0}")]
CodeVerification(Box<dyn std::error::Error + Send + Sync>),

Then the From<CodeFetchError> conversion becomes:

CodeFetchError::VerificationFailure { .. } => {
    Self::CodeVerification(Box::new(e))
}

Comment thread crates/stateless-common/src/local_source.rs Outdated
@claude
Copy link
Copy Markdown

claude Bot commented May 14, 2026

The overall design is sound — introducing LocalDataSource as an object-safe trait cleanly separates witness (always remote) from block/bytecode (pluggable), and the by-hash fetch preventing silent block swaps is a good correctness improvement.

Two issues to address before merging:

1. codes_by_hashes partial-return semantics are undefined (see inline)
The trait doc doesn't say whether returning a smaller map than requested is legal. InMemorySource silently drops unknown hashes; the RPC impl never does (it errors on any missing hash). The consumer in chain_sync.rs doesn't check completeness, so a partial return would cause opaque EVM failures rather than a clear error. Make the contract explicit: all-or-nothing, consistent with get_codes_with_deadline.

2. CodeVerification(CodeFetchError) leaks the RPC error type (see inline)
A non-RPC LocalDataSource impl that detects a hash mismatch has no CodeFetchError to construct — it must fall back to Provider(...) and lose the verification-failure semantics. Replace the inner type with Box<dyn Error + Send + Sync>.

nit: async-trait is pragmatic for object-safe async traits even on nightly — no objection there.

/// Fetch the RPC-format block at `(number, hash)`. `hash` is authoritative; `number` is
/// for diagnostics. `deadline` is best-effort — local-DB impls may ignore it.
async fn block_by_hash(
&self,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This project targets nightly 2026-02-03, so async fn in traits is available natively — async-trait is unnecessary here. Using it adds a dependency and wraps every call in a Box<dyn Future> allocation.

Suggested change
&self,
pub trait LocalDataSource: Send + Sync + 'static {

Dropping #[async_trait] on the trait and all impls (including the test mock) removes the dependency entirely.


/// Fetch contract bytecode for a deduplicated list of codehashes. Each returned
/// bytecode's keccak must match the requested hash.
async fn codes_by_hashes(
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc says "Each returned bytecode's keccak must match the requested hash" but this is a correctness invariant for stateless validation, not just a convention. Implementors that skip verification (e.g. a local DB impl that trusts its own storage) will silently break block validation without the pipeline detecting it. Consider asserting this at the call site or adding a default method that enforces it, rather than relying solely on the doc comment.

@claude
Copy link
Copy Markdown

claude Bot commented May 14, 2026

Review

PR description vs. actual changes

The description says "ValidatorFetcher and ValidatorProcessor now hold Arc<dyn LocalDataSource>" — but they don't. Both still hold Arc<RpcClient> and call self.rpc_client.get_block(...) / self.rpc_client.get_codes(...) directly. LocalDataSource is defined and implemented for RpcClient, but never used in the validator binary. Please update the description to reflect what this PR actually does (define the trait, plumb the by-hash block fetch) vs. what a follow-up will do (wire Arc<dyn LocalDataSource> into the fetcher and processor).

async-trait is unnecessary on this toolchain

See inline comment on line 54 of local_source.rs. The nightly-2026-02-03 toolchain has async fn in traits natively; async-trait only adds a dependency and a Box<dyn Future> allocation per call.

Bytecode verification is a correctness invariant, not just a doc convention

See inline comment on line 62. An alternative LocalDataSource impl that skips keccak verification would compile and run silently while accepting tampered bytecode. This should be enforced structurally.

Smaller nits

  • LocalDataError::CodeFetch is #[error(transparent)] but does not delegate source via #[from] — the manual From<CodeFetchError> impl is clear, but the asymmetry with Deadline (which has #[from]) may confuse future readers. A brief comment would help.
  • The BlockId import in local_source.rs is used only inside the RpcClient impl block; consider moving it to a tighter scope.

What's good

The by-hash block fetch is a genuine correctness improvement — the old by-number path could silently race a reorg. The From<CodeFetchError> splitting of Deadline is clean and the rationale is well-explained. The integration test now covers eth_getBlockByHash end-to-end.

@claude
Copy link
Copy Markdown

claude Bot commented May 14, 2026

Three previous threads still need resolution before this is ready — and the PR description overstates what's in the diff.

PR description vs actual diff

The description says:

ValidatorFetcher and ValidatorProcessor now hold Arc<dyn LocalDataSource> for block/bytecode fetches. ValidatorProcessor matches all four LocalDataError variants…

None of that is in the diff. chain_sync.rs and workers.rs still use RpcClient directly. The trait is introduced but not wired into any production code path. Please update the description to match what's actually here — the integration is presumably a follow-up.

Open threads

  1. async-trait dependency (thread still open) — the project targets nightly 2026-02-03; native async fn in traits is available and #[async_trait] is unnecessary overhead (one Box<dyn Future> allocation per call).

  2. CodeFetch(CodeFetchError) variant (thread still open) — wrapping an RPC-specific error type in a trait meant for local sources. An embedded impl that detects a hash mismatch has no CodeFetchError to produce.

  3. Keccak invariant enforcement (thread still open) — the contract doc says implementations must verify keccak(bytecode) == hash, but nothing enforces it at the call site.

What's good

  • BlockId::Hash switch in chain_sync.rs is a real safety improvement.
  • All-or-nothing contract on codes_by_hashes is now documented and the InMemorySource mock enforces it correctly — resolved that thread.
  • shape_block helper is a clean deduplication.

@claude
Copy link
Copy Markdown

claude Bot commented May 15, 2026

The three open threads and the PR description all reference local_source.rs, which no longer exists in the PR.

Please update the title and description. The current diff does three things — none of which is "add LocalDataSource trait":

  1. Switch block fetch from by-number to by-hash (chain_sync.rs)
  2. Add wait_until_available pre-flight hook to BlockFetcher (pipeline/traits.rs, fetcher.rs)
  3. Add eth_getBlockByHash to the integration test mock server

The three open inline threads (CodeVerification coupling, async-trait, keccak enforcement) are now orphaned — they reference a file that was removed. Please resolve them so future reviewers are not misled.

Design question on wait_until_available: this method is added to the public BlockFetcher trait solely as groundwork for the mega-reth embedding that was removed from this PR. With LocalDataSource gone, there is no current non-default implementation or consumer. Per the project's own guidelines, speculative API surface should be deferred until it has a concrete user. Either land it in the follow-up that actually implements LocalDataSource, or keep it here if you plan to send that follow-up immediately.

The rest of the diff is clean. The by-hash fetch is a genuine correctness improvement and the test additions are correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant