refactor(l1,l2): make ethrex-trie no-std#6966
Conversation
|
🤖 Kimi Code ReviewOverall Assessment: This is a well-structured PR that correctly introduces Detailed Feedback: 1. Code Correctness & Safety
2. API Compatibility
3. Dependency Management
4. Minor Issues
5. CI Configuration
Conclusion: The PR is ready to merge. The Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code ReviewNo findings. The PR is narrowly scoped to making Residual risk: I could not run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Lines of code reportTotal lines added: Detailed view |
Greptile SummaryThis PR makes
Confidence Score: 4/5Safe to merge — the core trie logic is unchanged and the no_std gating is narrowly scoped to primitives with clear single-threaded guest semantics. The changes are well-contained and the soundness regression (unsound unsafe impl Sync) from the prior code is actually fixed here. The two small observations — spin::Lazy used in all builds instead of just no_std, and hashbrown compiled unconditionally — are compile-time and performance-adjacent concerns rather than correctness issues. No logic paths in host builds are altered. crates/common/trie/Cargo.toml (hashbrown as unconditional dep) and crates/common/trie/trie.rs (spin::Lazy for EMPTY_TRIE_HASH in all builds)
|
| Filename | Overview |
|---|---|
| crates/common/trie/trie.rs | Crate root converted to no_std via cfg_attr; modules gated on std feature; lazy_static replaced by spin::Lazy for EMPTY_TRIE_HASH (now applies spin-wait in std builds too); FxHashMap/FxHashSet become build-dependent type aliases |
| crates/common/trie/Cargo.toml | Workspace deps inlined with explicit versions to control default-features; std feature added to propagate std to all deps; crossbeam/rayon become optional; hashbrown and spin added unconditionally (hashbrown compiled but unused in std builds) |
| crates/common/trie/db.rs | std::sync::Mutex replaced by spin::Mutex in no_std builds; lock_inner() helper abstracts the Result vs direct-return API difference; from_nodes gated to std only; API and logic appear correct |
| crates/common/trie/node.rs | Custom OnceLock widened from cfg(eip-8025+riscv64) to cfg(not(std)); unsafe impl Sync correctly removed making the type !Sync; soundness improved over prior approach |
| crates/common/common.rs | TrieLogger/TrieWitness re-exports removed; callers now import directly from ethrex_trie; blockchain.rs updated accordingly; storage/store.rs was already importing from ethrex_trie directly |
| .github/workflows/pr_nostd.yaml | ethrex-trie added to no_std CI check with --no-default-features; eip-8025 feature also verified no_std-clean; appropriate guard against regressions |
| crates/common/trie/nibbles.rs | std::{cmp,mem,arch} replaced with core equivalents; Hash trait moved to core::hash; minimal and correct changes |
| crates/common/types/block_execution_witness.rs | FxHashMap import moved from rustc_hash to ethrex_trie re-export; in std builds the type resolves identically to rustc_hash::FxHashMap; safe change |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph std["std build (default)"]
STD_LOCK["std::sync::Mutex\n(NodeMap)"]
STD_ONCE["std::sync::OnceLock\n(node hash cache)"]
STD_LAZY["spin::Lazy\n(EMPTY_TRIE_HASH)"]
STD_HASH["rustc_hash::FxHashMap"]
STD_MOD["logger, threadpool,\ntrie_sorted, from_nodes,\nvalidate_parallel"]
end
subgraph nostd["no_std build (guest)"]
NS_LOCK["spin::Mutex\n(NodeMap)"]
NS_ONCE["UnsafeCell OnceLock\n(!Sync, no atomics)"]
NS_LAZY["spin::Lazy\n(EMPTY_TRIE_HASH)"]
NS_HASH["hashbrown::HashMap\n(FxBuildHasher)"]
NS_MOD["core trie ops only\n(get, insert, remove,\ncommit, verify_range)"]
end
FEAT{"feature = std?"}
FEAT -- yes --> std
FEAT -- no --> nostd
HOST["Host (ethrex-trie default)"] --> FEAT
GUEST["zkVM Guest\n(ethrex-common, no_std)"] --> FEAT
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
subgraph std["std build (default)"]
STD_LOCK["std::sync::Mutex\n(NodeMap)"]
STD_ONCE["std::sync::OnceLock\n(node hash cache)"]
STD_LAZY["spin::Lazy\n(EMPTY_TRIE_HASH)"]
STD_HASH["rustc_hash::FxHashMap"]
STD_MOD["logger, threadpool,\ntrie_sorted, from_nodes,\nvalidate_parallel"]
end
subgraph nostd["no_std build (guest)"]
NS_LOCK["spin::Mutex\n(NodeMap)"]
NS_ONCE["UnsafeCell OnceLock\n(!Sync, no atomics)"]
NS_LAZY["spin::Lazy\n(EMPTY_TRIE_HASH)"]
NS_HASH["hashbrown::HashMap\n(FxBuildHasher)"]
NS_MOD["core trie ops only\n(get, insert, remove,\ncommit, verify_range)"]
end
FEAT{"feature = std?"}
FEAT -- yes --> std
FEAT -- no --> nostd
HOST["Host (ethrex-trie default)"] --> FEAT
GUEST["zkVM Guest\n(ethrex-common, no_std)"] --> FEAT
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
crates/common/trie/trie.rs:77-81
**`spin::Lazy` used in std builds for `EMPTY_TRIE_HASH`**
`spin::Lazy` replaces `lazy_static!` in all builds (std and no_std). In std builds this swaps `std::sync::Once` (OS-level blocking) for a spin-wait during the very first access. The keccak computation is ~microseconds so the practical impact is negligible, but this is worth noting: under extremely unusual pathological conditions (e.g., many threads hammering `EMPTY_TRIE_HASH` simultaneously before it is initialised) threads will busy-spin rather than yield. Restricting the spin primitive to only the `not(feature = "std")` path and keeping `std::sync::OnceLock` for the std path would preserve the previous behaviour.
### Issue 2 of 2
crates/common/trie/Cargo.toml:29
**`hashbrown` compiled in every build even when unused**
`hashbrown` is an unconditional non-optional dependency, but its types (`FxHashMap`, `FxHashSet`) are only referenced under `#[cfg(not(feature = "std"))]`. In a `std` build Cargo compiles `hashbrown` even though no code uses it, adding unnecessary compile time. Making it optional (`optional = true`) and declaring it only active in the `not(std)` code path would avoid this overhead.
Reviews (1): Last reviewed commit: "feat(trie): build guest trie as no_std" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // `spin::Lazy` keeps the `*EMPTY_TRIE_HASH` deref API while working in no_std. | ||
| pub static EMPTY_TRIE_HASH: Lazy<H256> = Lazy::new(|| H256(keccak_hash([RLP_NULL]))); | ||
|
|
||
| /// RLP-encoded trie path | ||
| pub type PathRLP = Vec<u8>; |
There was a problem hiding this comment.
spin::Lazy used in std builds for EMPTY_TRIE_HASH
spin::Lazy replaces lazy_static! in all builds (std and no_std). In std builds this swaps std::sync::Once (OS-level blocking) for a spin-wait during the very first access. The keccak computation is ~microseconds so the practical impact is negligible, but this is worth noting: under extremely unusual pathological conditions (e.g., many threads hammering EMPTY_TRIE_HASH simultaneously before it is initialised) threads will busy-spin rather than yield. Restricting the spin primitive to only the not(feature = "std") path and keeping std::sync::OnceLock for the std path would preserve the previous behaviour.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/common/trie/trie.rs
Line: 77-81
Comment:
**`spin::Lazy` used in std builds for `EMPTY_TRIE_HASH`**
`spin::Lazy` replaces `lazy_static!` in all builds (std and no_std). In std builds this swaps `std::sync::Once` (OS-level blocking) for a spin-wait during the very first access. The keccak computation is ~microseconds so the practical impact is negligible, but this is worth noting: under extremely unusual pathological conditions (e.g., many threads hammering `EMPTY_TRIE_HASH` simultaneously before it is initialised) threads will busy-spin rather than yield. Restricting the spin primitive to only the `not(feature = "std")` path and keeping `std::sync::OnceLock` for the std path would preserve the previous behaviour.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| rkyv = { version = "0.8.10", default-features = false, features = ["alloc", "unaligned", "bytecheck"] } | ||
|
|
||
| # no_std HashMap/HashSet, only compiled when `std` is off (replaces the std-only rustc-hash aliases) | ||
| hashbrown = { version = "0.15", default-features = false } |
There was a problem hiding this comment.
hashbrown compiled in every build even when unused
hashbrown is an unconditional non-optional dependency, but its types (FxHashMap, FxHashSet) are only referenced under #[cfg(not(feature = "std"))]. In a std build Cargo compiles hashbrown even though no code uses it, adding unnecessary compile time. Making it optional (optional = true) and declaring it only active in the not(std) code path would avoid this overhead.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/common/trie/Cargo.toml
Line: 29
Comment:
**`hashbrown` compiled in every build even when unused**
`hashbrown` is an unconditional non-optional dependency, but its types (`FxHashMap`, `FxHashSet`) are only referenced under `#[cfg(not(feature = "std"))]`. In a `std` build Cargo compiles `hashbrown` even though no code uses it, adding unnecessary compile time. Making it optional (`optional = true`) and declaring it only active in the `not(std)` code path would avoid this overhead.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Benchmark Results ComparisonBenchmark Results: MstoreBench
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
Certain zkEVMs can't handle std code, and thus we want to allow no-std usage. Currently ethrex-trie has all it's ethrex dependencies already no-std, so it's a good candidate for no-std-ification
Description
Makes the crate no-std by:
This also adds the crate to the no-std CI job to prevent obvious regressions.