feat: add ev-dev local chain and txpool transaction support#145
feat: add ev-dev local chain and txpool transaction support#145
Conversation
…mode Add ev-dev binary providing a one-command local development chain with pre-funded Hardhat accounts, similar to Anvil or Hardhat Node. Includes genesis configuration and Makefile targets for convenience. Fix payload builder to pull pending transactions from the txpool when Engine API attributes are empty, enabling cast send and other RPC-submitted transactions to be included in blocks during --dev mode. Add DebugNode implementation for EvolveNode supporting local payload attributes builder.
|
Claude finished @randygrok's task —— View job Claude finished @randygrok's task —— View job Pull Request ReviewAfter analyzing this PR thoroughly, here's my comprehensive review of the code quality, potential issues, performance considerations, security concerns, and test coverage. Todo List:
OverviewThis PR adds significant development tooling improvements to ev-reth by introducing the Code Quality and Best practices✅ Strengths
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new ev-dev CLI binary and genesis asset to run a local dev chain with deterministic pre-funded accounts and a sponsor RPC; refactors the payload builder to accept and pull transactions from a txpool in dev mode; introduces a DebugNode impl for EvolveNode and adds justfile recipes. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as ev-dev CLI
participant KeyGen as Key Generator
participant Genesis as Genesis Loader
participant EvNode as EvNode
participant TxpoolRPC as Sponsor-aware Txpool RPC
User->>CLI: run `ev-dev` with args
CLI->>KeyGen: derive accounts from mnemonic
KeyGen-->>CLI: addresses & private keys
CLI->>Genesis: read devnet-genesis.json
Genesis-->>CLI: genesis config
CLI->>CLI: write temp genesis & data dir
CLI->>EvNode: assemble node config (chainId, RPC, dev flags)
EvNode->>TxpoolRPC: register sponsor-aware txpool API
TxpoolRPC-->>EvNode: API ready
EvNode-->>CLI: node started, RPC endpoints available
CLI->>User: display endpoints & accounts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
Makefile (1)
58-64: Add new command targets to.PHONYto prevent accidental skipped execution.Without
.PHONY,make dev-chain/make build-ev-devcan be treated as up-to-date if same-named files exist.♻️ Proposed Makefile update
-.PHONY: all build test clean fmt lint run help +.PHONY: all build build-dev build-maxperf build-ev-dev build-all \ + test test-verbose test-unit test-integration test-node test-evolve test-common \ + run run-dev dev-chain fmt fmt-check lint check clean update audit doc doc-all check-all help🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 58 - 64, Add the new make targets to the .PHONY list so they are always executed: update the existing .PHONY definition to include build-ev-dev and dev-chain (the targets defined by the rules for "build-ev-dev:" and "dev-chain:") so make won't treat them as up-to-date when files with those names exist.crates/node/src/payload_service.rs (3)
325-413: Add a regression test forattributes empty -> txpool fallbacktransaction inclusion.This PR’s core behavior change isn’t directly asserted yet; please add an integration test that seeds pool txs and verifies they are included when attributes are empty.
Based on learnings: Write integration tests in
crates/tests/covering Engine API interactions, payload building with transactions, state execution validation, and Evolve-specific scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/payload_service.rs` around lines 325 - 413, Add an integration test under crates/tests/ that seeds a real/test transaction pool and verifies EvolveEnginePayloadBuilder.try_build includes pool transactions when RpcPayloadAttributes.withdrawals and other attributes are empty: replace NoopTransactionPool in the setup (EvolveEnginePayloadBuilder.pool) with a seeded test pool implementation (or a helper that implements TransactionPool and inserts known EvPooledTransaction(s)), call engine_builder.try_build(BuildArguments::new(...)) with EthPayloadBuilderAttributes built from empty RpcPayloadAttributes, then assert the built payload contains those transactions (check payload.transaction list or tx_count field and that known tx hashes are present). Use the same fixtures from the test (EvolvePayloadBuilder, EvolveEnginePayloadBuilder, BuildArguments, RpcPayloadAttributes, PayloadConfig) and name the test to reflect "attributes empty -> txpool fallback".
139-143:tx_countspan field is inaccurate on txpool fallback path.When
attributes.transactionsis empty, the span still reports0even if pool transactions are used.🧭 Suggested instrumentation adjustment
#[instrument(skip(self, args), fields( - tx_count = args.config.attributes.transactions.len(), + tx_count = tracing::field::Empty, payload_id = %args.config.attributes.payload_id(), duration_ms = tracing::field::Empty, ))] fn try_build( @@ let transactions = if attributes.transactions.is_empty() { @@ } else { attributes.transactions.clone() }; + + tracing::Span::current().record("tx_count", transactions.len() as u64);Also applies to: 182-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/payload_service.rs` around lines 139 - 143, The span field tx_count set in the #[instrument(..., fields(tx_count = args.config.attributes.transactions.len(), ...))] attribute is stale on the txpool fallback path; change to compute the actual transaction count at runtime inside the function (determine whether to use args.config.attributes.transactions or pool transactions), then update the span after that decision by recording the real count into the span (e.g., Span::current().record or equivalent) using the same field name "tx_count"; apply the same fix for the other instrumented block referenced around lines 182-197 so both places reflect the actual tx count rather than args.config.attributes.transactions.len().
185-190: Avoid unbounded materialization of the entire pending pool each build.Collecting all pending txs into a
Vecper payload build can degrade latency/memory under larger pools; prefer a bounded selection strategy for fallback mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/payload_service.rs` around lines 185 - 190, The current fallback path in payload_service.rs materializes the entire pending pool into pool_txs by collecting pending_transactions() into a Vec, which can blow memory/latency; change the fallback selection to a bounded strategy: iterate pending_transactions() without collecting all, apply a deterministic or priority-based filter and .take(MAX_FALLBACK_TXS) (or reservoir/sample if needed), and build the payload from that bounded iterator instead of the full Vec; update references to pool_txs and the place that expects Vec<TransactionSigned> (e.g., where TransactionSigned instances are consumed) to accept the streamed/limited selection and define a MAX_FALLBACK_TXS constant for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/ev-dev/src/main.rs`:
- Around line 46-49: The documented max for the CLI flag 'accounts' is not
enforced at parse time; update the 'accounts' argument declaration (the struct
field named accounts) to validate the range at parse-time (e.g., add a Clap
value/validator that restricts values to 1..=20) so invalid inputs produce an
error instead of being silently clamped, or alternatively adjust the
help/default text to reflect the actual behavior; ensure the change is applied
on the 'accounts' field annotation used by clap.
- Line 148: Run rustfmt/cargo fmt to fix formatting in main.rs: reformat the
file (or run `cargo fmt`) to resolve the lint error reported at line 148; ensure
the `main` function and surrounding declarations in bin/ev-dev/src/main.rs are
formatted to match rustfmt rules and commit the resulting changes.
- Around line 192-194: Replace the .expect("valid CLI args") panic by handling
the result of Cli::<EvolveChainSpecParser, EvolveArgs>::try_parse_from(args)
explicitly: match or map the Err case and convert the clap parse error into a
user-facing error (e.g., print the clap error via eprintln!("{}", err) or return
Err(err.into()) from main) so the launcher fails gracefully instead of
panicking; then pass the successful parsed Cli value into .run(|builder,
_evolve_args| async move { ... }) as before.
In `@crates/node/src/payload_service.rs`:
- Line 189: The file crates/node/src/payload_service.rs has formatting
mismatches flagged by CI; run rustfmt (cargo fmt) and reformat the file so the
closure/map expressions like the one that returns
tx.transaction.clone_into_consensus().into_inner() and the nearby expressions
are formatted according to rustfmt rules (fix spacing, line breaks and trailing
commas) — simply run cargo fmt and commit the resulting changes to resolve the
lint failures at the locations currently flagged.
---
Nitpick comments:
In `@crates/node/src/payload_service.rs`:
- Around line 325-413: Add an integration test under crates/tests/ that seeds a
real/test transaction pool and verifies EvolveEnginePayloadBuilder.try_build
includes pool transactions when RpcPayloadAttributes.withdrawals and other
attributes are empty: replace NoopTransactionPool in the setup
(EvolveEnginePayloadBuilder.pool) with a seeded test pool implementation (or a
helper that implements TransactionPool and inserts known
EvPooledTransaction(s)), call engine_builder.try_build(BuildArguments::new(...))
with EthPayloadBuilderAttributes built from empty RpcPayloadAttributes, then
assert the built payload contains those transactions (check payload.transaction
list or tx_count field and that known tx hashes are present). Use the same
fixtures from the test (EvolvePayloadBuilder, EvolveEnginePayloadBuilder,
BuildArguments, RpcPayloadAttributes, PayloadConfig) and name the test to
reflect "attributes empty -> txpool fallback".
- Around line 139-143: The span field tx_count set in the #[instrument(...,
fields(tx_count = args.config.attributes.transactions.len(), ...))] attribute is
stale on the txpool fallback path; change to compute the actual transaction
count at runtime inside the function (determine whether to use
args.config.attributes.transactions or pool transactions), then update the span
after that decision by recording the real count into the span (e.g.,
Span::current().record or equivalent) using the same field name "tx_count";
apply the same fix for the other instrumented block referenced around lines
182-197 so both places reflect the actual tx count rather than
args.config.attributes.transactions.len().
- Around line 185-190: The current fallback path in payload_service.rs
materializes the entire pending pool into pool_txs by collecting
pending_transactions() into a Vec, which can blow memory/latency; change the
fallback selection to a bounded strategy: iterate pending_transactions() without
collecting all, apply a deterministic or priority-based filter and
.take(MAX_FALLBACK_TXS) (or reservoir/sample if needed), and build the payload
from that bounded iterator instead of the full Vec; update references to
pool_txs and the place that expects Vec<TransactionSigned> (e.g., where
TransactionSigned instances are consumed) to accept the streamed/limited
selection and define a MAX_FALLBACK_TXS constant for clarity.
In `@Makefile`:
- Around line 58-64: Add the new make targets to the .PHONY list so they are
always executed: update the existing .PHONY definition to include build-ev-dev
and dev-chain (the targets defined by the rules for "build-ev-dev:" and
"dev-chain:") so make won't treat them as up-to-date when files with those names
exist.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
CHANGELOG.mdCargo.tomlMakefilebin/ev-dev/Cargo.tomlbin/ev-dev/assets/devnet-genesis.jsonbin/ev-dev/src/main.rscrates/node/src/node.rscrates/node/src/payload_service.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
bin/ev-dev/src/main.rs (2)
188-190:⚠️ Potential issue | 🟠 MajorAvoid panic on nested CLI parse failures; return a user-facing error path.
Using
.expect("valid CLI args")can panic instead of exiting cleanly when composed args become invalid.Suggested fix
- if let Err(err) = Cli::<EvolveChainSpecParser, EvolveArgs>::try_parse_from(args) - .expect("valid CLI args") - .run(|builder, _evolve_args| async move { + let cli = match Cli::<EvolveChainSpecParser, EvolveArgs>::try_parse_from(args) { + Ok(cli) => cli, + Err(err) => { + eprintln!("{err}"); + std::process::exit(2); + } + }; + + if let Err(err) = cli.run(|builder, _evolve_args| async move { info!("=== EV-DEV: Starting local development chain ==="); let handle = builder .node(EvolveNode::new()) @@ - }) + }) { eprintln!("Error: {err:?}"); std::process::exit(1); }Also applies to: 207-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ev-dev/src/main.rs` around lines 188 - 190, The code currently calls Cli::<EvolveChainSpecParser, EvolveArgs>::try_parse_from(args).expect("valid CLI args") which can panic; replace this with explicit error handling: call try_parse_from(args), match on the Result, and for Err(e) print a user-friendly error to stderr (or log via the existing logger) and return an Err/ExitCode instead of panicking; likewise remove any other uses of .expect for nested CLI parsing (e.g., the same pattern around the run invocation at the other occurrence) and propagate or map the parsing error into a graceful exit path so the run closure is only invoked on Ok(parsed_cli).
46-49:⚠️ Potential issue | 🟡 MinorEnforce
accountsbounds at parse-time instead of silently clamping.The CLI contract says max 20, but current behavior accepts larger values and clamps later in banner rendering. Validate on input so invalid values fail fast.
Suggested fix
- #[arg(long, default_value_t = 10)] + #[arg( + long, + default_value_t = 10, + value_parser = clap::value_parser!(usize).range(1..=20) + )] accounts: usize,- let count = args.accounts.min(20); - let accounts = derive_keys(count); + let accounts = derive_keys(args.accounts);Also applies to: 79-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ev-dev/src/main.rs` around lines 46 - 49, The CLI accepts an accounts usize but currently allows out-of-range values and clamps them later; change the clap arg to validate at parse-time by adding a value parser with an explicit range (e.g. use clap::value_parser!(usize).range(1..=20)) to the #[arg(...)] for accounts (and the similar arg at the other location around lines 79-80) so parsing fails on invalid input instead of silently clamping in banner rendering.
🧹 Nitpick comments (1)
crates/node/src/payload_service.rs (1)
316-317: Add a behavior test for “empty attributes + non-empty pool” path.The test wiring with
NoopTransactionPoolis good for construction, but it still doesn’t verify the new fallback behavior that actually pulls txs from pool and includes them in the built block. A focused integration test for that path would prevent regressions.Based on learnings: Write integration tests in
crates/tests/covering Engine API interactions, payload building with transactions, state execution validation, and Evolve-specific scenarios.Also applies to: 326-326, 378-378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/payload_service.rs` around lines 316 - 317, Add an integration test under crates/tests/ that exercises the "empty attributes + non-empty pool" fallback: construct a real tx pool (not NoopTransactionPool) populated with one or more txs using txpool::EvPooledTransaction, initialize the Engine API and the PayloadService instance, invoke the payload building flow with empty attributes and assert that PayloadService (or the public build/build_payload method) pulls transactions from the pool and includes them in the returned block/payload; the test should also validate state execution results returned by the Engine API to ensure the included txs were applied correctly and cover Evolve-specific behavior if applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/node/src/payload_service.rs`:
- Around line 182-200: Telemetry's tx_count span field is using the original
attributes.transactions.len() even when you fallback to pool transactions;
update the span to reflect the actual transactions used by computing the count
from the chosen transactions variable (e.g., after the fallback logic that
produces pool_txs and assigns transactions) and set the tx_count/span field to
transactions.len() (or pool_txs.len() when you use pool_txs) so the reported
tx_count matches the built payload; reference the attributes.transactions,
pool_txs, and transactions variables and update the span/telemetry call that
currently uses attributes.transactions.len().
---
Duplicate comments:
In `@bin/ev-dev/src/main.rs`:
- Around line 188-190: The code currently calls Cli::<EvolveChainSpecParser,
EvolveArgs>::try_parse_from(args).expect("valid CLI args") which can panic;
replace this with explicit error handling: call try_parse_from(args), match on
the Result, and for Err(e) print a user-friendly error to stderr (or log via the
existing logger) and return an Err/ExitCode instead of panicking; likewise
remove any other uses of .expect for nested CLI parsing (e.g., the same pattern
around the run invocation at the other occurrence) and propagate or map the
parsing error into a graceful exit path so the run closure is only invoked on
Ok(parsed_cli).
- Around line 46-49: The CLI accepts an accounts usize but currently allows
out-of-range values and clamps them later; change the clap arg to validate at
parse-time by adding a value parser with an explicit range (e.g. use
clap::value_parser!(usize).range(1..=20)) to the #[arg(...)] for accounts (and
the similar arg at the other location around lines 79-80) so parsing fails on
invalid input instead of silently clamping in banner rendering.
---
Nitpick comments:
In `@crates/node/src/payload_service.rs`:
- Around line 316-317: Add an integration test under crates/tests/ that
exercises the "empty attributes + non-empty pool" fallback: construct a real tx
pool (not NoopTransactionPool) populated with one or more txs using
txpool::EvPooledTransaction, initialize the Engine API and the PayloadService
instance, invoke the payload building flow with empty attributes and assert that
PayloadService (or the public build/build_payload method) pulls transactions
from the pool and includes them in the returned block/payload; the test should
also validate state execution results returned by the Engine API to ensure the
included txs were applied correctly and cover Evolve-specific behavior if
applicable.
chatton
left a comment
chatton
left a comment
There was a problem hiding this comment.
Looks good! It might be worth while to create a CI workflow that does some basic usage of each of the commands just to ensure it starts a node correctly and all the addresses exist and are funded.
bin/ev-dev/src/main.rs
Outdated
| println!("Available Accounts"); | ||
| println!("=================="); | ||
| for (i, (addr, _)) in accounts.iter().enumerate() { | ||
| println!("({i}) {addr} (1000 ETH)"); |
There was a problem hiding this comment.
I think this hard coded 1000 ETH value is not correct, I think the balance of each account is 1,000,000
from genesis
"0x14dc79964da2c08b23698b3d3cc7ca32193d9955": {
"balance": "0xd3c21bcecceda1000000"
}
There was a problem hiding this comment.
is it not even more, 1 billion ETH?
10^27 / 10^18 = 10^9
1,000,000,000 ETH
Makefile was deleted on main (replaced by justfile in #143). Added ev-dev recipes (build-ev-dev, dev-chain) to justfile instead.
- Validate --accounts at parse-time (1..=20) instead of silent clamping - Handle CLI parse errors gracefully instead of panicking with .expect() - Fix displayed account balance: 1000000 ETH (matches genesis) - Record tx_count span after pool fallback so telemetry reflects actual count
|
Claude finished @randygrok's task —— View job Pull Request ReviewAfter analyzing this PR thoroughly, here's my comprehensive review of the code quality, potential issues, performance considerations, security concerns, and test coverage. Todo List:
OverviewThis PR adds significant development tooling improvements to ev-reth by introducing the Code Quality and Best practices✅ Strengths
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
bin/ev-dev/src/main.rs (1)
115-115:⚠️ Potential issue | 🟡 MinorAvoid hardcoding displayed ETH balance in the banner.
Line 115 prints a fixed
1000000 ETH, which can drift from the actual genesis allocations and mislead users. Consider deriving the displayed value fromDEVNET_GENESIS(or removing the numeric claim from the banner).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ev-dev/src/main.rs` at line 115, The banner prints a hardcoded "1000000 ETH" which can be incorrect; update the println! at the site that prints "({i}) {addr} (1000000 ETH)" to derive the displayed balance from the DEVNET_GENESIS allocations (or remove the numeric balance entirely). Locate the loop in main.rs that emits the banner (the println! using variables i and addr), look up the corresponding allocation for addr in DEVNET_GENESIS (the genesis map/structure), convert the allocation to an ETH-readable unit/string, and interpolate that value into the printed message instead of the hardcoded "1000000 ETH"; if the allocation lookup is not desirable, simply omit the numeric claim to avoid displaying stale information.
🧹 Nitpick comments (1)
crates/node/src/payload_service.rs (1)
318-380: Add a test that exercises the non-empty txpool fallback branch.Current test wires
NoopTransactionPool(Line 380), so it only validates the empty-pool path. Please add a test that seeds at least one pending tx and asserts fallback inclusion whenattributes.transactionsis empty.As per coding guidelines: "Write unit tests for individual components in Rust workspace".
Also applies to: 402-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/payload_service.rs` around lines 318 - 380, The existing test try_build_span_has_expected_fields only covers the empty-txpool path because it uses NoopTransactionPool; add a new async #[tokio::test] that constructs an EvolveEnginePayloadBuilder (same setup: provider, chain_spec, config, evm_config) but supplies a transaction-pool seeded with at least one pending EvPooledTransaction (instead of NoopTransactionPool) and calls the builder with RpcPayloadAttributes that have attributes.transactions empty; assert the resulting payload used the pool fallback inclusion (e.g., by checking the built block contains the seeded tx or that fallback path in EvolveEnginePayloadBuilder was exercised). Use the same unique symbols (EvolveEnginePayloadBuilder, EvPooledTransaction, RpcPayloadAttributes, attributes.transactions) to locate code and mirror setup from the existing test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/ev-dev/src/main.rs`:
- Around line 196-201: Replace direct std::process::exit calls with returning an
error code from main so destructors run: change main's signature to return
Result<(), i32> (or another suitable Result type) and replace the
std::process::exit(2) in the Cli::<EvolveChainSpecParser,
EvolveArgs>::try_parse_from error branch (and the other std::process::exit calls
around the later failure handling) with an early return like return Err(2); (or
map parsing errors to Err(code)). Ensure all call sites compile with the new
main return type and that temporary files (tempfile::NamedTempFile /
tempfile::TempDir) are allowed to drop normally.
- Around line 26-33: Add unit tests covering the helper functions
parse_accounts, derive_keys, and chain_id_from_genesis: write tests for
parse_accounts that validate valid bounds (1 and 20) and out-of-range errors
(<1, >20) and invalid parse input; tests for derive_keys that assert
deterministic output, expected number of keypairs for a given account count and
format/length of keys; and tests for chain_id_from_genesis that extract the
correct chain id from representative genesis JSON blobs and handle
missing/invalid fields. Place tests in the same module (or a tests mod) so they
call parse_accounts, derive_keys, and chain_id_from_genesis directly and assert
both success and failure paths.
In `@crates/node/src/payload_service.rs`:
- Around line 182-200: The current fallback pulls all pending transactions into
memory (attributes.transactions, self.pool.pending_transactions()) which can
blow up for large pools; change the fallback in payload construction (the
transactions variable / pool_txs) to cap the number of pulled transactions by
introducing a configurable constant (e.g., MAX_POOL_FALLBACK_TXS) and use an
iterator .take(MAX_POOL_FALLBACK_TXS) or equivalent to limit cloning, and log
when truncation occurs so you still report pool_tx_count and whether the pool
was truncated; update any tests or configs to reflect the new cap.
---
Duplicate comments:
In `@bin/ev-dev/src/main.rs`:
- Line 115: The banner prints a hardcoded "1000000 ETH" which can be incorrect;
update the println! at the site that prints "({i}) {addr} (1000000 ETH)" to
derive the displayed balance from the DEVNET_GENESIS allocations (or remove the
numeric balance entirely). Locate the loop in main.rs that emits the banner (the
println! using variables i and addr), look up the corresponding allocation for
addr in DEVNET_GENESIS (the genesis map/structure), convert the allocation to an
ETH-readable unit/string, and interpolate that value into the printed message
instead of the hardcoded "1000000 ETH"; if the allocation lookup is not
desirable, simply omit the numeric claim to avoid displaying stale information.
---
Nitpick comments:
In `@crates/node/src/payload_service.rs`:
- Around line 318-380: The existing test try_build_span_has_expected_fields only
covers the empty-txpool path because it uses NoopTransactionPool; add a new
async #[tokio::test] that constructs an EvolveEnginePayloadBuilder (same setup:
provider, chain_spec, config, evm_config) but supplies a transaction-pool seeded
with at least one pending EvPooledTransaction (instead of NoopTransactionPool)
and calls the builder with RpcPayloadAttributes that have
attributes.transactions empty; assert the resulting payload used the pool
fallback inclusion (e.g., by checking the built block contains the seeded tx or
that fallback path in EvolveEnginePayloadBuilder was exercised). Use the same
unique symbols (EvolveEnginePayloadBuilder, EvPooledTransaction,
RpcPayloadAttributes, attributes.transactions) to locate code and mirror setup
from the existing test.
| fn parse_accounts(s: &str) -> Result<usize, String> { | ||
| let n: usize = s.parse().map_err(|e| format!("{e}"))?; | ||
| if (1..=20).contains(&n) { | ||
| Ok(n) | ||
| } else { | ||
| Err(format!("{n} is not in 1..=20")) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add unit tests for the new helper logic.
parse_accounts, derive_keys, and chain_id_from_genesis are core behavior in this new binary and should be covered with unit tests (range boundaries, deterministic derivation count/format, and chain ID extraction).
🧪 Minimal test scaffold
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn parse_accounts_enforces_bounds() {
+ assert!(parse_accounts("1").is_ok());
+ assert!(parse_accounts("20").is_ok());
+ assert!(parse_accounts("0").is_err());
+ assert!(parse_accounts("21").is_err());
+ }
+
+ #[test]
+ fn chain_id_is_readable_from_genesis() {
+ assert!(chain_id_from_genesis() > 0);
+ }
+}As per coding guidelines: "Write unit tests for individual components in Rust workspace".
Also applies to: 60-85, 132-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ev-dev/src/main.rs` around lines 26 - 33, Add unit tests covering the
helper functions parse_accounts, derive_keys, and chain_id_from_genesis: write
tests for parse_accounts that validate valid bounds (1 and 20) and out-of-range
errors (<1, >20) and invalid parse input; tests for derive_keys that assert
deterministic output, expected number of keypairs for a given account count and
format/length of keys; and tests for chain_id_from_genesis that extract the
correct chain id from representative genesis JSON blobs and handle
missing/invalid fields. Place tests in the same module (or a tests mod) so they
call parse_accounts, derive_keys, and chain_id_from_genesis directly and assert
both success and failure paths.
| let cli = match Cli::<EvolveChainSpecParser, EvolveArgs>::try_parse_from(args) { | ||
| Ok(cli) => cli, | ||
| Err(err) => { | ||
| eprintln!("{err}"); | ||
| std::process::exit(2); | ||
| } |
There was a problem hiding this comment.
Use return codes instead of std::process::exit to preserve cleanup.
Line 200 and Line 223 call std::process::exit, which skips destructors and can leak tempfile::NamedTempFile / tempfile::TempDir cleanup on failures.
♻️ Suggested change
-fn main() {
+fn main() -> std::process::ExitCode {
@@
let cli = match Cli::<EvolveChainSpecParser, EvolveArgs>::try_parse_from(args) {
Ok(cli) => cli,
Err(err) => {
eprintln!("{err}");
- std::process::exit(2);
+ return std::process::ExitCode::from(2);
}
};
@@
{
eprintln!("Error: {err:?}");
- std::process::exit(1);
+ return std::process::ExitCode::from(1);
}
+
+ std::process::ExitCode::SUCCESS
}Also applies to: 221-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ev-dev/src/main.rs` around lines 196 - 201, Replace direct
std::process::exit calls with returning an error code from main so destructors
run: change main's signature to return Result<(), i32> (or another suitable
Result type) and replace the std::process::exit(2) in the
Cli::<EvolveChainSpecParser, EvolveArgs>::try_parse_from error branch (and the
other std::process::exit calls around the later failure handling) with an early
return like return Err(2); (or map parsing errors to Err(code)). Ensure all call
sites compile with the new main return type and that temporary files
(tempfile::NamedTempFile / tempfile::TempDir) are allowed to drop normally.
| // Use transactions from Engine API attributes if provided, otherwise pull from the pool | ||
| // (e.g. in --dev mode where LocalMiner sends empty attributes). | ||
| let transactions = if attributes.transactions.is_empty() { | ||
| let pool_txs: Vec<TransactionSigned> = self | ||
| .pool | ||
| .pending_transactions() | ||
| .into_iter() | ||
| .map(|tx| tx.transaction.clone_into_consensus().into_inner()) | ||
| .collect(); | ||
| if !pool_txs.is_empty() { | ||
| info!( | ||
| pool_tx_count = pool_txs.len(), | ||
| "pulling transactions from pool" | ||
| ); | ||
| } | ||
| pool_txs | ||
| } else { | ||
| attributes.transactions.clone() | ||
| }; |
There was a problem hiding this comment.
Bound txpool fallback selection to prevent unbounded payload-build work.
Line 185-Line 190 clones all pending pool transactions into memory each build. Under a large pool this can inflate memory and delay block construction.
💡 Suggested guardrail
+ // Safety cap for fallback path to keep payload building bounded.
+ const MAX_FALLBACK_TXS: usize = 1024;
+
let transactions = if attributes.transactions.is_empty() {
let pool_txs: Vec<TransactionSigned> = self
.pool
.pending_transactions()
.into_iter()
+ .take(MAX_FALLBACK_TXS)
.map(|tx| tx.transaction.clone_into_consensus().into_inner())
.collect();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Use transactions from Engine API attributes if provided, otherwise pull from the pool | |
| // (e.g. in --dev mode where LocalMiner sends empty attributes). | |
| let transactions = if attributes.transactions.is_empty() { | |
| let pool_txs: Vec<TransactionSigned> = self | |
| .pool | |
| .pending_transactions() | |
| .into_iter() | |
| .map(|tx| tx.transaction.clone_into_consensus().into_inner()) | |
| .collect(); | |
| if !pool_txs.is_empty() { | |
| info!( | |
| pool_tx_count = pool_txs.len(), | |
| "pulling transactions from pool" | |
| ); | |
| } | |
| pool_txs | |
| } else { | |
| attributes.transactions.clone() | |
| }; | |
| // Safety cap for fallback path to keep payload building bounded. | |
| const MAX_FALLBACK_TXS: usize = 1024; | |
| // Use transactions from Engine API attributes if provided, otherwise pull from the pool | |
| // (e.g. in --dev mode where LocalMiner sends empty attributes). | |
| let transactions = if attributes.transactions.is_empty() { | |
| let pool_txs: Vec<TransactionSigned> = self | |
| .pool | |
| .pending_transactions() | |
| .into_iter() | |
| .take(MAX_FALLBACK_TXS) | |
| .map(|tx| tx.transaction.clone_into_consensus().into_inner()) | |
| .collect(); | |
| if !pool_txs.is_empty() { | |
| info!( | |
| pool_tx_count = pool_txs.len(), | |
| "pulling transactions from pool" | |
| ); | |
| } | |
| pool_txs | |
| } else { | |
| attributes.transactions.clone() | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/node/src/payload_service.rs` around lines 182 - 200, The current
fallback pulls all pending transactions into memory (attributes.transactions,
self.pool.pending_transactions()) which can blow up for large pools; change the
fallback in payload construction (the transactions variable / pool_txs) to cap
the number of pulled transactions by introducing a configurable constant (e.g.,
MAX_POOL_FALLBACK_TXS) and use an iterator .take(MAX_POOL_FALLBACK_TXS) or
equivalent to limit cloning, and log when truncation occurs so you still report
pool_tx_count and whether the pool was truncated; update any tests or configs to
reflect the new cap.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
bin/ev-dev/src/main.rs (2)
26-33: 🛠️ Refactor suggestion | 🟠 MajorAdd unit tests for helper logic before merge.
parse_accounts,derive_keys, andchain_id_from_genesisare core behavior but currently untested. Please add unit tests for bounds, deterministic derivation count/format, and chain-id parsing success/failure paths.As per coding guidelines: "Write unit tests for individual components in Rust workspace".
Also applies to: 60-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ev-dev/src/main.rs` around lines 26 - 33, Add unit tests for the helper functions parse_accounts, derive_keys, and chain_id_from_genesis: create tests that validate parse_accounts accepts values 1 and 20 and rejects 0 and 21 (checking error messages), verify derive_keys returns the expected number of deterministic keypairs and that output format (e.g., public/key lengths or canonical representation) matches the spec for given seed inputs, and test chain_id_from_genesis for both successful parsing of a valid genesis JSON and failure on malformed/missing chain_id; add these tests in the Rust crate's tests module (or #[cfg(test)] mod tests) and reference the functions by name so CI exercises bounds, determinism, and parsing success/failure paths.
196-201:⚠️ Potential issue | 🟠 MajorReturn exit codes instead of calling
std::process::exit.At Line 200 and Line 222,
std::process::exitbypasses destructors, sotempfile::NamedTempFile/tempfile::TempDircleanup is skipped on failure paths. ReturningExitCodefrommainkeeps error semantics and preserves cleanup.♻️ Suggested fix
-fn main() { +fn main() -> std::process::ExitCode { @@ let cli = match Cli::<EvolveChainSpecParser, EvolveArgs>::try_parse_from(args) { Ok(cli) => cli, Err(err) => { eprintln!("{err}"); - std::process::exit(2); + return std::process::ExitCode::from(2); } }; @@ }) { eprintln!("Error: {err:?}"); - std::process::exit(1); + return std::process::ExitCode::from(1); } + + std::process::ExitCode::SUCCESS }Also applies to: 221-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ev-dev/src/main.rs` around lines 196 - 201, The code currently calls std::process::exit(...) inside main (e.g., after Cli::<EvolveChainSpecParser, EvolveArgs>::try_parse_from(...) error handling and the other failure path around lines shown), which skips destructors; change main to return std::process::ExitCode (e.g., fn main() -> ExitCode or fn real_main() -> ExitCode called by main) and replace std::process::exit(2) / exit calls with early returns like return ExitCode::from(2) or ExitCode::FAILURE so cleanup (tempfile::NamedTempFile / tempfile::TempDir destructors) runs; update both locations that call std::process::exit (the error branch handling try_parse_from and the other failure branch around lines ~221-223).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/ev-dev/src/main.rs`:
- Around line 170-177: The code currently forces "--http.corsdomain" = "*" while
allowing dev_args.host/--ws.addr to bind non-loopback addresses; add a new
boolean CLI flag (e.g., allow_public_rpc) and use it to gate public bindings and
wildcard CORS: if dev_args.host or ws addr is non-loopback (not 127.0.0.1/::1)
then require allow_public_rpc to be true or else fail/start with an error;
otherwise set "--http.corsdomain" to a localhost-only value like
"http://127.0.0.1:*" by default; update the codepaths that build the args (the
block that appends "--http.addr", "--http.port", "--http.api",
"--http.corsdomain") to enforce these checks and use the new flag
(allow_public_rpc) to allow wildcard CORS and non-loopback binding only when
explicitly enabled.
---
Duplicate comments:
In `@bin/ev-dev/src/main.rs`:
- Around line 26-33: Add unit tests for the helper functions parse_accounts,
derive_keys, and chain_id_from_genesis: create tests that validate
parse_accounts accepts values 1 and 20 and rejects 0 and 21 (checking error
messages), verify derive_keys returns the expected number of deterministic
keypairs and that output format (e.g., public/key lengths or canonical
representation) matches the spec for given seed inputs, and test
chain_id_from_genesis for both successful parsing of a valid genesis JSON and
failure on malformed/missing chain_id; add these tests in the Rust crate's tests
module (or #[cfg(test)] mod tests) and reference the functions by name so CI
exercises bounds, determinism, and parsing success/failure paths.
- Around line 196-201: The code currently calls std::process::exit(...) inside
main (e.g., after Cli::<EvolveChainSpecParser, EvolveArgs>::try_parse_from(...)
error handling and the other failure path around lines shown), which skips
destructors; change main to return std::process::ExitCode (e.g., fn main() ->
ExitCode or fn real_main() -> ExitCode called by main) and replace
std::process::exit(2) / exit calls with early returns like return
ExitCode::from(2) or ExitCode::FAILURE so cleanup (tempfile::NamedTempFile /
tempfile::TempDir destructors) runs; update both locations that call
std::process::exit (the error branch handling try_parse_from and the other
failure branch around lines ~221-223).
| "--http.addr".to_string(), | ||
| dev_args.host.clone(), | ||
| "--http.port".to_string(), | ||
| dev_args.port.to_string(), | ||
| "--http.api".to_string(), | ||
| "eth,net,web3,txpool,debug,trace".to_string(), | ||
| "--http.corsdomain".to_string(), | ||
| "*".to_string(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the file exists and get its line count
if [ -f "bin/ev-dev/src/main.rs" ]; then
wc -l "bin/ev-dev/src/main.rs"
else
echo "File not found. Searching for main.rs in the repo..."
find . -name "main.rs" -type f | grep ev-dev
fiRepository: evstack/ev-reth
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Read the file to see lines 160-190 (context around 170-177 and 179-185)
cat -n "bin/ev-dev/src/main.rs" | sed -n '160,190p'Repository: evstack/ev-reth
Length of output: 1212
🏁 Script executed:
#!/bin/bash
# Also check the struct definition for EvDevArgs (host field, defaults)
cat -n "bin/ev-dev/src/main.rs" | head -100Repository: evstack/ev-reth
Length of output: 3980
🏁 Script executed:
#!/bin/bash
# Check for any other CORS or security-related configurations
rg -i "cors|security|loopback|public" "bin/ev-dev/src/main.rs"Repository: evstack/ev-reth
Length of output: 169
🏁 Script executed:
#!/bin/bash
# Check the full context of the main function to see if there's any validation
cat -n "bin/ev-dev/src/main.rs" | sed -n '100,160p'Repository: evstack/ev-reth
Length of output: 2419
🏁 Script executed:
#!/bin/bash
# Check the end of the file for any additional context
cat -n "bin/ev-dev/src/main.rs" | tail -50Repository: evstack/ev-reth
Length of output: 2089
Restrict CORS and gate non-loopback RPC binding to explicit opt-in.
The code unconditionally sets --http.corsdomain "*" while allowing user-controlled --http.addr and --ws.addr binding. Users can pass --host 0.0.0.0 to expose the RPC on all interfaces, combining wildcard CORS with unauthenticated debug, trace, and txpool methods—a security risk if deployed on shared networks or accidentally exposed to the internet, despite being a dev tool.
Add an --allow-public-rpc flag and validate that non-loopback binding requires explicit opt-in. Restrict CORS to localhost-only by default (e.g., "http://127.0.0.1:*" instead of "*").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ev-dev/src/main.rs` around lines 170 - 177, The code currently forces
"--http.corsdomain" = "*" while allowing dev_args.host/--ws.addr to bind
non-loopback addresses; add a new boolean CLI flag (e.g., allow_public_rpc) and
use it to gate public bindings and wildcard CORS: if dev_args.host or ws addr is
non-loopback (not 127.0.0.1/::1) then require allow_public_rpc to be true or
else fail/start with an error; otherwise set "--http.corsdomain" to a
localhost-only value like "http://127.0.0.1:*" by default; update the codepaths
that build the args (the block that appends "--http.addr", "--http.port",
"--http.api", "--http.corsdomain") to enforce these checks and use the new flag
(allow_public_rpc) to allow wildcard CORS and non-loopback binding only when
explicitly enabled.
3 participants
Description
Adds
ev-devbinary providing a one-command local development chain with pre-funded Hardhat accounts, similar to Anvil or Hardhat Node. Also fixes the payload builder to pull pending transactions from the txpool when Engine API attributes are empty, enablingcast sendand other RPC-submitted transactions to be included in blocks during--devmode. Includes DebugNode implementation for EvolveNode and dynamic chain ID parsing in the banner.Summary by CodeRabbit
New Features
Bug Fixes