fix(provider): handle storage wipes in batched persistence - #26750
Conversation
Co-authored-by: Derek Cofausper <256792747+decofe@users.noreply.github.com>
|
cyclops audit |
|
cc @klkvr Cyclops audit event published. View workflow run Config: config: |
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
Reviewed the current PR head 4c8b95290ac2769c0e19dad95b819c324f99e9c4 after head drift. The storage-wipe fallback still has actionable issues: two findings are inline on the changed fallback, and one defense-in-depth item is body-only because its primary line is outside the diff.
🛡️ [DEFENSE-IN-DEPTH] The sibling hashed-state disjoint merge still panics on wiped storage
Severity: Low
File: crates/storage/provider/src/providers/database/provider.rs:780
The new guard only protects the trie update merge. The same save path still calls HashedPostStateSorted::disjointed_merge_batch(&batch, &mask) before the guarded trie merge, and that implementation contains release-mode assert!(!storage.wiped) checks (crates/trie/common/src/hashed_state.rs:729-731 and :747-749). Current in-tree producers appear to encode destroyed storage as explicit zero-valued slots rather than wiped = true, so this is latent today, but downstream/future providers can still panic the persistence task instead of returning a typed error.
Recommended Fix: Mirror the trie fallback for HashedPostStateSorted when hashed storage is wiped, convert the asserts into typed provider errors, or explicitly enforce/document that block hashed states must never carry wiped storage.
Reviewer Callouts
- ⚡ Proof-v2 cursor resets: Add an end-to-end proof-v2 regression with mixed unknown/known parent target groups over a wiped storage trie; the unit-level cursor PoC covers the state machine, but the production reset site should be pinned too.
- ⚡ Producer semantics and observability: The current comments/tests document that serial updates can contain empty/transient wipes, but operators still cannot tell when partial-persistence masking has been disabled. A metric around the fallback would make the operational tradeoff visible.
- ⚡ Hashed wipe assumptions: If future code starts propagating
HashedStorageSorted::wiped, both the hashed-state disjoint merge and the hashed cursor reset path need the same treatment as trie-side wipes.
| // Sparse-trie streaming emits node updates, while the serial state-root path, | ||
| // including fallback, can emit a whole storage-trie wipe. `disjointed_merge_batch` | ||
| // rejects wipes, so use the ordered merge. Persisting updates shadowed by the | ||
| // masking suffix is redundant but safe: the in-memory overlay still takes |
There was a problem hiding this comment.
🚨 [SECURITY] InMemoryTrieCursor::reset() can drop the wipe that this fallback relies on
This safety claim depends on the masking suffix keeping a wiped storage trie opaque. On current head, InMemoryTrieCursor::reset() (crates/trie/trie/src/trie_cursor/in_memory.rs:371-381) always changes DbCursorState::Wiped to NeedsPosition. proof_v2::ProofCalculator::proof_inner calls self.trie_cursor.reset() between overlapping/backward sub-trie target ranges (crates/trie/trie/src/proof_v2/mod.rs:1564) without calling set_hashed_address again. After this branch persists the batch unmasked, a subsequent seek can read stale DB nodes that only the suffix wipe was supposed to hide.
Recommended Fix:
Preserve DbCursorState::Wiped across reset() (or persist a separate wipe flag and reconstruct the state), and add cursor plus proof-v2 regression tests for wiped storage overlays across reset.
| // With a revm release containing bluealloy/revm#3863, post-Cancun selfdestructs | ||
| // will no longer result in `storage.is_deleted` in serial trie updates. The flag | ||
| // remains valid `save_blocks` input when processing pre-Cancun historical data. | ||
| let contains_storage_wipe = batch.iter().chain(&mask).any(|updates| { |
There was a problem hiding this comment.
is_deleted semantics
contains_storage_wipe treats any StorageTrieUpdatesSorted::is_deleted in the persisted prefix or mask as a whole-storage wipe and switches the entire batch to unmasked merge_batch. The serial TrieUpdates::finalize path sets this flag for every destroyed account, including storage-less accounts (crates/trie/common/src/updates.rs:153-156), while sparse trie updates normally do not set it (crates/trie/sparse/src/state.rs:345, :415-417). Since timeout handling races sparse results against serial fallback (crates/engine/tree/src/tree/state_root_strategy/mod.rs:1185-1223), this can make physical trie persistence non-deterministic and silently disable partial-persistence masking for a whole window.
Recommended Fix:
Normalize the flag semantics and narrow the fallback to actual/per-account storage wipes. Add observability for when this fallback disables masked persistence.
Automated nightly update of reth dependencies from `paradigmxyz/reth` main branch. ## Upstream reth changes [`10aa6a5...00ff650`](paradigmxyz/reth@10aa6a5...00ff650) 🔗 Amp thread: https://ampcode.com/threads/T-01a036f1-9f58-7640-b789-4e3dd9778ebe - **Engine** - Improved payload building across canonical ancestors, finality, persistence handoffs, pending resolution, and responsive cancellation ([#26559](paradigmxyz/reth#26559), [#26567](paradigmxyz/reth#26567), [#26580](paradigmxyz/reth#26580), [#26708](paradigmxyz/reth#26708), [#26759](paradigmxyz/reth#26759)). - Added Bogota Engine API support and fork-time validation, plus FOCIL inclusion-list construction and stubs ([#26682](paradigmxyz/reth#26682), [#26706](paradigmxyz/reth#26706), [#26711](paradigmxyz/reth#26711), [#26737](paradigmxyz/reth#26737)). - Tightened payload and block-access-list validation, including state-gas admission and malformed BAL rejection ([#26651](paradigmxyz/reth#26651), [#26694](paradigmxyz/reth#26694), [#26719](paradigmxyz/reth#26719)). - Fixed little-endian cell bitvectors, Osaka `getBlobsV4`, and prewarm-worker shutdown ([#26650](paradigmxyz/reth#26650), [#26703](paradigmxyz/reth#26703), [#26768](paradigmxyz/reth#26768)). - Reused scratch buffers for faster BAL hash encoding ([#26701](paradigmxyz/reth#26701)). - **RPC** - Added `debug_traceChain`, Alloy trace-chain result types, block-level EVM reuse, and raw block transactions on the auth server ([#26582](paradigmxyz/reth#26582), [#26614](paradigmxyz/reth#26614), [#26669](paradigmxyz/reth#26669), [#26760](paradigmxyz/reth#26760)). - Added configurable response compression and request decompression ([#26668](paradigmxyz/reth#26668), [#20277](paradigmxyz/reth#20277)). - Added Bogota Engine API stubs and Amsterdam system contracts to `eth_config` ([#26691](paradigmxyz/reth#26691), [#26705](paradigmxyz/reth#26705)). - Fixed cancellation of payload-hash and blocking-I/O work ([#26569](paradigmxyz/reth#26569), [#26776](paradigmxyz/reth#26776)). - Corrected execution-witness block identifiers, optional receipt conversion/caching, and access-list environment preparation ([#26572](paradigmxyz/reth#26572), [#26596](paradigmxyz/reth#26596), [#26599](paradigmxyz/reth#26599), [#26598](paradigmxyz/reth#26598)). - Fixed testing block gas limits, transaction gas-limit preservation, timestamp overflow, and chain-ID validation ([#26632](paradigmxyz/reth#26632), [#26743](paradigmxyz/reth#26743), [#26767](paradigmxyz/reth#26767), [#26782](paradigmxyz/reth#26782)). - Added network-specific log responses and generic testing RPC handlers ([#26491](paradigmxyz/reth#26491), [#26547](paradigmxyz/reth#26547)). - **Networking** - Added ingress limits, configurable no-op client versions, and outbound `GetCells` support ([#26660](paradigmxyz/reth#26660), [#26652](paradigmxyz/reth#26652), [#26673](paradigmxyz/reth#26673)). - Improved handshake and protocol safety through ECIES identity checks, message-ID validation, negotiated-protocol assertions, and bad-message handling ([#26639](paradigmxyz/reth#26639), [#26654](paradigmxyz/reth#26654), [#26659](paradigmxyz/reth#26659), [#26671](paradigmxyz/reth#26671)). - Fixed ping/pong validation and pacing ([#26698](paradigmxyz/reth#26698), [#26702](paradigmxyz/reth#26702)). - Made eth/72 blob-cell announcements interoperable with geth while preserving availability masks ([#26573](paradigmxyz/reth#26573), [#26670](paradigmxyz/reth#26670)). - Shared the snap/2 slim-account codec between client and server ([#26587](paradigmxyz/reth#26587)). - **Txpool** - Added blob-cell availability tracking and exposure on pooled transactions ([#25463](paradigmxyz/reth#25463), [#26642](paradigmxyz/reth#26642)). - Included blob-pool transactions in queued counts and listings ([#26677](paradigmxyz/reth#26677), [#26679](paradigmxyz/reth#26679)). - Allowed senders with empty code hashes, added consensus encoding, and converted sender accessors to iterators ([#26644](paradigmxyz/reth#26644), [#26739](paradigmxyz/reth#26739), [#26681](paradigmxyz/reth#26681)). - **Trie & State** - Added partial trie unwind and persistence support, including changeset-cache handling ([#26543](paradigmxyz/reth#26543), [#26612](paradigmxyz/reth#26612)). - Corrected witness construction to use depth-first node order ([#26707](paradigmxyz/reth#26707)). - Simplified `HashedPostState` wipe handling and added trie-data reference collectors ([#26524](paradigmxyz/reth#26524), [#26752](paradigmxyz/reth#26752)). - **Storage & Providers** - Removed `ConsistentDbView`, relocated `OverlayStateProvider`, and simplified provider bounds ([#26581](paradigmxyz/reth#26581), [#26611](paradigmxyz/reth#26611), [#26591](paradigmxyz/reth#26591)). - Fixed storage-wipe handling during batched persistence ([#26750](paradigmxyz/reth#26750)). - Made RocksDB tolerate unknown column families ([#26647](paradigmxyz/reth#26647)). - Updated static-file consistency checks to respect prune checkpoints ([#26565](paradigmxyz/reth#26565)). - **Chainspec & Consensus** - Added Bogota hardfork support and validated block-access-list hashes during import ([#26686](paradigmxyz/reth#26686), [#26696](paradigmxyz/reth#26696)). - Honored the genesis `slotNumber` instead of hardcoding zero ([#26680](paradigmxyz/reth#26680)). - Defaulted unspecified payload attributes ([#26684](paradigmxyz/reth#26684)). - **DNS** - Fixed EIP-1459 discovery records by rejoining long TXT character strings and ignoring unrelated TXT records ([#26602](paradigmxyz/reth#26602), [#26603](paradigmxyz/reth#26603)). - **Snapshots & CLI** - Added base-URL resolution and exposed prepared snapshot context ([#26576](paradigmxyz/reth#26576), [#26777](paradigmxyz/reth#26777)). - Fixed history downloads when the final snapshot chunk is partial ([#26607](paradigmxyz/reth#26607)). - Added bootnode configuration to `reth.toml` ([#26551](paradigmxyz/reth#26551)). - **Testing & Development** - Improved engine reorg tests with explicit finality management and expanded execute-blob Hive coverage ([#26584](paradigmxyz/reth#26584), [#26606](paradigmxyz/reth#26606)). - Added a persistent-datadir testing node and made dev-mined blocks canonical immediately ([#26774](paradigmxyz/reth#26774), [#26761](paradigmxyz/reth#26761)). - **Bench** - Restored metrics visibility in benchmark run configurations ([#26461](paradigmxyz/reth#26461)). - **Dependencies & Releases** - Updated Alloy to 2.4.x, `alloy-hardforks`/`alloy-eip7928` to 0.4.8, and released Reth 2.5.0–2.5.1 ([#26663](paradigmxyz/reth#26663), [#26666](paradigmxyz/reth#26666), [#26685](paradigmxyz/reth#26685), [#26687](paradigmxyz/reth#26687), [#26700](paradigmxyz/reth#26700), [#26771](paradigmxyz/reth#26771)). ## Migrations 🔗 Amp thread: https://ampcode.com/threads/T-01a036f2-23b2-77c0-8959-5c3f0f9df6c5 - Upgraded Reth to `00ff650`, Alloy to `2.4.1`, and related dependencies to match their latest APIs. - Renamed workspace lint keys from kebab-case to snake_case for updated Cargo lint syntax. - Removed the no-longer-needed crate recursion limit. - Migrated hashed storage construction from the removed `from_iter` API to direct struct initialization. - Reused Reth’s prepared snapshot manifest, base URL, and data directory, removing Tempo’s duplicate manifest discovery, fetching, parsing, and path-resolution logic. - Updated snapshot planning to handle Reth’s new `(plan, prepared)` return value and execution’s optional prepared manifest. - Updated `HashedPostStateProvider` implementations and callers for its new fallible `ProviderResult` return type. - Added the required RPC log associated type and identity `convert_log` implementation for the updated receipt converter trait. - Updated pooled transaction construction for the reordered transaction field and new `blob_cell_availability` field. [GitHub Workflow](https://github.com/tempoxyz/tempo/actions/runs/32804636065) --------- Co-authored-by: Alexey Shekhirin <github@shekhirin.com> Co-authored-by: Alexey Shekhirin <5773434+shekhirin@users.noreply.github.com> Co-authored-by: Matthias Seitz <19890894+mattsse@users.noreply.github.com> Co-authored-by: Richard Janis Goldschmidt <701177+SuperFluffy@users.noreply.github.com>
…yz#26750) Co-authored-by: Matthias Seitz <19890894+mattsse@users.noreply.github.com> Co-authored-by: joshieDo <93316087+joshieDo@users.noreply.github.com>
Summary
Tests
Prompted by: @mattsse