Conversation
Co-authored-by: phexyz <32248504+phexyz@users.noreply.github.com>
Co-authored-by: Spencer Solit <sol@seismic.systems>
…`evm_wtith_evn` (#76) Co-authored-by: SFYLL <sd@seismic.systems>
… validation Replace per-transaction client iteration with O(1) cache lookups. The cache is populated at startup and updated via on_new_head_block, which uses the same consensus-driven source as the RPC layer, fixing the intermittent "recent_block_hash not found" error caused by database lag.
…315) This PR adds the following: ## Adversarial fuzzing framework A proptest-based fuzzing framework (`reth-seismic-fuzz`) that stress-tests all Seismic-specific code paths for panics. Any panic in production code is treated as a security bug since it can crash the node. ### What's tested - **Precompile crash-freedom**: All 5 stateless seismic precompiles (AES-GCM encrypt/decrypt, ECDH, HKDF, secp256k1-sign) fuzzed with arbitrary bytes and gas limits, including boundary-length inputs near each precompile's expected input size. - **EVM execution crash-freedom**: Full `SeismicEvm.transact()` with arbitrary transactions across all tx types except EIP-4844 (Legacy, EIP-2930, EIP-1559, Seismic 0x4A) and contract deployments. - **Flagged storage crash-freedom**: Dynamically-constructed bytecode with SLOAD, CLOAD (0xB0), and CSTORE (0xB1) opcodes on arbitrary storage slots, exercising the privacy boundary enforcement between public and private storage. - **Differential correctness**: Same non-seismic transaction executed on both SeismicEvm and plain revm, comparing success/failure outcome, gas usage, output bytes, and error reasons. Any divergence is a regression. - **Transaction encoding roundtrips**: EIP-2718 and Compact codec encode/decode roundtrips, plus raw arbitrary bytes fed into decoders to verify crash-freedom on the P2P attack surface. ### Bug fix: `Decompress` panic on corrupt database entries The zstd decompressor in `ReusableDecompressor::decompress` (`zstd-compressors/src/lib.rs`) panics via `assert!` when it encounters malformed data (e.g. "Unknown frame descriptor", "Dictionary mismatch"). This panic is reachable through any DB read that touches a corrupt entry — including P2P request handlers serving data to remote peers. This is a known issue in upstream reth. A user reported it in [paradigmxyz/reth#16052](paradigmxyz/reth#16052): their node ran for 6 hours with a corrupt snapshot, surviving its own sync loop (which handled MDBX-level corruption gracefully), until a random peer requested data from the corrupt region. The P2P handler hit the zstd assert and the entire node crashed. **Fix**: The `Decompress` trait implementations in `db-api/src/models/mod.rs` (the bridge between `Compact::from_compact` and the DB read path) now wrap `from_compact` in `catch_unwind`. If the zstd decompressor (or any other `from_compact` code) panics, it's caught and converted to `DatabaseError::Decode`. The existing error handling at every layer above — staged sync pipeline, P2P request handler, RPC — handles `DatabaseError` gracefully. The node stays up. **Validation**: The `db_corruption.rs` integration test writes a valid transaction to MDBX, overwrites it with corrupt bytes (zstd flag set, garbage payload) via `RawTable`, then reads it back through the production code path (`tx.get::<Transactions>()` → `decode_one` → `Decompress::decompress`). It asserts that: 1. The read returns `Err(DatabaseError::Decode)` instead of panicking 2. The zstd decompressor panic actually fired internally (captured via `set_hook`)
This PR introduces EIP-4844 (blob transaction) coverage to the fuzz testing framework. ## What changed - Removed the EIP-4844 skip guards in `tx_encoding.rs` that were blocking roundtrip fuzz coverage. The `arbitrary(skip)` on the EIP-4844 variant in `seismic-alloy` has been removed upstream, so `proptest` now generates blob transactions naturally (~20% of cases). The "known encoding limitations" comment was stale — both EIP-2718 and `Compact` roundtrips pass for EIP-4844. - Added `tx_type = 3` (EIP-4844) to the `FuzzSeismicTx` generator in `tx_gen.rs`. The selector changed from `% 4` to `% 5`, and blob-specific fields (`blob_hashes`, `max_fee_per_blob_gas`) are populated when generating blob transactions. This means EVM execution, differential, and flagged storage fuzz tests now exercise EIP-4844 code paths. - Updated `into_eth_compatible_tx` to include EIP-4844 in differential testing (it's a standard Ethereum tx type). ## What's now fuzzed - `tx_roundtrip_2718` — EIP-4844 encode/decode roundtrip - `tx_roundtrip_compact` — EIP-4844 `Compact`/zstd roundtrip - `evm_transact_never_panics` — EVM execution with `tx_type=3` and blob fields - `differential_eth_vs_seismic_outcome` — `SeismicEvm` vs plain `revm` with blob txs
Just a small change where enclave uses tracing instead of println so that --quiet actually doesnt show the enclave logs. Only incorporates this one commit: SeismicSystems/enclave#173
Adds automated PR review via Claude Code GitHub Action. Reviews trigger on every PR open/push and on `@claude` mentions in PR comments. Scoped to read-only tools (gh pr diff/view/comment, git). Review focus includes Seismic-specific concerns: - Confidential tx (0x4a) encryption handling and plaintext leak prevention - Enclave/TEE purpose key safety - EVM execution and confidential state isolation - TxPool RecentBlockCache correctness - RPC extension safety - Clippy strictness (no unwrap/expect/indexing/panic/unreachable/todo) Requires `ANTHROPIC_API_KEY` secret to be set in repo Settings > Secrets and variables > Actions.
PR Review SummaryChangesThis PR ([DONT MERGE] Seismic => upstream diff) captures the full delta between the Blocking Issues1.
2. Hardcoded let rng_mode = RngMode::Execution; // TODO WARNING: chose a default valueThe comment itself flags this as a known incorrect default. 3. Debug log of full consensus tx (potentially including input) in payload builder debug!("default_seismic_payload: tx: {:?}", tx); // tx is full SeismicTransactionSignedMissing 4. Debug log of full recovered transaction (ciphertext + sender) in tracing::debug!(target: "reth-seismic-rpc::eth", ?recovered, "serving seismic_eth_api::send_raw_transaction");
tracing::debug!(target: "reth-seismic-rpc::eth", tx_hash=%recovered.tx_hash(), sender=%recovered.signer(), "serving seismic_eth_api::send_raw_transaction");5. Debug log of full tracing::debug!("reth-seismic-rpc::eth create_txn_env {:?}", env);Missing 6. Debug log of full tracing::debug!("from_recovered_tx: tx: {:?}", tx);
7. Mock enclave server gated only by runtime CLI flag, not compile-time feature The mock server and all #[cfg(any(test, feature = "mock-enclave"))]
if config.mock_server { ... }Emit a compile error if Suggestions
Positive Notes
|
…e-review (#321) ## Summary - Remove `synchronize` from PR triggers to break the push→review→new-issues→push loop; re-reviews now happen on-demand via `@claude` comments only - Add exhaustive initial review mode that catches all issues in a single pass and stamps the reviewed commit SHA - Add incremental re-review mode that diffs only new changes since the last review and tracks resolution of previously flagged issues - Use `fetch-depth: 0` for full git history and add `PR_NUMBER`/`IS_REREVIEW` env vars for cleaner prompt interpolation ## Test plan - [ ] Open a test PR to verify the initial review triggers on `opened` and produces a single exhaustive comment with the reviewed commit SHA - [ ] Push a follow-up commit and confirm no automatic re-review fires - [ ] Comment `@claude` on the PR and verify the re-review mode activates, diffs against the last reviewed SHA, and uses the re-review output format
All of the changes are purely syntactic. Just diffed the seismic branch with the main branch and removed any diff that was not necessary. Intention is to keep our diff surface smaller, to make rebasing against upstream easier and to make it easier when checking diff manually to only look at semantically relevant changes. The optimism crate diff is now down from 15+ files to 2 files: <img width="368" height="118" alt="image" src="https://github.com/user-attachments/assets/98ff5de1-f68b-4b41-bc21-bee36f8ac3ba" /> The remaining diff is because we forked the chainspec crate to use FlaggedStorage in our genesis files. Claude analyzed and said we can probably get rid of this by not forking and using a SeismicChainSpec under crates/seismic, but that would require more thought/analysis and a bigger diff, so keeping for a future PR.
Similar issue that we've been patching on all repos; see SeismicSystems/seismic-foundry-fork-db#29
Adds the consensus state root contract for EIP-4788 as a pre-deploy. This is essentially the same contract as Ethereum's beacon roots contract defined in EIP-4788. The only difference is that the TIMESTAMP (0x42) opcode was replaced with TIMESTAMP_MS (0x4b). The reason for this change is that the contract will store each block's consensus state merkle root using the block's timestamp as the key. Since Seismic has sub-second block times, there will be multiple blocks that return the same value for TIMESTAMP. Those blocks would overwrite each other's consensus state merkle root.
Small refactor to minimize diff with upstream. Think this was changed by mistake. new diff in this PR: ``` ~/devel/seismic-workspace/seismic-reth [refactor--minimize-chain-state-crate-diff-with-upstream] $ git diff main --stat crates/chain-state crates/chain-state/src/in_memory.rs | 6 ++++-- crates/chain-state/src/lib.rs | 2 +- crates/chain-state/src/memory_overlay.rs | 6 ++++-- 3 files changed, 9 insertions(+), 5 deletions(-) ``` Current seismic branch diff with main: ``` ~/devel/seismic-workspace/seismic-reth [seismic] $ git diff main --stat crates/chain-state crates/chain-state/Cargo.toml | 5 ++--- crates/chain-state/src/in_memory.rs | 6 ++++-- crates/chain-state/src/lib.rs | 4 +++- crates/chain-state/src/memory_overlay.rs | 6 ++++-- 4 files changed, 13 insertions(+), 8 deletions(-) ``` ## Claude Analysis ``` The diff has 3 related changes: 1. revm-state changed from optional to non-optional (line 30) 2. "revm-state?/serde" commented out in the serde feature (line 71) 3. "revm-state" removed from test-utils feature (line 79) Why it was done: In upstream reth, revm-state is an optional dependency only pulled in by the test-utils feature — it's used solely in test_utils.rs (use revm_state::AccountInfo). The ? syntax in "revm-state?/serde" means "enable serde on revm-state only if it's already enabled." On the seismic branch, someone added use revm_state as _; to lib.rs (not behind #[cfg(feature = "test-utils")]). This is a dummy import that exists only to suppress the unused_crate_dependencies lint (the #![cfg_attr(not(test), warn(unused_crate_dependencies))] at the top). But because this import is unconditional, it requires revm-state to always be present — hence making it non-optional. Once it's non-optional: - The ? conditional syntax in "revm-state?/serde" becomes pointless (it's always present), so it was commented out — though it should have been changed to "revm-state/serde" rather than commented out (this is likely a bug: serde won't propagate to revm-state now). - Listing "revm-state" in test-utils is redundant since it's always included, so it was removed. Was this necessary? No, this change appears unnecessary and introduces a minor bug. The cleaner approach would have been to either: 1. Not add the use revm_state as _; line at all (it's not needed since the crate genuinely uses it in test-utils), or 2. Gate the dummy import behind #[cfg(feature = "test-utils")] And the commented-out "revm-state?/serde" line should be "revm-state/serde" (without ?) to correctly propagate the serde feature. ```
…333) The alloy_compat module (TryFrom<AnyRpcTransaction> for SeismicTransactionSigned) was gated behind an alloy-compat feature that no crate in the workspace ever enabled — making it dead code. Remove the module, the feature flag, and its associated optional dependencies (alloy-rpc-types-eth, alloy-serde, num-traits). Seems like we copied over this code from op when creating our seismic crates. op defines similar feature in op-alloy-consensus, but its only used for reth-bench, and we haven't updated that. Claude also recommends that if we do want this conversion code it should live in seismic-alloy anyways. So feel safe deleting this code from reth. ## Claude analysis ``` The TryFrom<AnyRpcTransaction> conversion is for turning JSON-RPC transaction responses into reth-native types. The concrete use case would be something like: // Fetch a tx from a Seismic RPC node let rpc_tx: AnyRpcTransaction = provider.get_transaction_by_hash(hash).await?; // Convert to reth's internal type let signed_tx: SeismicTransactionSigned = rpc_tx.try_into()?; But nobody does this. In practice: - reth receives transactions either via p2p (already native types) or via eth_sendRawTransaction (raw RLP bytes decoded directly, not JSON-RPC format). The RPC→native conversion is never needed. - Foundry doesn't use reth-seismic-primitives at all — it has its own transaction types. - Upstream reth-primitives has alloy-compat = [] (empty!) — it's just a marker feature. The actual conversions live in op-alloy-consensus, not in reth itself. So to answer your question: no, you don't need this code right now. It's a speculative compatibility shim that nothing calls. The only scenario where it would matter is if some future tool needs to fetch Seismic transactions from an RPC endpoint and convert them to reth-internal types — but that tool doesn't exist yet, and if it did, the conversions would arguably belong in seismic-alloy-consensus (like upstream puts them in op-alloy-consensus), not in reth-seismic-primitives. ```
There were a few issues when test fails:
1. started reth binary not killed
2. nextest restarts the test 4 times, but all 3 extra times just fail
due to port conflict with non-cleaned leftover binary
3. also test would panic reporting failure
Changed nextest.toml config to only run this test once.
## More involved future improvements
I feel like the test harness with compiling at beginning of test (!) +
running a reth binary is overkill. Claude seems to agree with me:
```
What SeismicRethTestCommand does
It shells out to cargo run --bin seismic-reth at runtime (utils.rs:47), which means:
1. Recompiles reth from scratch inside the test — a full cargo run that re-invokes the compiler
2. Parses stdout waiting for "Starting consensus engine" to know it's ready
3. Communicates over HTTP RPC on a hardcoded port (127.0.0.1:8545) — no isolation, port conflicts possible
4. Uses mpsc channels + kill_on_drop for lifecycle management — brittle compared to in-process
What upstream reth does instead
Reth already has reth_e2e_test_utils which starts a full node in-process — no subprocess, no recompilation. The pattern from
crates/ethereum/node/tests/e2e/eth.rs:
let (mut nodes, _tasks, wallet) = setup::<EthereumNode>(
1, Arc::new(chain_spec), false, eth_payload_attributes,
).await?;
let mut node = nodes.pop().unwrap();
// inject tx, advance block, assert — all in-process
let raw_tx = TransactionTestContext::transfer_tx_bytes(1, wallet.inner).await;
let tx_hash = node.rpc.inject_tx(raw_tx).await?;
let payload = node.advance_block().await?;
This uses NodeBuilder::new(config).testing_node(executor) which creates an ephemeral TempDatabase, starts the node in the same process/tokio runtime,
and gives you direct programmatic access to all internals (pool, provider, network, RPC, payload builder).
Benefits of switching to in-process
- No double compilation — reth is compiled once as part of the test binary, not again at runtime via cargo run
- No port conflicts — RPC is in-process, no hardcoded 8545
- Better isolation — each test gets its own TempDatabase
- Direct access — you can call into node internals, not just HTTP RPC
- Proper lifecycle — no process management, stdout parsing, or kill_on_drop
So yes — the test should use setup::<SeismicNode>(...) (or equivalent) instead of SeismicRethTestCommand. The only reason this approach might have been
used is if SeismicNode didn't yet implement the traits needed by the NodeBuilder test infrastructure, but that would be the right thing to fix.
```
## Summary - Replaces the inline-prompt claude review workflow with a sticky-comment architecture that edits a single comment in-place on each push (no duplicate comments) - Moves the review prompt to `.claude/prompts/claude-pr-review.md`, tailored to seismic-reth (confidential txs, enclave/TEE, flagged storage, precompiles, hardforks, clippy strictness) - Adds `reopened`/`synchronize` triggers, dependabot + merge queue exclusions, 20min timeout ## Test plan - [ ] Open a test PR and verify the workflow triggers on `opened` - [ ] Push a follow-up commit and verify the sticky comment is updated in-place (not duplicated) - [ ] Comment `@claude` on a PR and verify the `issue_comment` trigger works - [ ] Verify dependabot PRs and draft PRs do not trigger the workflow 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Based on my examination of the key seismic files in this large comparison PR, I can provide a focused review of the most critical security issues I found in the current codebase. Large comparison PR between Seismic fork and upstream reth showing ~50k additions and ~5k deletions. Phase 1
Phase 2
Note: This appears to be a comparison PR showing differences between Seismic and upstream rather than actual changes for merge. The Debug derive issue is a pre-existing vulnerability that should be addressed regardless of this PR's purpose. |
## Summary - Removes stale `default-filter` from `.config/nextest.toml` that was excluding 8 tests from CI - All 8 tests pass locally — the filters were likely added during early development and never revisited ## Tests re-enabled **Static file truncation tests** (`reth-provider`): - `providers::static_file::tests::test_header_truncation` - `providers::static_file::tests::test_tx_based_truncation` **Fee history RPC tests** (`reth-rpc`): - `eth::core::tests::test_fee_history_empty` - `eth::core::tests::test_fee_history_all_blocks` - `eth::core::tests::test_fee_history_no_block_requested` - `eth::core::tests::test_fee_history_invalid_block_range_in_future` - `eth::core::tests::test_fee_history_invalid_block_range_before_genesis` - `eth::core::tests::test_fee_history_single_block` ## Test plan - [x] Ran `test_header_truncation` and `test_tx_based_truncation` locally — both pass - [x] Ran all 6 `eth::core::tests` locally — all pass - [x] CI passes with the filter removed
2 main changes from revm: 1. CLOAD is now allowed to read public storage (had to fix a test). See SeismicSystems/seismic-revm#180 - this is a pretty old PR now, its even on the seismic branch and not the audit branch. We were just pointing to a very old commit. 2. SeismicHaltReason no longer exists. Changed invalid public/private access to reverts instead of halts. See SeismicSystems/seismic-revm#210 for more details
… test (#335) ## Summary - Registers `EthConfigHandler` in the Seismic node's RPC add-ons, enabling the `eth_config` RPC method ([EIP-7910](https://eips.ethereum.org/EIPS/eip-7910)). This was already wired in the Ethereum node but missing from the Seismic node. - Adds `test_mercury_hardfork_config` E2E test that validates Mercury hardfork configuration is correctly reported via `eth_config`. ## Changes ### `crates/seismic/node/src/node.rs` - Register `EthConfigHandler` in `SeismicAddOns::launch_add_ons`, merged into the `Eth` RPC namespace — matches upstream `EthereumNode` pattern at `crates/ethereum/node/src/node.rs:304-316` - This is a non-breaking, additive change: adds `eth_config` alongside existing `eth_*` methods ### `crates/seismic/node/tests/e2e/hardfork_config.rs` (new) - Starts a `SeismicNode` (not `EthereumNode`) with `SEISMIC_DEV` chain spec - Initializes mock purpose keys via `std::sync::Once` (required for in-process Seismic node startup) - Calls `eth_config` RPC and asserts: - `activation_time == 0` — Mercury active at genesis - `chain_id` matches Seismic dev chain (5124) - `fork_id` matches locally computed fork hash from `chain_spec.fork_id()` — this is a CRC32 of the genesis hash XOR'd with all active fork timestamps, proving the full Seismic fork schedule and genesis state (including system contracts) is correct - No next fork scheduled - Note: `precompiles` field is not asserted because `EthConfigHandler` currently returns an empty map (upstream TODO at `crates/rpc/rpc-eth-api/src/helpers/config.rs:103`) ### `crates/seismic/node/tests/e2e/main.rs` - Adds `mod hardfork_config` and clippy allows for test code ### `crates/ethereum/primitives/src/receipt.rs` - Import reorder to match CI nightly rustfmt ## Test plan - [x] `cargo nextest run -p reth-seismic-node -E 'test(mercury_hardfork_config)'` passes locally - [x] `cargo clippy -p reth-seismic-node --tests --no-deps` clean - [x] `cargo +nightly fmt --all` clean - [ ] CI passes
## Summary Re-enables the commented-out `mod testsuite` in seismic E2E tests and fixes compilation errors that accumulated since it was disabled. ## Changes **`crates/seismic/node/tests/e2e/main.rs`**: - Uncomments `mod testsuite;` - Adds `clippy::unwrap_used`, `clippy::expect_used` allows (standard for test files) **`crates/seismic/node/tests/e2e/testsuite.rs`**: - Fix import paths: `SeismicEngineTypes`/`SeismicNode` moved to `engine`/`node` submodules - Use `SEISMIC_MAINNET.clone()` directly instead of `ChainSpecBuilder` + missing genesis.json asset - Flatten `PayloadAttributes` to match current `SeismicEngineTypes` (was using old nested Optimism-style wrapper) - Set post-Cancun required fields: `withdrawals: Some(vec![])`, `parent_beacon_block_root: Some(B256::ZERO)` - Rename `test_testsuite_op_assert_mine_block` → `test_testsuite_seismic_assert_mine_block` - Change expected hash from `Some(B256::ZERO)` → `None` (block hash can't be predicted) - Extract `SEISMIC_TIMESTAMP_MULTIPLIER` constant to replace magic number 1000 ## Test plan - [x] `cargo clippy -p reth-seismic-node --tests --no-deps` passes - [x] `cargo +nightly fmt --all` clean - [x] CI passes 🤖 Generated with [Claude Code](https://claude.com/claude-code)
## Summary - Re-enable the `mod p2p` E2E test module that was previously commented out - Fix `utils::setup()` to use `SEISMIC_DEV` chain spec instead of a vanilla Ethereum Cancun spec, so p2p tests actually run with Seismic fork rules - Deduplicate test helpers: move `ensure_mock_purpose_keys()`, `SEISMIC_TIMESTAMP_MULTIPLIER`, and `seismic_payload_attributes()` into shared `utils.rs` - Fix `seismic_payload_attributes()` to apply the millisecond timestamp multiplier - Remove unused `seismic-alloy-genesis` dependency from test-utils feature ## Test plan - [x] `cargo +nightly fmt --all` clean - [x] `cargo clippy -p reth-seismic-node --tests --no-deps` passes (seismic-strict) - [ ] CI passes 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…346) ## Summary - Add known Seismic antipatterns (wrong chain spec, missing timestamp multiplier, wrong node type) to the Claude PR review prompt as Phase 1 critical issues - Add a dedicated "Known Antipatterns" section to the review guidelines for patterns that are always bugs in Seismic code - Grant the Claude review bot `Read` and `Grep` tools so it can follow imports and verify semantic correctness when reviewing changes to `crates/seismic/` ## Test plan - [ ] Verify the Claude PR review workflow triggers correctly on a test PR - [ ] Confirm the bot can use Read/Grep to inspect files beyond the diff 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
## Summary - Replace the commented-out `cargo build` step with `cargo check --workspace` for fast (~30s) workspace-wide compilation validation - Provides early feedback before slower test jobs finish - Future improvement: upgrade to `cargo hack check --workspace` (like upstream reth) once the pre-existing `SerdeBincodeCompat` trait bound issue in `reth-ethereum-primitives` is fixed ## Test plan - [ ] Verify the new `cargo check --workspace` step passes in CI 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…recovered_tx (#349) Same fix as that from SeismicSystems/seismic-evm#42 Not sure if there was an issue here, but its just unnecessary duplicate code that is likely to contain a bug like it did in evm.
No description provided.