Repository navigation
Conversation
|
derek bench preset=tip20 |
|
cc @decofe ❌ Benchmark complete: Regression View job ❌ Bench Comparison: RegressionRefs: df1547d vs reth-auto-bump Configuration
Tempo Metrics
Builder
Builder details
Validator
Observability |
📊 Tempo Precompiles CoverageprecompilesCoverage: 6426/10550 lines (60.91%) File details
contractsCoverage: 1/202 lines (0.50%) File details
Total: 6427/10752 lines (59.77%) |
|
cyclops audit note="Review every upstream reth commit included in the revision diff." |
|
cc @decofe Cyclops audit event published. View workflow run Config: config: |
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
One actionable finding survived consolidation: the reth dependency bump introduces a medium-severity full-block range download liveness issue. See the inline comment on Cargo.lock:10087; the root cause is in upstream reth crates/net/p2p/src/error.rs:30 and crates/net/p2p/src/full_block.rs:818.
Reviewer Callouts
- ⚡ Upstream reth networking: The relaxed partial-header policy was added for reverse-header recovery, but exact-range consumers still require complete batches. Please check all
GetBlockHeadersconsumers for whether short non-empty responses are actually consumable before keeping peers non-bad. - ⚡ Feature-gated upstream persistence: A worker flagged a crash-recovery edge case in upstream split-frontier/state-masking persistence when
partial-persistenceand non-zero state masking are enabled. This is not reported as a finding because Tempo’s default dependency features do not enablepartial-persistence, but it should be reviewed before enabling that feature.
|
cyclops audit note="Review every upstream reth commit included in the revision diff." |
|
cc @decofe Cyclops audit event published. View workflow run Config: config: |
|
cyclops audit note="Review every upstream reth commit included in the revision diff." |
|
cc @grandizzy Cyclops audit event published. View workflow run Config: config: |
|
cyclops audit note="Review every upstream reth commit included in the revision diff." |
|
cc @grandizzy Cyclops audit event published. View workflow run Config: config: |
|
cyclops audit note="Review every upstream reth commit included in the revision diff." |
|
cc @decofe Cyclops audit event published. View workflow run Config: config: |
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This update contains one reachable medium-severity RPC denial-of-service risk and two verified low-severity state-gas accounting defects behind currently disabled Amsterdam flags. The latter should be fixed before EIP-8037/EIP-2780 is enabled.
Reviewer Callouts
- ⚡ Historical replay after
CodeChangerevert correction: revm 42 now restores prior code on revert instead of clearing it. Confirm no historical reverted precompileset_codetargeted an already-coded account (crates/precompiles/src/storage/evm.rs:354-365). - ⚡ Validation-only checkpoint lifecycle:
validate_transactionrelies on its current caller'sdiscard_tx()to settle revm 42's pre-execution checkpoint. Settle it inside the helper before adding callers or EIP-2780 runtime failure handling (crates/revm/src/handler.rs:2248-2262). - ⚡ Vacuous Amsterdam state-gas tests: current TIP-1016 tests leave EIP-8037 disabled, so state-gas assertions remain zero. Exercise these paths with the Amsterdam table and flag enabled (
crates/revm/src/handler/tests.rs:66-88). - ⚡ Finality override trust boundary: confirm no authenticated Engine API route bypasses the consensus actor's finalized-height guard (
bin/tempo/src/defaults.rs:199-214,crates/consensus/src/executor/actor.rs:81-96).
|
cyclops audit note="Review every upstream reth commit included in the revision diff." |
|
cc @decofe Cyclops audit event published. View workflow run Config: config: |
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This Reth update is largely behavior-preserving, but one latent hashed-state inconsistency should be fixed before future protocol or account-model changes make it reachable.
Reviewer Callouts
- ⚡ Destroyed-account overlay cost: Reth's updated
MemoryOverlayStateProviderRef::hashed_post_statecan aggregate and clone every unpersisted block's trie input for a destroyed account withoriginal_info.is_some(), even when it has no storage slots. Measure this hot path against Tempo's persistence threshold and block budget, and consider reporting the avoidable aggregation upstream. - ⚡ New payload-build failure path:
crates/payload/builder/src/lib.rs:1032now propagates database errors from the destroyed-account cursor walk. Confirm that aborting payload construction is the intended proposer-liveness behavior for transient storage errors.
| hashed_state | ||
| } else { | ||
| Arc::new(finish_provider.hashed_post_state(&db.bundle_state)) | ||
| Arc::new(finish_provider.hashed_post_state(&db.bundle_state)?) |
There was a problem hiding this comment.
🛡️ [DEFENSE-IN-DEPTH] Keep sparse-trie and provider hashed states equivalent for destroyed accounts
This Reth bump changes hashed_post_state to append explicit zero entries for every pre-existing storage slot of an account where was_destroyed() && original_info.is_some(), but the branch above can still use the sparse-trie task's HashedPostState, whose conversion omits storage for destroyed accounts. Because the selected state is persisted with the payload and reused by the synchronous state-root fallback, task availability or failure can make nodes use different hashed states. Tempo's current EIP-6780/EIP-7610 rules appear to make this latent, but a future account-writing feature or hardfork change can expose persisted-state or state-root divergence.
Recommended Fix:
Make both branches derive the same hashed state, preferably by always calling this provider method; alternatively apply Reth's zero_destroyed_account_storage to the sparse-trie result. Add a differential test covering a destroyed account with pre-existing storage and report the inconsistent sparse-trie path upstream.
Automated nightly update of reth dependencies from
paradigmxyz/rethmain branch.Upstream reth changes
10aa6a5...b8f775a🔗 Amp thread: https://ampcode.com/threads/T-019fd796-8b31-77d0-980b-97e09781c583
Engine
RPC
BlockIdfor execution witnesses (#26569, #26572).eth_createAccessListenvironment preparation (#26596, #26599, #26598).Trie & Stages
wipedfield mutation fromHashedPostState(#26524).Storage & Providers
ConsistentDbView, movedOverlayStateProviderto use a provider reference, and dropped unusedAccountReaderbounds (#26581, #26611, #26591).Transaction Pool
Networking & Configuration
reth.toml(#26551).Testing
Bench
Migrations
🔗 Amp thread: https://ampcode.com/threads/T-019fd796-c9e4-708d-a4cc-27f676160f7c
10aa6a…tob8f775aand pinnedrevm-inspectorsto0.42.0for compatibility with the updated SDK.HashedStorage::from_iterto its parameterless API and now marks replacement storage as wiped by settingstorage.wiped = true.HashedPostStateProvider::hashed_post_stateimplementations and call sites to return and propagateProviderResult<HashedPostState>.RpcLogassociated type andconvert_loghook, preserving Tempo logs unchanged.u64toi64across precompile storage providers, dispatch, adapters, and tests to match the updated REVM gas tracker API.TempoEvm::initial_gas_and_reservoirafter upstream API changes and moved its legacy pre-T0 underflow handling intoHandler::tx_gas.InitialAndFloorGas::initial_gas_and_reservoirdirectly with the transaction gas limit and configured cap.EthPooledTransactionfield construction withEthPooledTransaction::new, then overrides the computed cost to accommodate upstream field encapsulation and initialization changes.GitHub Workflow