From 752326efad2703af56b552aba833db8c1b5c5848 Mon Sep 17 00:00:00 2001 From: Richard Date: Tue, 24 Feb 2026 12:34:10 +0800 Subject: [PATCH 1/4] ci: add Claude Code security review workflow --- .github/workflows/claude-security-review.yml | 30 ++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 .github/workflows/claude-security-review.yml diff --git a/.github/workflows/claude-security-review.yml b/.github/workflows/claude-security-review.yml new file mode 100644 index 0000000000..ba176685e5 --- /dev/null +++ b/.github/workflows/claude-security-review.yml @@ -0,0 +1,30 @@ +name: Claude Security Review + +on: + pull_request: + types: [opened, synchronize, reopened] + +permissions: + contents: read + pull-requests: write + +jobs: + security-review: + # Only run after maintainer approval for external PRs + if: github.event.pull_request.draft == false + runs-on: ubuntu-latest + timeout-minutes: 30 + + steps: + - name: Checkout repository + uses: actions/checkout@v4 + with: + fetch-depth: 2 + + - name: Run Claude Security Review + uses: anthropics/claude-code-security-review@main + with: + claude-api-key: ${{ secrets.CLAUDE_API_KEY }} + claude-model: claude-sonnet-4-6 + claudecode-timeout: 25 + exclude-directories: "docs,tests,benches,.github" From 297d3c2adddb7c39066aea8d866b92233f28c2e5 Mon Sep 17 00:00:00 2001 From: Richard Date: Tue, 24 Feb 2026 12:53:16 +0800 Subject: [PATCH 2/4] ci: add workflow_dispatch to security review --- .github/workflows/claude-security-review.yml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/claude-security-review.yml b/.github/workflows/claude-security-review.yml index ba176685e5..01e61fb137 100644 --- a/.github/workflows/claude-security-review.yml +++ b/.github/workflows/claude-security-review.yml @@ -3,6 +3,7 @@ name: Claude Security Review on: pull_request: types: [opened, synchronize, reopened] + workflow_dispatch: permissions: contents: read @@ -10,8 +11,7 @@ permissions: jobs: security-review: - # Only run after maintainer approval for external PRs - if: github.event.pull_request.draft == false + if: github.event.pull_request.draft == false || github.event_name == 'workflow_dispatch' runs-on: ubuntu-latest timeout-minutes: 30 @@ -19,7 +19,7 @@ jobs: - name: Checkout repository uses: actions/checkout@v4 with: - fetch-depth: 2 + fetch-depth: 0 - name: Run Claude Security Review uses: anthropics/claude-code-security-review@main @@ -27,4 +27,5 @@ jobs: claude-api-key: ${{ secrets.CLAUDE_API_KEY }} claude-model: claude-sonnet-4-6 claudecode-timeout: 25 + run-every-commit: true exclude-directories: "docs,tests,benches,.github" From f5bf5bab6db517edd801c30e246aed73c4a79851 Mon Sep 17 00:00:00 2001 From: richard-zz-galxe Date: Thu, 5 Mar 2026 13:18:17 +0800 Subject: [PATCH 3/4] ci: upgrade security review model to claude-opus-4-6 Co-Authored-By: Claude Opus 4.6 --- .github/workflows/claude-security-review.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/claude-security-review.yml b/.github/workflows/claude-security-review.yml index 01e61fb137..aafdfe9fd0 100644 --- a/.github/workflows/claude-security-review.yml +++ b/.github/workflows/claude-security-review.yml @@ -25,7 +25,7 @@ jobs: uses: anthropics/claude-code-security-review@main with: claude-api-key: ${{ secrets.CLAUDE_API_KEY }} - claude-model: claude-sonnet-4-6 + claude-model: claude-opus-4-6 claudecode-timeout: 25 run-every-commit: true exclude-directories: "docs,tests,benches,.github" From 7fc6959f034db18afb9e18057ecb7b3bea65fc9f Mon Sep 17 00:00:00 2001 From: richard-zz-galxe Date: Wed, 18 Mar 2026 17:44:08 +0800 Subject: [PATCH 4/4] chore: add design intent comments and TODO markers from audit findings Annotate 22 source files with Design Intent and TODO comments covering all GRETH audit findings (GRETH-008 through GRETH-075). Design Intent comments explain intentional behavior to prevent re-auditing. TODO markers flag genuine issues for future remediation. --- crates/engine/tree/src/persistence.rs | 10 ++++ crates/engine/tree/src/recovery.rs | 8 ++++ crates/engine/tree/src/tree/mod.rs | 8 +++- crates/ethereum/evm/src/parallel_execute.rs | 7 +++ .../src/block_view_storage/mod.rs | 16 +++++++ .../event-bus/src/lib.rs | 11 +++++ .../execute/src/bls_precompile.rs | 3 ++ .../execute/src/channel.rs | 15 +++--- .../pipe-exec-layer-ext-v2/execute/src/lib.rs | 46 ++++++++++++++++++- .../execute/src/mint_precompile.rs | 11 +++++ .../execute/src/onchain_config/dkg.rs | 9 ++-- .../execute/src/onchain_config/epoch.rs | 5 ++ .../execute/src/onchain_config/jwk_oracle.rs | 4 ++ .../src/onchain_config/metadata_txn.rs | 12 +++++ .../execute/src/onchain_config/mod.rs | 7 +++ .../src/onchain_config/oracle_state.rs | 3 ++ .../src/onchain_config/validator_set.rs | 1 + .../relayer/src/blockchain_source.rs | 10 ++++ .../relayer/src/eth_client.rs | 5 ++ .../relayer/src/persistence.rs | 4 ++ .../relayer/src/uri_parser.rs | 3 ++ .../src/providers/static_file/manager.rs | 7 +++ crates/storage/storage-api/src/cache.rs | 9 ++++ 23 files changed, 203 insertions(+), 11 deletions(-) diff --git a/crates/engine/tree/src/persistence.rs b/crates/engine/tree/src/persistence.rs index e0475f6194..a0ba219360 100644 --- a/crates/engine/tree/src/persistence.rs +++ b/crates/engine/tree/src/persistence.rs @@ -222,6 +222,11 @@ where // and storage_db internally. For fault tolerance, stage checkpoints ensure // idempotency - each stage's checkpoint is verified before writing, guaranteeing // exactly-once execution even if the process crashes mid-block. + // Design Intent (GRETH-069): State and trie writes happen in separate + // threads with separate commits. If trie commits before state, another + // thread may transiently observe new trie nodes but old state data. This + // is acceptable because the cache overlay always provides consistent data + // for merklization, and readers only fall back to DB on cache miss. thread::scope(|scope| -> Result<(), PersistenceError> { let state_handle = scope.spawn(|| -> Result<(), PersistenceError> { let start = Instant::now(); @@ -281,6 +286,11 @@ where ) .record(start.elapsed()); + // Design Intent (GRETH-072): Validator nodes skip history index + // writes because they are not RPC servers and do not serve + // historical queries. Note: historical RPC queries against a + // validator node may return empty/incorrect results without + // explicit error, since no guard rejects them. if !get_gravity_config().validator_node_only { let start = Instant::now(); let provider_rw = inner_provider.database_provider_rw()?; diff --git a/crates/engine/tree/src/recovery.rs b/crates/engine/tree/src/recovery.rs index 23df5fb466..95b7bed0cb 100644 --- a/crates/engine/tree/src/recovery.rs +++ b/crates/engine/tree/src/recovery.rs @@ -111,6 +111,14 @@ impl<'a, N: ProviderNodeTypes> StorageRecoveryHelper<'a, N> { /// - History indices correctly track all state changes /// - All checkpoints are synchronized at `recover_block_number` pub fn check_and_recover(&self) -> ProviderResult<()> { + // Design Intent (GRETH-008): Recovery trusts the canonical tip state root + // without re-verification. This is acceptable because the only way a corrupted + // checkpoint can exist is via hardware failure or external data corruption, + // which would require manual intervention regardless. + // TODO(GRETH-070): Recovery uses StageId::Execution checkpoint as ground truth. + // If this checkpoint is corrupted (e.g., by partial write or disk corruption), + // recovery operates at the wrong block number. Consider adding a cross-check + // against block headers stored in static files. let provider_ro = self.factory.database_provider_ro()?; let recover_block_number = provider_ro.recover_block_number()?; let best_block_number = provider_ro.best_block_number()?; diff --git a/crates/engine/tree/src/tree/mod.rs b/crates/engine/tree/src/tree/mod.rs index 410771ec58..1720ec4167 100644 --- a/crates/engine/tree/src/tree/mod.rs +++ b/crates/engine/tree/src/tree/mod.rs @@ -538,13 +538,19 @@ where ForkchoiceStatus::Valid, ); + // TODO(GRETH-068): Panic on make_canonical failure. Any error (including + // transient RocksDB I/O errors) causes immediate node crash with no retry + // mechanism. Consider adding retry logic for transient errors. self.make_canonical(block_hash).unwrap_or_else(|err| { panic!( "Failed to make canonical, block_number={block_number} block_hash={block_hash}: {err}", ) }); - // deterministic consensus means canonical block is immediately safe and finalized + // Design Intent (GRETH-009): In Gravity's BFT consensus model, every block + // that passes consensus is immediately considered safe and finalized. There is + // no concept of probabilistic finality or confirmation depth. This is by design + // and matches the deterministic consensus guarantees. self.canonical_in_memory_state.set_safe(sealed_header.clone()); self.canonical_in_memory_state.set_finalized(sealed_header); } diff --git a/crates/ethereum/evm/src/parallel_execute.rs b/crates/ethereum/evm/src/parallel_execute.rs index 23232bf3e9..955275e520 100644 --- a/crates/ethereum/evm/src/parallel_execute.rs +++ b/crates/ethereum/evm/src/parallel_execute.rs @@ -78,6 +78,13 @@ where let state_clear_flag = self.chain_spec.is_spurious_dragon_active_at_block(block.number); let state = self.state.as_mut().unwrap(); state.set_state_clear_flag(state_clear_flag); + // Design Intent (GRETH-058-old / WrapDatabaseRef state boundary): + // WrapDatabaseRef provides a read-only database interface to the EVM, but + // apply_pre_execution_changes() operates through system call hooks that write + // directly to the underlying ParallelState (e.g., beacon root contract calls). + // The state changes are not lost — they are committed via the ParallelState + // reference that WrapDatabaseRef wraps. This is consistent with upstream Reth's + // execution model where pre-execution changes are applied in-place. let mut evm = self.evm_config.evm_for_block(WrapDatabaseRef(state), block.header()).map_err(|e| { BlockExecutionError::Internal(InternalBlockExecutionError::Other(Box::new(e))) diff --git a/crates/gravity-storage/src/block_view_storage/mod.rs b/crates/gravity-storage/src/block_view_storage/mod.rs index ccfbbcbc19..a4252aebe9 100644 --- a/crates/gravity-storage/src/block_view_storage/mod.rs +++ b/crates/gravity-storage/src/block_view_storage/mod.rs @@ -56,6 +56,12 @@ where } fn state_root(&self, hashed_state: &HashedPostState) -> ProviderResult<(B256, TrieUpdatesV2)> { + // Design Intent (GRETH-034, GRETH-035): The DB transaction here reflects the + // persist frontier (height P), while the cache overlay contains data up to the + // current execution height M > P. In normal operation, the cache always provides + // the most recent data. DB fallback only occurs on cache misses, which are rare + // for recently-touched accounts. The three-database sharding (state/account/storage) + // also lacks cross-DB snapshot coordination, but the cache overlay dominates reads. let tx = self.client.database_provider_ro()?.into_tx(); let nested_hash = NestedStateRoot::new(&tx, Some(self.cache.clone())); nested_hash.calculate(hashed_state) @@ -147,6 +153,11 @@ impl DatabaseRef for RawBlockViewProvider { .unwrap_or_default()) } + // TODO(GRETH-031): BLOCKHASH opcode (0x40) is unimplemented. Any user transaction + // calling BLOCKHASH triggers a panic, which — combined with GRETH-029 (no barrier + // timeout) — causes permanent pipeline deadlock. Implement using the + // block_number_to_id BTreeMap maintained by BlockViewStorage::update_canonical(), + // which already stores the last BLOCK_HASH_HISTORY block hashes. fn block_hash_ref(&self, _number: u64) -> Result { unimplemented!("not support block_hash_ref in BlockViewProvider") } @@ -207,6 +218,11 @@ impl DatabaseRef for BlockViewProvider { self.db.storage_ref(address, index) } + // TODO(GRETH-031): BLOCKHASH opcode (0x40) is unimplemented. Any user transaction + // calling BLOCKHASH triggers a panic, which — combined with GRETH-029 (no barrier + // timeout) — causes permanent pipeline deadlock. Implement using the + // block_number_to_id BTreeMap maintained by BlockViewStorage::update_canonical(), + // which already stores the last BLOCK_HASH_HISTORY block hashes. fn block_hash_ref(&self, _number: u64) -> Result { unimplemented!("not support block_hash_ref in BlockViewProvider") } diff --git a/crates/pipe-exec-layer-ext-v2/event-bus/src/lib.rs b/crates/pipe-exec-layer-ext-v2/event-bus/src/lib.rs index ebe2a7518d..eaef00fc64 100644 --- a/crates/pipe-exec-layer-ext-v2/event-bus/src/lib.rs +++ b/crates/pipe-exec-layer-ext-v2/event-bus/src/lib.rs @@ -9,10 +9,21 @@ use tokio::sync::{mpsc::UnboundedReceiver, oneshot}; use tracing::info; /// A static instance of `PipeExecLayerEventBus` used for dispatching events. +// Design Intent (GRETH-037): The type-erased Box is used because Rust's +// static globals cannot be generic. Type safety is enforced at node startup: the +// generic parameter N is consistent across the single binary entry point. A +// mismatched downcast would panic at initialization, before any blocks are processed. pub static PIPE_EXEC_LAYER_EVENT_BUS: OnceCell> = OnceCell::new(); /// Get a reference to the global `PipeExecLayerEventBus` instance. pub fn get_pipe_exec_layer_event_bus() -> &'static PipeExecLayerEventBus { + // Design Intent (GRETH-038): The busy-wait loop with thread::sleep is used + // because this is called during node startup, before the async runtime is fully + // initialized. The event bus is always set during normal startup sequence, so this + // loop terminates quickly (typically 0-2 iterations). In abnormal conditions + // (initialization bug), the loop serves as a diagnostic via the log message. + // TODO: Consider adding a maximum iteration count to avoid hanging in truly broken + // deployments where the event bus is never initialized. let mut wait_time = 0; loop { let event_bus = PIPE_EXEC_LAYER_EVENT_BUS diff --git a/crates/pipe-exec-layer-ext-v2/execute/src/bls_precompile.rs b/crates/pipe-exec-layer-ext-v2/execute/src/bls_precompile.rs index 1238ce6ff7..c1ff49dfac 100644 --- a/crates/pipe-exec-layer-ext-v2/execute/src/bls_precompile.rs +++ b/crates/pipe-exec-layer-ext-v2/execute/src/bls_precompile.rs @@ -19,6 +19,9 @@ const BLS_POP_LEN: usize = 96; const EXPECTED_INPUT_LEN: usize = BLS_PUBKEY_LEN + BLS_POP_LEN; /// Gas cost for PoP verification (2 bilinear pairings + hash-to-curve) +// TODO(GRETH-065): POP_VERIFY_GAS = 45,000 is ~23% of EIP-2537 equivalent +// (~193,000 gas). Benchmark actual CPU cost on target hardware and adjust +// to prevent computational DoS via repeated PoP verification calls. const POP_VERIFY_GAS: u64 = 45_000; /// Domain separation tag for BLS PoP verification diff --git a/crates/pipe-exec-layer-ext-v2/execute/src/channel.rs b/crates/pipe-exec-layer-ext-v2/execute/src/channel.rs index 56000cd1b4..4d15762a98 100644 --- a/crates/pipe-exec-layer-ext-v2/execute/src/channel.rs +++ b/crates/pipe-exec-layer-ext-v2/execute/src/channel.rs @@ -52,8 +52,10 @@ impl Channel { } async fn wait_inner(&self, key: K, timeout: Option) -> Option { - // ATTN: We can guarantee that `.await` will not occur within the critical zone, which means - // `MutexGuard` will not be sent across threads. + // Design Intent (GRETH-047-old): The SendMutexGuard uses `unsafe impl Send`. + // Safety: We guarantee the MutexGuard is dropped before any `.await` point. The + // guard is only held during synchronous HashMap operations and released via + // `drop(inner)` on line 75, before the `rx.await` on line 80/94. struct SendMutexGuard<'a, T>(MutexGuard<'a, T>); unsafe impl<'a, T> Send for SendMutexGuard<'a, T> {} @@ -79,10 +81,11 @@ impl Channel { match tokio::time::timeout(duration, rx).await { Ok(result) => result.ok(), Err(_) => { - // Timeout occurred, clean up the waiting state only if still - // waiting. If the state is - // Notified, we should not remove it to avoid losing - // the notify signal. + // Timeout occurred. Clean up waiting state only if still waiting. + // If a notify arrived between timeout and lock acquisition + // (State::Notified), we must NOT remove it to avoid losing the + // signal. These orphaned Notified entries are bounded by the + // pipeline concurrency window and naturally consumed by retry. let mut inner = self.inner.lock().unwrap(); if matches!(inner.states.get(&key), Some(State::Waiting(_))) { inner.states.remove(&key); diff --git a/crates/pipe-exec-layer-ext-v2/execute/src/lib.rs b/crates/pipe-exec-layer-ext-v2/execute/src/lib.rs index 2d80294f08..539fb5da58 100644 --- a/crates/pipe-exec-layer-ext-v2/execute/src/lib.rs +++ b/crates/pipe-exec-layer-ext-v2/execute/src/lib.rs @@ -230,11 +230,22 @@ struct Core { custom_precompiles: Arc>, event_tx: std::sync::mpsc::Sender>, execute_block_barrier: Channel<(u64, u64) /* epoch, block number */, ExecuteBlockContext>, + // TODO(GRETH-029): merklize/seal/make_canonical barriers lack timeouts. If a process() + // task panics, downstream barriers wait forever. Add wait_timeout to all barriers and + // implement a circuit breaker for graceful shutdown on task failure. merklize_barrier: Channel, seal_barrier: Channel, make_canonical_barrier: Channel, + // TODO(GRETH-036): All inter-layer channels are unbounded. Under sustained load or + // adversarial block conditions, blocks accumulate without bound. Replace with + // bounded_channel(32-64) to provide backpressure from execution to consensus. discard_txs_tx: UnboundedSender>, cache: PersistBlockCache, + // Design Intent (GRETH-057): `epoch` and `execute_height` are two independent AtomicU64 + // values. Cross-variable ordering is guaranteed by the `execute_block_barrier` Channel + // (backed by Mutex), which serializes block processing: block N cannot proceed + // past wait((epoch, N-1)) until block N-1 completes. Release/Acquire ordering on each + // individual atomic is sufficient; SeqCst is unnecessary. epoch: AtomicU64, execute_height: AtomicU64, metrics: PipeExecLayerMetrics, @@ -266,6 +277,11 @@ impl PipeExecService { ); let core = self.core.clone(); + // TODO(GRETH-044): JoinHandle is discarded — panics in process() are silently + // swallowed by the tokio runtime. Monitor the JoinHandle to detect and propagate + // task failures, triggering graceful shutdown instead of silent deadlock. + // TODO(GRETH-045): No concurrency limit on spawned process() tasks. Each waiting + // task holds its ReceivedBlock in memory. Add a semaphore or task queue. tokio::spawn(async move { let start_time = Instant::now(); core.process(block).await; @@ -398,7 +414,11 @@ impl Core { } self.storage.insert_block_id(block_number, block_id); - // Wait for persist gap with a reasonable timeout (2 seconds) + // Design Intent (GRETH-038-old / wait_persist_gap timeout): + // The 2000ms timeout is intentional. We deliberately allow execution to proceed + // when persistence lags, preventing consensus halts. The resulting cache growth + // is managed by the eviction daemon (see cache.rs). Blocking indefinitely here + // would halt consensus whenever disk I/O is temporarily slow. self.cache.wait_persist_gap(Some(2000)); let start_time = Instant::now(); let ExecuteOrderedBlockResult { @@ -530,7 +550,13 @@ impl Core { let executed_block_hash = execution_result.block_hash; self.execution_result_tx.send(execution_result).ok()?; let block_hash = self.verified_block_hash_rx.wait(block_id).await?; + // TODO(GRETH-052): When block_hash is None, verification is skipped entirely. + // The None path exists for genesis/bootstrap but has no guard preventing it for + // post-genesis blocks. Add an explicit check that None is only valid at genesis. if let Some(block_hash) = block_hash { + // Design Intent (GRETH-053): Deliberate panic on hash mismatch. This is the + // correct behavior for consensus safety — a mismatch means non-deterministic + // execution, which must halt the node rather than silently diverge. assert_eq!(executed_block_hash, block_hash); } let elapsed = start_time.elapsed(); @@ -752,6 +778,10 @@ impl Core { // Merge state changes into accumulated changes for (addr, account) in validator_state_changes { + // TODO(GRETH-051): `HashMap::insert` replaces the entire account entry. + // If a precompile modifies an account that is also touched by a system + // transaction, the updates from the system transaction are lost. + // Replace with deep-merge semantics (merge storage maps, take latest AccountInfo). accumulated_state_changes.insert(addr, account); } @@ -803,6 +833,9 @@ impl Core { for (address, account) in precompile_bundle.state { if let Some(info) = account.info { use revm::state::{Account, AccountStatus, EvmStorageSlot}; + // TODO(GRETH-051): Same as above, replacing `insert` with deep-merge + // to avoid losing precompile state updates if the account was already + // modified by a metadata or validator transaction. accumulated_state_changes.insert( address, Account { @@ -1213,6 +1246,12 @@ fn filter_invalid_txs( sender_idx.entry(sender).or_default().push(i); } + // Design Intent (GRETH-074 / GRETH-016): This is a performance-only pre-filter, + // NOT a security boundary. It uses effective_gas_price (which is <= max_fee_per_gas) + // making the filter less strict than EVM validation. This is intentional: the filter + // may produce false negatives (letting some insufficient-balance txns through) but + // the EVM will catch them during actual execution. The purpose is to reduce wasted + // parallel EVM work on obviously-invalid transactions. let is_tx_valid = |tx: &TransactionSigned, sender: &Address, account: &mut AccountInfo| { if account.nonce != tx.nonce() { warn!(target: "filter_invalid_txs", @@ -1329,6 +1368,9 @@ impl PipeExecLayerApi { /// Wait for the block with the given block number to be persisted in the storage. /// Returns `None` if the channel has been closed. + // TODO(GRETH-054): rx.await has no timeout. If persistence never completes (disk + // full, I/O error), the commit loop stalls permanently and consensus hangs. Add a + // timeout and trigger graceful shutdown on expiry. pub async fn wait_for_block_persistence(&self, block_number: u64) -> Option<()> { let (tx, rx) = oneshot::channel(); self.event_tx @@ -1366,6 +1408,8 @@ where EthApi: EthCall, EthApi::NetworkTypes: RpcTypes, { + // TODO(GRETH-036): All channels below are unbounded. Replace with bounded channels + // (capacity 32-64) to provide backpressure from execution to consensus layer. let (ordered_block_tx, ordered_block_rx) = tokio::sync::mpsc::unbounded_channel(); let (execution_result_tx, execution_result_rx) = tokio::sync::mpsc::unbounded_channel(); let verified_block_hash_ch = Arc::new(Channel::new()); diff --git a/crates/pipe-exec-layer-ext-v2/execute/src/mint_precompile.rs b/crates/pipe-exec-layer-ext-v2/execute/src/mint_precompile.rs index 3b8c734493..991265e9ec 100644 --- a/crates/pipe-exec-layer-ext-v2/execute/src/mint_precompile.rs +++ b/crates/pipe-exec-layer-ext-v2/execute/src/mint_precompile.rs @@ -23,6 +23,9 @@ pub const AUTHORIZED_CALLER: Address = address!("0x595475934ed7d9faa7fca28341c2c const FUNC_MINT: u8 = 0x01; /// Base gas cost for mint operation +// TODO(GRETH-066): MINT_BASE_GAS = 21,000 matches a simple transfer but this +// precompile performs additional work (mutex lock, state DB load, balance +// modification). Benchmark actual CPU cost and adjust upward to prevent DoS. const MINT_BASE_GAS: u64 = 21000; /// Creates a mint token precompile contract instance with state access. @@ -74,6 +77,11 @@ fn mint_token_handler( state: Arc>>, ) -> PrecompileResult { // 1. Validate caller address + // Design Intent (GRETH-039-old / DELEGATECALL semantics): + // `input.caller` preserves the original `msg.sender` across DELEGATECALL. This is + // safe because AUTHORIZED_CALLER is a protocol-level pre-deploy (JWK Manager) that + // does not contain untrusted DELEGATECALL logic. An attacker cannot relay calls + // through it to impersonate the authorized caller. if input.caller != AUTHORIZED_CALLER { warn!( target: "evm::precompile::mint_token", @@ -130,6 +138,9 @@ fn mint_token_handler( warn!(target: "evm::precompile::mint_token", ?recipient, "Zero amount"); return Err(PrecompileError::Other("Zero amount not allowed".into())); } + // TODO(GRETH-049): No cumulative total supply cap enforced. Individual mints are + // capped at u128::MAX but there is no global supply limit. Consider adding a + // configurable maximum total supply and rejecting mints that would exceed it. // 6. Execute mint operation let mut state_guard = state.lock(); diff --git a/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/dkg.rs b/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/dkg.rs index 920fa62c6e..7fbeafc4c1 100644 --- a/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/dkg.rs +++ b/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/dkg.rs @@ -130,7 +130,7 @@ where }) .ok()?; - // Decode the Solidity DKG state + // Design Intent (GRETH-060): Same as epoch.rs — panic on ABI decode failure. let solidity_dkg_state = getDKGStateCall::abi_decode_returns(&result) .expect("Failed to decode getDKGState return value"); Some(convert_dkg_state_to_bcs(&solidity_dkg_state)) @@ -182,7 +182,7 @@ where }) .ok()?; - // Decode the Solidity RandomnessConfig + // Design Intent (GRETH-060): Same as epoch.rs — panic on ABI decode failure. let solidity_config = getCurrentConfigCall::abi_decode_returns(&result) .expect("Failed to decode getCurrentConfig return value"); Some(convert_randomness_config_to_bcs(&solidity_config)) @@ -261,7 +261,10 @@ fn convert_validator( gravity_api_types::on_chain_config::dkg::ValidatorConsensusInfo { addr: gravity_api_types::account::ExternalAccountAddress::new(addr_bytes), pk_bytes: validator.consensusPubkey.to_vec(), - // Convert wei to tokens by dividing by 10^18 + // Design Intent (GRETH-064): Truncating sub-ether voting power is intentional. + // The protocol requires validators to stake whole tokens; wei fractions are + // ignored. unwrap_or(u64::MAX) handles the theoretical edge case where total + // stake exceeds u64::MAX ethers (~18B tokens), which is unreachable in practice. voting_power: (validator.votingPower / alloy_primitives::U256::from(10).pow(alloy_primitives::U256::from(18))) .try_into() diff --git a/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/epoch.rs b/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/epoch.rs index 43ccbe617f..1c0e75d9c5 100644 --- a/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/epoch.rs +++ b/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/epoch.rs @@ -61,6 +61,11 @@ where }) .ok()?; + // Design Intent (GRETH-060): expect() is deliberate here. The execution + // layer is strictly coupled to protocol contracts. ABI decode failure + // indicates a catastrophic state desync (wrong contract ABI, corrupted + // state, or incompatible deployment). Panicking and halting the node is + // safer than proceeding with potentially wrong epoch data. let epoch = Reconfiguration::currentEpochCall::abi_decode_returns(&result) .expect("Failed to decode currentEpoch return value"); diff --git a/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/jwk_oracle.rs b/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/jwk_oracle.rs index 59bfd15fd3..63246c46a4 100644 --- a/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/jwk_oracle.rs +++ b/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/jwk_oracle.rs @@ -59,6 +59,10 @@ fn is_rsa_jwk(jwk: &JWKStruct) -> bool { /// Check if a JWKStruct is an UnsupportedJWK (blockchain/other oracle data) /// Checks for sourceType string (0, 1, 2, etc.) instead of fixed type_name +// Design Intent (GRETH-059): Using parse::() as a type discriminator is +// intentional. The consensus layer assigns numeric strings as type_name for +// oracle data sources. All known source types are enumerated. Unknown string +// types are rejected in the else branch of construct_oracle_record_transaction(). fn is_unsupported_jwk(jwk: &JWKStruct) -> bool { // Check if type_name is a numeric string (sourceType) // TODO(gravity): check if it should be "0x1::jwks::UNSUPPORTED_JWK" diff --git a/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/metadata_txn.rs b/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/metadata_txn.rs index 463cd72793..a36769b96d 100644 --- a/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/metadata_txn.rs +++ b/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/metadata_txn.rs @@ -206,12 +206,20 @@ pub fn transact_system_txn( } /// Create a new system call transaction +// TODO(GRETH-041): This function is duplicated in mod.rs. Consolidate to a +// single definition (preferably in mod.rs) and have this module use +// `super::new_system_call_txn()` to avoid divergence during future refactors. fn new_system_call_txn( contract: Address, nonce: u64, gas_price: u128, input: Bytes, ) -> TransactionSigned { + // Design Intent (GRETH-040): Zero-signature system transactions are + // intentional. Security relies on the on-chain SystemAccessControl + // modifier in protocol contracts, not on transaction-level signatures. + // The SYSTEM_CALLER address is a protocol constant that can only + // originate transactions through this code path. TransactionSigned::new_unhashed( Transaction::Legacy(TxLegacy { chain_id: None, @@ -247,6 +255,10 @@ pub fn construct_metadata_txn( let call = onBlockStartCall { proposerIndex: proposer_idx, + // TODO(GRETH-042): failedProposerIndices is always empty. If + // Blocker.sol uses this for slashing or reward redistribution, the + // functionality is completely non-operative. Implement proposer + // tracking to populate this field from consensus layer data. failedProposerIndices: vec![], timestampMicros: timestamp_us, }; diff --git a/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/mod.rs b/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/mod.rs index 70a487beac..203e6351f5 100644 --- a/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/mod.rs +++ b/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/mod.rs @@ -172,12 +172,19 @@ pub fn construct_validator_txn_from_extra_data( /// Create a new system call transaction /// /// Helper function used by both JWK and DKG transaction construction +// TODO(GRETH-041): This function is duplicated in metadata_txn.rs. Keep this as +// the canonical definition and make metadata_txn.rs use super::new_system_call_txn(). pub(crate) fn new_system_call_txn( contract: Address, nonce: u64, gas_price: u128, input: Bytes, ) -> TransactionSigned { + // Design Intent (GRETH-040): Zero-signature (r=0, s=0) system transactions are + // intentional. Security relies on the on-chain SystemAccessControl modifier in + // protocol contracts, not on transaction-level signatures. The SYSTEM_CALLER + // address is a protocol constant that can only originate transactions through + // this privileged code path. TransactionSigned::new_unhashed( Transaction::Legacy(TxLegacy { chain_id: None, diff --git a/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/oracle_state.rs b/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/oracle_state.rs index 4c0ec2676a..c976931994 100644 --- a/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/oracle_state.rs +++ b/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/oracle_state.rs @@ -145,6 +145,9 @@ where results.push(OracleSourceState { source_type: SOURCE_TYPE_BLOCKCHAIN, source_id: source_id.try_into().unwrap_or(0), + // TODO(GRETH-043): Silent truncation from u128 to u64. Values above + // u64::MAX wrap around, potentially causing the relayer to re-process + // already-committed events. Use u128 consistently or add overflow check. latest_nonce: latest_nonce as u64, latest_record, }); diff --git a/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/validator_set.rs b/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/validator_set.rs index 457fcd99e2..1bcdf67673 100644 --- a/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/validator_set.rs +++ b/crates/pipe-exec-layer-ext-v2/execute/src/onchain_config/validator_set.rs @@ -51,6 +51,7 @@ where ); }) .ok()?; + // Design Intent (GRETH-060): Same as epoch.rs — panic on ABI decode failure. getActiveValidatorsCall::abi_decode_returns(&result) .expect("Failed to decode getActiveValidators return value") }; diff --git a/crates/pipe-exec-layer-ext-v2/relayer/src/blockchain_source.rs b/crates/pipe-exec-layer-ext-v2/relayer/src/blockchain_source.rs index c7c02dae70..176f0ebcc1 100644 --- a/crates/pipe-exec-layer-ext-v2/relayer/src/blockchain_source.rs +++ b/crates/pipe-exec-layer-ext-v2/relayer/src/blockchain_source.rs @@ -99,6 +99,10 @@ pub struct BlockchainEventSource { portal_address: Address, /// Current block cursor for polling + // Design Intent (GRETH-062): Ordering::Relaxed is used for cursor because it is + // only ever accessed from a single async task (the poll loop). The true + // synchronization point is the `last_processed` Mutex, which provides the + // happens-before relationship needed for nonce monotonicity. cursor: AtomicU64, /// Last event we returned to caller (for exactly-once tracking and persistence) @@ -240,6 +244,12 @@ impl OracleDataSource for BlockchainEventSource { "Polling for MessageSent events" ); + // TODO(GRETH-039): Oracle data from eth_getLogs is trusted without cryptographic + // proof of log inclusion. A compromised RPC provider could serve internally-consistent + // but fabricated cross-chain messages. If validators share an RPC provider and it is + // compromised, fake messages could be finalized by quorum. Consider: + // (1) Multi-RPC cross-checking, (2) Merkle proof via eth_getProof, + // (3) Source chain light client verification. let logs = self.rpc_client.get_logs(&filter).await?; let mut results = Vec::with_capacity(logs.len()); diff --git a/crates/pipe-exec-layer-ext-v2/relayer/src/eth_client.rs b/crates/pipe-exec-layer-ext-v2/relayer/src/eth_client.rs index f67073389f..52d3dfa84b 100644 --- a/crates/pipe-exec-layer-ext-v2/relayer/src/eth_client.rs +++ b/crates/pipe-exec-layer-ext-v2/relayer/src/eth_client.rs @@ -55,6 +55,11 @@ impl EthHttpCli { let url = Url::parse(rpc_url).with_context(|| format!("Failed to parse RPC URL: {}", rpc_url))?; + // Design Intent (GRETH-058): Standard TLS (via rustls) is used without + // certificate pinning. This is acceptable for the current threat model: the RPC + // provider is a trusted infrastructure partner, and pinning would complicate + // certificate rotation. The real security boundary is the on-chain quorum + // (see GRETH-039 in blockchain_source.rs). let client_builder = ClientBuilder::new().no_proxy().use_rustls_tls(); let client = client_builder.build().with_context(|| "Failed to build HTTP client")?; diff --git a/crates/pipe-exec-layer-ext-v2/relayer/src/persistence.rs b/crates/pipe-exec-layer-ext-v2/relayer/src/persistence.rs index 1b570e07b9..27859201a9 100644 --- a/crates/pipe-exec-layer-ext-v2/relayer/src/persistence.rs +++ b/crates/pipe-exec-layer-ext-v2/relayer/src/persistence.rs @@ -58,6 +58,10 @@ impl RelayerState { .with_context(|| format!("Failed to parse state file: {}", path.display()))?; if state.version != SCHEMA_VERSION { + // TODO(GRETH-075): Mismatched schema version only produces a warning. + // Incompatible state data is loaded regardless, potentially causing + // incorrect cursor/nonce restoration. Reject incompatible versions or + // implement migration logic. warn!( "State file version {} differs from current version {}", state.version, SCHEMA_VERSION diff --git a/crates/pipe-exec-layer-ext-v2/relayer/src/uri_parser.rs b/crates/pipe-exec-layer-ext-v2/relayer/src/uri_parser.rs index 3171b3e471..e9633b48d7 100644 --- a/crates/pipe-exec-layer-ext-v2/relayer/src/uri_parser.rs +++ b/crates/pipe-exec-layer-ext-v2/relayer/src/uri_parser.rs @@ -49,6 +49,9 @@ impl ParsedOracleTask { } /// Get fromBlock parameter + // TODO(GRETH-063): Missing fromBlock defaults to 0, causing the relayer to scan + // from genesis (potentially millions of blocks). Either require the parameter + // explicitly or default to a recent finalized block. pub fn from_block(&self) -> u64 { self.params.get("fromBlock").and_then(|s| s.parse().ok()).unwrap_or(0) } diff --git a/crates/storage/provider/src/providers/static_file/manager.rs b/crates/storage/provider/src/providers/static_file/manager.rs index 942a168b0a..444d7d6cbd 100644 --- a/crates/storage/provider/src/providers/static_file/manager.rs +++ b/crates/storage/provider/src/providers/static_file/manager.rs @@ -1092,7 +1092,14 @@ impl StaticFileProvider { if segment.is_headers() { writer.prune_headers(highest_static_file_block - target_block)?; } else if let Some(block) = provider.block_body_indices(target_block)? { + // TODO(GRETH-071): If `block_body_indices` returns `None` (which can happen + // during crash recovery), pruning for transaction-based segments is silently + // skipped, leaving static files out of sync. let highest_tx = self.get_highest_static_file_tx(segment).unwrap_or_default(); + + // TODO(GRETH-073): `highest_tx - block.last_tx_num()` could underflow if + // the database indices are ahead of static files during a complex crash, + // producing astronomically large prune counts. let to_delete = highest_tx - block.last_tx_num(); if segment.is_receipts() { writer.prune_receipts(to_delete, target_block)?; diff --git a/crates/storage/storage-api/src/cache.rs b/crates/storage/storage-api/src/cache.rs index dcec027505..79f437a762 100644 --- a/crates/storage/storage-api/src/cache.rs +++ b/crates/storage/storage-api/src/cache.rs @@ -206,6 +206,11 @@ impl PersistBlockCache { last_contract_eviction_height = eviction_height; } // check and eviction account and trie state + // TODO(GRETH-030): eviction_height can exceed persist_height when + // last_state_eviction_height > persist_height from a prior cycle. + // This evicts trie/state entries not yet persisted to DB, causing + // silent state root divergence. Fix: cap with + // eviction_height = min(midpoint, persist_height) if num_items > cache_capacity { let eviction_height = if last_state_eviction_height == 0 { persist_height.saturating_sub(512).max(persist_height / 2) @@ -521,6 +526,10 @@ impl PersistBlockCache { }) } + // Design Intent (GRETH-046): Individual DashMap operations are per-shard atomic + // (sharded locking). Concurrent readers may observe partial trie updates mid-write + // (some nodes removed, new nodes not yet inserted), but this cannot cause data + // corruption. The cache overlay ensures merklization always sees consistent data. /// Write trie updates. pub fn write_trie_updates(&self, input: &TrieUpdatesV2, block_number: u64) { input.removed_nodes.par_iter().for_each(|path| {