[WIP] Add Gnosis support#6720
Conversation
|
🤖 Claude Code ReviewNow I have a thorough understanding of the diff. Let me write up the review. Code Review: PR #6720 — Add Gnosis SupportOverviewThis PR adds Gnosis Chain (chain ID 100) and Chiado testnet (chain ID 10200) support to ethrex. The implementation covers:
The approach is generally sound and well-commented, but there are several correctness concerns and at least one critical cross-chain regression risk. Critical Issues1. Global snap-sync constants hardcoded for Gnosis break Ethereum mainnet
pub const SECONDS_PER_BLOCK: u64 = 5; // was 12
pub const MISSING_SLOTS_PERCENTAGE: f64 = 0.98; // was 0.8These are consumed globally — Correctness Issues2. AuRa seal length is not validated on decode
let (seal_bytes, decoder) = decoder.decode_field::<Bytes>("aura_seal")?;
// seal_bytes is accepted without length checkThe doc comment on if seal_bytes.len() != 65 {
return Err(RLPDecodeError::InvalidValue);
}3. Fee-collector base fee double-credit risk
let base_fee_credit = U256::from(gas_to_pay)
.checked_mul(vm.env.base_fee_per_gas)
.ok_or(InternalError::Overflow)?;
vm.increase_account_balance(fee_collector, base_fee_credit)?;This is correct only if 4.
let withdrawal_contract = chain_config.deposit_contract_address;
5.
debug_assert_eq!(amounts_gwei.len(), addresses.len());In release mode a mismatched slice pair produces malformed ABI calldata without any error. Since the contract would then be called with garbage, and a revert is fatal to the block, this should be a hard error: assert_eq!(amounts_gwei.len(), addresses.len(), "withdrawal amounts/addresses length mismatch");or return a Design / Architecture Issues6.
7. #[serde(skip)]
pub genesis_hash_override: Option<H256>,If a 8.
let _ = header.hash.set(canonical);If 9. AuRa detection heuristic relies on an RLP prefix byte
let is_geth_shape = next_encoded.first().copied() == Some(0xa0);
P2P / Security Issue10. ENR peers without fork ID are now accepted optimistically
-self.peer_table.set_is_fork_id_valid(node_id, false)?;
+self.peer_table.set_is_fork_id_valid(node_id, true)?;The intent (verify via eth/68+ handshake) is documented and reasonable. However, this widens the set of peers we attempt to connect to and shifts the filtering to a later, more expensive stage. On a high-traffic network, this could increase the number of useless RLPx connections attempted before the status handshake rejects them. This is an acceptable tradeoff, but worth calling out explicitly since it changes security-relevant behavior. Minor / Style Issues11. Missing newlines at end of JSON files
12.
trait U256ToBuf {
fn to_big_endian_buf(&self) -> [u8; 32];
}
let mut buf = [0u8; 32];
val.to_big_endian(&mut buf);
out.extend_from_slice(&buf);The local trait adds an extra abstraction for no gain. 13. The function is duplicated across What's Done Well
Automated review by Claude (Anthropic) · sonnet · custom prompt |
Lines of code reportTotal lines added: Detailed view |
🤖 Codex Code Review
I did not run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR adds initial Gnosis Chain (chain ID 100) and Chiado testnet (chain ID 10200) support to ethrex, including post-block system calls (block-rewards + withdrawal contracts), EIP-1559/4844 fee redirection to a collector contract, AuRa-era genesis hash overrides, and pruning-aware header download.
Confidence Score: 3/5The Gnosis-specific execution logic is well-structured with tests, but one global constant change in the snap-sync layer directly degrades Ethereum mainnet sync behaviour and should be addressed before merging to main. The snap-sync constant SECONDS_PER_BLOCK is set to 5 (Gnosis's block time) globally, shrinking the staleness window for Ethereum mainnet from ~25 minutes to ~10 minutes and making pivot-block estimation 2.4x too aggressive. The author's own comment acknowledges this needs to be parameterised through ChainConfig. Everything else — the new gnosis crate, AuRa RLP bifurcation, fee-collector hooks, and peer pruning logic — looks correct and is backed by unit tests. crates/networking/p2p/snap/constants.rs (SECONDS_PER_BLOCK regression) and crates/vm/levm/src/environment.rs (inconsistent min_blob_base_fee default).
|
| Filename | Overview |
|---|---|
| crates/networking/p2p/snap/constants.rs | Global snap-sync tuning constants changed for Gnosis compatibility; SECONDS_PER_BLOCK hardcoded to 5 (Gnosis value), regressing Ethereum mainnet staleness-window and pivot-estimation logic. |
| crates/gnosis/src/system_calls.rs | New file: hand-rolled ABI encoding/decoding for the two Gnosis post-block system calls (withdrawal contract and POSDAO block-rewards contract); well-tested with round-trip and selector tests. |
| crates/gnosis/src/genesis.rs | New file: loads embedded Gnosis/Chiado genesis JSON and applies canonical hash overrides to handle AuRa-era genesis blocks; tests verify hash matches live networks. |
| crates/common/types/block.rs | Adds aura_step/aura_seal header fields and bifurcated RLP encode/decode path; detection heuristic (0xa0 prefix) is sound, but partial field population silently falls back to geth encoding without an error. |
| crates/vm/backends/levm/mod.rs | Adds apply_gnosis_post_block_calls (withdrawal + block-rewards system calls) to all three execution pipelines, and skips native EIP-4895 withdrawal credits on Gnosis. |
| crates/common/types/genesis.rs | Adds Gnosis-specific ChainConfig fields (fee collector, block-rewards contract, POSDAO block, Balancer hardfork, min blob price) and genesis_hash_override for canonical genesis hash override. |
| crates/networking/p2p/peer_handler.rs | Reads min_peer_earliest_block before header download and skips to the peer-served window on Chiado/pruned networks; scoring change marks partial responses as failures to avoid score saturation. |
| crates/networking/p2p/snap/client.rs | Refactors storage-range task scheduling to separate bulk and per-interval paths, fixing a stuck-account bug where completed big accounts were re-queued indefinitely. |
| crates/vm/levm/src/environment.rs | Adds fee_collector and min_blob_base_fee to EVMConfig; min_blob_base_fee default uses literal 1 rather than MIN_BASE_FEE_PER_BLOB_GAS constant used elsewhere. |
| crates/networking/p2p/peer_table.rs | Adds earliest_block field to PeerData, set_peer_earliest_block send handler, and min_peer_earliest_block request handler for pruning-aware peer selection. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Block received] --> B{is_gnosis?}
B -- No --> C[Standard EIP-4895 withdrawal credits]
B -- Yes --> D[Skip native withdrawal credits]
D --> E{is_shanghai_activated?}
E -- Yes --> F[Call withdrawal contract\nexecuteSystemWithdrawals\ndeposit_contract_address]
E -- No --> G[Skip withdrawal contract]
F --> H[Call block-rewards contract\nreward coinbase,0\nblock_rewards_contract]
G --> H
H --> I[Decode receivers+amounts\nCredit native balances]
C --> J[Finalize block state]
I --> J
K[pay_coinbase hook] --> L{fee_collector set?}
L -- Yes --> M[Credit base_fee to fee_collector\ninstead of burning]
L -- No --> N[Burn base fee standard]
O[deduct_caller hook] --> P{fee_collector set?}
P -- Yes --> Q[Credit blob_gas_cost to fee_collector]
P -- No --> R[Burn blob fee standard]
Comments Outside Diff (2)
-
crates/networking/p2p/snap/constants.rs, line 162-172 (link)SECONDS_PER_BLOCKis hardcoded to Gnosis's 5 s block time globallyThe comment in this PR explicitly says "this constant should eventually be plumbed through
ChainConfig", acknowledging the regression. On Ethereum mainnet (12 s blocks), snap sync is now affected: the staleness window shrinks from128 × 12 = 1 536 s (~25 min)to128 × 5 = 640 s (~10 min), andnew_pivot_block_numberoverestimates blocks elapsed by ×2.4, potentially requesting pivot numbers that haven't been produced yet. This forces unnecessary pivot re-selections and wasted state-root re-downloads on Ethereum mainnet. The constant must be read fromChainConfig(or passed as a parameter) before this lands on main so Ethereum sync isn't degraded.Prompt To Fix With AI
This is a comment left during a code review. Path: crates/networking/p2p/snap/constants.rs Line: 162-172 Comment: **`SECONDS_PER_BLOCK` is hardcoded to Gnosis's 5 s block time globally** The comment in this PR explicitly says "this constant should eventually be plumbed through `ChainConfig`", acknowledging the regression. On Ethereum mainnet (12 s blocks), snap sync is now affected: the staleness window shrinks from `128 × 12 = 1 536 s (~25 min)` to `128 × 5 = 640 s (~10 min)`, and `new_pivot_block_number` overestimates blocks elapsed by ×2.4, potentially requesting pivot numbers that haven't been produced yet. This forces unnecessary pivot re-selections and wasted state-root re-downloads on Ethereum mainnet. The constant must be read from `ChainConfig` (or passed as a parameter) before this lands on main so Ethereum sync isn't degraded. How can I resolve this? If you propose a fix, please make it concise.
-
crates/common/types/block.rs, line 246-270 (link)Partial AuRa fields silently fall back to geth encoding
If a
BlockHeaderis constructed withaura_step = Some(…)butaura_seal = None(or vice versa), the conditionif let (Some(aura_step), Some(aura_seal)) = (…)isfalseand the header is silently encoded with the gethprev_randao/nonceshape, even though the caller's intent was AuRa. This means the hash computed for the block will be wrong without any error. An assertion or a returned error when exactly one of the two fields isSomewould catch this at the point of creation rather than producing a silently incorrect block hash.Prompt To Fix With AI
This is a comment left during a code review. Path: crates/common/types/block.rs Line: 246-270 Comment: **Partial AuRa fields silently fall back to geth encoding** If a `BlockHeader` is constructed with `aura_step = Some(…)` but `aura_seal = None` (or vice versa), the condition `if let (Some(aura_step), Some(aura_seal)) = (…)` is `false` and the header is silently encoded with the geth `prev_randao`/`nonce` shape, even though the caller's intent was AuRa. This means the hash computed for the block will be wrong without any error. An assertion or a returned error when exactly one of the two fields is `Some` would catch this at the point of creation rather than producing a silently incorrect block hash. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
crates/networking/p2p/snap/constants.rs:162-172
**`SECONDS_PER_BLOCK` is hardcoded to Gnosis's 5 s block time globally**
The comment in this PR explicitly says "this constant should eventually be plumbed through `ChainConfig`", acknowledging the regression. On Ethereum mainnet (12 s blocks), snap sync is now affected: the staleness window shrinks from `128 × 12 = 1 536 s (~25 min)` to `128 × 5 = 640 s (~10 min)`, and `new_pivot_block_number` overestimates blocks elapsed by ×2.4, potentially requesting pivot numbers that haven't been produced yet. This forces unnecessary pivot re-selections and wasted state-root re-downloads on Ethereum mainnet. The constant must be read from `ChainConfig` (or passed as a parameter) before this lands on main so Ethereum sync isn't degraded.
### Issue 2 of 3
crates/vm/levm/src/environment.rs:93-97
Inconsistent default for `min_blob_base_fee` — literal `1` is used here while every other callsite in the PR uses `MIN_BASE_FEE_PER_BLOB_GAS`. They are equal today, but if the constant is ever changed the two defaults will diverge silently.
```suggestion
min_blob_base_fee: chain_config.min_blob_gas_price.unwrap_or(MIN_BASE_FEE_PER_BLOB_GAS),
}
}
/// This function is used for running the EF tests. If you don't
```
### Issue 3 of 3
crates/common/types/block.rs:246-270
**Partial AuRa fields silently fall back to geth encoding**
If a `BlockHeader` is constructed with `aura_step = Some(…)` but `aura_seal = None` (or vice versa), the condition `if let (Some(aura_step), Some(aura_seal)) = (…)` is `false` and the header is silently encoded with the geth `prev_randao`/`nonce` shape, even though the caller's intent was AuRa. This means the hash computed for the block will be wrong without any error. An assertion or a returned error when exactly one of the two fields is `Some` would catch this at the point of creation rather than producing a silently incorrect block hash.
Reviews (1): Last reviewed commit: "feat(l1): implement support for the gnos..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| min_blob_base_fee: chain_config.min_blob_gas_price.unwrap_or(1), | ||
| } | ||
| } | ||
|
|
||
| /// This function is used for running the EF tests. If you don't |
There was a problem hiding this comment.
min_blob_base_fee — literal 1 is used here while every other callsite in the PR uses MIN_BASE_FEE_PER_BLOB_GAS. They are equal today, but if the constant is ever changed the two defaults will diverge silently.
| min_blob_base_fee: chain_config.min_blob_gas_price.unwrap_or(1), | |
| } | |
| } | |
| /// This function is used for running the EF tests. If you don't | |
| min_blob_base_fee: chain_config.min_blob_gas_price.unwrap_or(MIN_BASE_FEE_PER_BLOB_GAS), | |
| } | |
| } | |
| /// This function is used for running the EF tests. If you don't |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/vm/levm/src/environment.rs
Line: 93-97
Comment:
Inconsistent default for `min_blob_base_fee` — literal `1` is used here while every other callsite in the PR uses `MIN_BASE_FEE_PER_BLOB_GAS`. They are equal today, but if the constant is ever changed the two defaults will diverge silently.
```suggestion
min_blob_base_fee: chain_config.min_blob_gas_price.unwrap_or(MIN_BASE_FEE_PER_BLOB_GAS),
}
}
/// This function is used for running the EF tests. If you don't
```
How can I resolve this? If you propose a fix, please make it concise.
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
1 participant
Motivation
We want to be able to execute blocks in the Gnosis network.
Description
Gnosis is very similar to ethereum mainnet, and so the differences are rather small.
To test, start a consensus node as explained on the gnosis documentation, and start ethrex setting the network to 'gnosis'.