feat(proofs): add bounded historical proof history#142
Conversation
validate_payload already registered an expectation in convert_payload_to_block; clearing it on the early L1-index reject path matches the inner-failure cleanup and avoids a stale cache entry until LRU eviction.
Format the L1-index cleanup path, upgrade crossbeam-epoch to 0.9.20, and drop advisory ignores that no longer match after the reth main bump.
📝 WalkthroughWalkthroughAdds bounded historical EIP-1186 proof storage using MDBX, an EXEX synchronization and pruning pipeline, proof-history RPC endpoints, CLI management commands, node startup wiring, metrics, tests, and documentation. ChangesHistorical proof storage
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Node
participant ProofsExEx
participant MdbxProofsStorage
participant ProofRpc
Node->>ProofsExEx: start proof-history EXEX
ProofsExEx->>MdbxProofsStorage: initialize and sync block updates
ProofsExEx->>MdbxProofsStorage: prune retained history
ProofRpc->>MdbxProofsStorage: read proof window snapshot
MdbxProofsStorage-->>ProofRpc: historical proof data
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
crates/proofs/src/live.rs (1)
92-92: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant full-block clone on the execution hot path.
Executor::executetakes a&RecoveredBlock, so&(*block).clone()clones the entire block (including all transactions) only to borrow it immediately. This runs on the batch/cold catch-up path where many blocks are re-executed, so the extra allocation and copy is pure waste. Pass the existing reference instead.
crates/proofs/src/live.rs#L92-L92: replaceblock_executor.execute(&(*block).clone())?withblock_executor.execute(block)?(hereblock: &RecoveredBlock<...>).crates/proofs/src/live.rs#L408-L408: same change;blockis already&RecoveredBlock<...>.Please confirm the
executesignature in your reth version accepts&RecoveredBlockdirectly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/proofs/src/live.rs` at line 92, Remove the redundant full-block clones before execution: in crates/proofs/src/live.rs at lines 92-92 and 408-408, update both calls in the relevant execution flows to pass the existing block reference directly to Executor::execute. Confirm the reth version’s execute signature accepts &RecoveredBlock, preserving the existing error propagation.crates/proofs/src/prune/error.rs (1)
64-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
strum::Displaydiscards the underlying error and field context. This compiles (thiserror'sErrorderive only requires aDisplayimpl to exist and doesn't emit one without#[error]), butstrum::Displayrenders just the variant name —Storage,Provider,BlockNotFound,TimedOut— dropping the wrapped error message and theu64/Durationpayloads.
MorphProofStoragePruner::runlogs failures viaerr=%e(crates/proofs/src/prune/pruner.rsLine 216), so the actual cause (e.g. which block was missing, or the inner storage/provider error) is lost in production logs. Consider replacingstrum::Displaywith thiserror#[error("...")]messages:♻️ Proposed change
-use strum::Display; use thiserror::Error; @@ -/// Error returned by the pruner. -#[derive(Debug, Error, Display)] +/// Error returned by the pruner. +#[derive(Debug, Error)] pub enum PrunerError { /// Wrapped error from the underlying `MorphProofStorage` layer. - Storage(#[from] MorphProofsStorageError), + #[error(transparent)] + Storage(#[from] MorphProofsStorageError), /// Wrapped error from the reth db provider. - Provider(#[from] ProviderError), + #[error(transparent)] + Provider(#[from] ProviderError), /// Block not found in the underlying reth storage provider. - BlockNotFound(u64), + #[error("block {0} not found in the underlying reth storage provider")] + BlockNotFound(u64), /// The pruner timed out before finishing the prune - TimedOut(Duration), + #[error("pruner timed out after {0:?}")] + TimedOut(Duration), }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/proofs/src/prune/error.rs` around lines 64 - 78, Replace the strum::Display derive on PrunerError with explicit thiserror #[error(...)] annotations for every variant. Ensure Storage and Provider include their wrapped source errors, while BlockNotFound and TimedOut include their u64 and Duration payloads, so MorphProofStoragePruner::run preserves complete failure context in err=%e logs.crates/proofs/src/db/store.rs (1)
693-713: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid cloning the whole
HistoryDeleteBatchper table.Each
history.clone()copies all four vectors and discards three, so this performs 4× full-batch allocations on the prune/unwind hot path. Compute the counts first, then move each field intodelete_dup_sortedby value.♻️ Proposed change
- // Delete using the simplified API: iterator of (key, subkey) - self.delete_dup_sorted::<AccountTrieHistory, _, _>(tx, history.clone().account_trie)?; - self.delete_dup_sorted::<StorageTrieHistory, _, _>(tx, history.clone().storage_trie)?; - self.delete_dup_sorted::<HashedAccountHistory, _, _>(tx, history.clone().hashed_account)?; - self.delete_dup_sorted::<HashedStorageHistory, _, _>(tx, history.clone().hashed_storage)?; - - Ok(WriteCounts { - account_trie_updates_written_total: history.account_trie.len() as u64, - storage_trie_updates_written_total: history.storage_trie.len() as u64, - hashed_accounts_written_total: history.hashed_account.len() as u64, - hashed_storages_written_total: history.hashed_storage.len() as u64, - }) + let counts = WriteCounts { + account_trie_updates_written_total: history.account_trie.len() as u64, + storage_trie_updates_written_total: history.storage_trie.len() as u64, + hashed_accounts_written_total: history.hashed_account.len() as u64, + hashed_storages_written_total: history.hashed_storage.len() as u64, + }; + + // Delete using the simplified API: iterator of (key, subkey) + self.delete_dup_sorted::<AccountTrieHistory, _, _>(tx, history.account_trie)?; + self.delete_dup_sorted::<StorageTrieHistory, _, _>(tx, history.storage_trie)?; + self.delete_dup_sorted::<HashedAccountHistory, _, _>(tx, history.hashed_account)?; + self.delete_dup_sorted::<HashedStorageHistory, _, _>(tx, history.hashed_storage)?; + + Ok(counts)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/proofs/src/db/store.rs` around lines 693 - 713, Update the history deletion flow around delete_dup_sorted to avoid cloning the entire HistoryDeleteBatch for each table. Compute the four vector lengths before consuming history, then move account_trie, storage_trie, hashed_account, and hashed_storage by value into their respective delete_dup_sorted calls, preserving the existing WriteCounts values.bin/morph-reth/src/proofs.rs (1)
68-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared storage-path/env args to remove 3x duplication.
InitCommand,PruneCommand, andUnwindCommandall repeat the identicalenv+storage_pathfields and the identicalenv.init(...)→ resolve path →open_storage(...)sequence.♻️ Proposed refactor: shared flattened args + helper
#[derive(Debug, Parser)] struct ProofStorageArgs { #[command(flatten)] env: EnvironmentArgs<MorphChainSpecParser>, /// Proof-history MDBX directory (defaults to `<chain-datadir>/historical-proofs`). #[arg(long = "proofs-history.storage-path", value_name = "PATH")] storage_path: Option<PathBuf>, } impl ProofStorageArgs { fn open(&self, runtime: reth_tasks::Runtime) -> eyre::Result<(Environment, MorphProofsStorage<Arc<MdbxProofsStorage>>)> { let env = self.env.init::<MorphNode>(AccessRights::RO, runtime)?; let path = self .storage_path .clone() .unwrap_or_else(|| env.data_dir.data_dir().join("historical-proofs")); let storage = open_storage(&path, &self.env.chain)?; Ok((env, storage)) } }Then each command flattens
#[command(flatten)] shared: ProofStorageArgsand callslet (Environment { provider_factory, .. }, storage) = self.shared.open(runtime)?;.Also applies to: 78-88, 115-123, 144-153, 163-171, 178-187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/morph-reth/src/proofs.rs` around lines 68 - 76, Extract the duplicated env and storage_path fields from InitCommand, PruneCommand, and UnwindCommand into a shared ProofStorageArgs type. Add a ProofStorageArgs::open helper containing the common env.init, default historical-proofs path resolution, and open_storage logic, then flatten this shared type into each command and use its returned environment and storage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/proofs/src/in_memory.rs`:
- Around line 820-822: Update InMemoryStorage::unwind_history when computing
unwind_upto_block_number to use saturating subtraction, matching the MDBX
backend and preventing underflow for block number 0.
In `@crates/proofs/src/lib.rs`:
- Around line 19-20: Update the DEFAULT_PROOFS_HISTORY_WINDOW constant to
1,296,000 blocks and revise its documentation to describe the PR-defined
retention window without claiming seven days at one-second block time. Preserve
any downstream startup or CLI wiring that consumes this constant.
In `@README.md`:
- Line 126: Update the README table entry for --proofs-history.window to
document the actual default of 1,296,000 blocks and revise the retention
description from 7 days to 15 days at 1-second blocks.
---
Nitpick comments:
In `@bin/morph-reth/src/proofs.rs`:
- Around line 68-76: Extract the duplicated env and storage_path fields from
InitCommand, PruneCommand, and UnwindCommand into a shared ProofStorageArgs
type. Add a ProofStorageArgs::open helper containing the common env.init,
default historical-proofs path resolution, and open_storage logic, then flatten
this shared type into each command and use its returned environment and storage.
In `@crates/proofs/src/db/store.rs`:
- Around line 693-713: Update the history deletion flow around delete_dup_sorted
to avoid cloning the entire HistoryDeleteBatch for each table. Compute the four
vector lengths before consuming history, then move account_trie, storage_trie,
hashed_account, and hashed_storage by value into their respective
delete_dup_sorted calls, preserving the existing WriteCounts values.
In `@crates/proofs/src/live.rs`:
- Line 92: Remove the redundant full-block clones before execution: in
crates/proofs/src/live.rs at lines 92-92 and 408-408, update both calls in the
relevant execution flows to pass the existing block reference directly to
Executor::execute. Confirm the reth version’s execute signature accepts
&RecoveredBlock, preserving the existing error propagation.
In `@crates/proofs/src/prune/error.rs`:
- Around line 64-78: Replace the strum::Display derive on PrunerError with
explicit thiserror #[error(...)] annotations for every variant. Ensure Storage
and Provider include their wrapped source errors, while BlockNotFound and
TimedOut include their u64 and Duration payloads, so
MorphProofStoragePruner::run preserves complete failure context in err=%e logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89868985-c564-4c54-9d4a-51fe61b82a45
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (55)
Cargo.tomlREADME.mdbin/morph-reth/Cargo.tomlbin/morph-reth/src/main.rsbin/morph-reth/src/proofs.rscrates/node/Cargo.tomlcrates/node/src/add_ons.rscrates/node/src/args.rscrates/node/src/node.rscrates/node/tests/it/main.rscrates/node/tests/it/proof_history.rscrates/proofs-exex/Cargo.tomlcrates/proofs-exex/NOTICE.mdcrates/proofs-exex/src/lib.rscrates/proofs-exex/src/sync_target.rscrates/proofs/Cargo.tomlcrates/proofs/NOTICE.mdcrates/proofs/src/api.rscrates/proofs/src/batch_provider.rscrates/proofs/src/cursor.rscrates/proofs/src/cursor_factory.rscrates/proofs/src/db/batch.rscrates/proofs/src/db/cursor.rscrates/proofs/src/db/mod.rscrates/proofs/src/db/models/block.rscrates/proofs/src/db/models/change_set.rscrates/proofs/src/db/models/kv.rscrates/proofs/src/db/models/metadata.rscrates/proofs/src/db/models/mod.rscrates/proofs/src/db/models/storage.rscrates/proofs/src/db/models/version.rscrates/proofs/src/db/store.rscrates/proofs/src/error.rscrates/proofs/src/in_memory.rscrates/proofs/src/initialize.rscrates/proofs/src/lib.rscrates/proofs/src/live.rscrates/proofs/src/metrics.rscrates/proofs/src/proof.rscrates/proofs/src/provider.rscrates/proofs/src/prune/error.rscrates/proofs/src/prune/metrics.rscrates/proofs/src/prune/mod.rscrates/proofs/src/prune/pruner.rscrates/proofs/src/prune/task.rscrates/proofs/tests/identity.rscrates/rpc/Cargo.tomlcrates/rpc/src/eth/mod.rscrates/rpc/src/eth/proofs.rscrates/rpc/src/lib.rscrates/rpc/src/proof_status.rscrates/rpc/src/state.rslocal-test/README.mdlocal-test/reset.shlocal-test/reth-start.sh
💤 Files with no reviewable changes (1)
- local-test/reth-start.sh
| fn unwind_history(&self, unwind_upto_block: BlockWithParent) -> MorphProofsStorageResult<()> { | ||
| let mut inner = self.inner.write(); | ||
| let unwind_upto_block_number = unwind_upto_block.block.number - 1; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Potential subtraction underflow on block 0.
unwind_upto_block.block.number - 1 panics when the target block number is 0. The MDBX backend uses saturating_sub(1) here; mirror that to keep the test double from diverging and panicking.
🛡️ Proposed fix
- let unwind_upto_block_number = unwind_upto_block.block.number - 1;
+ let unwind_upto_block_number = unwind_upto_block.block.number.saturating_sub(1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn unwind_history(&self, unwind_upto_block: BlockWithParent) -> MorphProofsStorageResult<()> { | |
| let mut inner = self.inner.write(); | |
| let unwind_upto_block_number = unwind_upto_block.block.number - 1; | |
| fn unwind_history(&self, unwind_upto_block: BlockWithParent) -> MorphProofsStorageResult<()> { | |
| let mut inner = self.inner.write(); | |
| let unwind_upto_block_number = unwind_upto_block.block.number.saturating_sub(1); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/proofs/src/in_memory.rs` around lines 820 - 822, Update
InMemoryStorage::unwind_history when computing unwind_upto_block_number to use
saturating subtraction, matching the MDBX backend and preventing underflow for
block number 0.
| /// Default proof-history retention window: 7 days at a one-second block time. | ||
| pub const DEFAULT_PROOFS_HISTORY_WINDOW: u64 = 604_800; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the default retention window with the PR contract.
This exports 604_800 and documents a seven-day, one-second-based window, while the PR objective requires a default of 1_296_000 blocks. If downstream startup/CLI wiring uses this constant, nodes will prune history earlier than promised.
Proposed fix
-/// Default proof-history retention window: 7 days at a one-second block time.
-pub const DEFAULT_PROOFS_HISTORY_WINDOW: u64 = 604_800;
+/// Default proof-history retention window in blocks.
+pub const DEFAULT_PROOFS_HISTORY_WINDOW: u64 = 1_296_000;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Default proof-history retention window: 7 days at a one-second block time. | |
| pub const DEFAULT_PROOFS_HISTORY_WINDOW: u64 = 604_800; | |
| /// Default proof-history retention window in blocks. | |
| pub const DEFAULT_PROOFS_HISTORY_WINDOW: u64 = 1_296_000; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/proofs/src/lib.rs` around lines 19 - 20, Update the
DEFAULT_PROOFS_HISTORY_WINDOW constant to 1,296,000 blocks and revise its
documentation to describe the PR-defined retention window without claiming seven
days at one-second block time. Preserve any downstream startup or CLI wiring
that consumes this constant.
| | `--rpc.eth-proof-window` | 0 (disabled) | Max historical blocks for `eth_getProof` (up to 1209600) | | ||
| | `--proofs-history` | false | Enable historical `eth_getProof` and proof-history accumulation | | ||
| | `--proofs-history.storage-path` | `<chain-datadir>/historical-proofs` | Override the proof MDBX directory | | ||
| | `--proofs-history.window` | 604800 | Number of canonical blocks retained (7 days at 1s/block) | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the documented retention default.
Line 126 says the default is 604800, but the proof-history default is 1,296,000 blocks (15 days at 1-second blocks). Update the README so operators do not configure or size storage for the wrong retention window.
Proposed documentation fix
-| `--proofs-history.window` | 604800 | Number of canonical blocks retained (7 days at 1s/block) |
+| `--proofs-history.window` | 1296000 | Number of canonical blocks retained (15 days at 1s/block) |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | `--proofs-history.window` | 604800 | Number of canonical blocks retained (7 days at 1s/block) | | |
| | `--proofs-history.window` | 1296000 | Number of canonical blocks retained (15 days at 1s/block) | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 126, Update the README table entry for
--proofs-history.window to document the actual default of 1,296,000 blocks and
revise the retention description from 7 days to 15 days at 1-second blocks.
1 participant
Summary
eth_getProofrequests on both normal and authenticated RPCproofs init,proofs prune, andproofs unwindoperator commandsThe versioned-trie implementation is adapted from Base commit
b2673bbd927cb34d7cfad4d448bfbd5bd30eae88under MIT; Morph owns the integration,MDBX backend, lifecycle, RPC policy, and tests.
Behavior
--proofs-historyproofs initanchors the proof database at the current canonical tip; there is no backward backfill, schema migration, or automatic data deletionproofs pruneexplicitlyeth_getProofaccepts only the inclusive durable window, checks the latest stored canonical hash, and limits each request to 1,024 storage keysdebug_proofsSyncStatusreports earliest/latest bounds from one MDBX snapshot<chain-datadir>/historical-proofs; cold snapshots remain whole-data-directory copiesCLI
Optional node settings:
--proofs-history.storage-path <PATH>--proofs-history.window <BLOCKS>--proofs-history.verification-interval <BLOCKS>Validation
cargo fmt --all -- --checkcargo test --workspace --no-fail-fastcargo clippy --workspace --all-targets --all-features -- -D warningsmorph-proofsandmorph-proofs-exexeth_getProofmatchScope
This PR does not add a reference-index disable flag, reference-index status RPC,
snapshot format, compatibility migration, or benchmark harness. Live long-running
network sync, Hive/geth datasets, and performance benchmarks remain separate validation work.
Summary by CodeRabbit
eth_getProof.