feat: add ev-dev local chain and txpool transaction support#145
Open
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 PR Review Complete ✅I've thoroughly analyzed this PR that adds Review Checklist:
🎯 OverviewThis is a well-implemented PR that adds a valuable development tool ( ✅ Code Quality & Best Practices
🐛 Potential Issues
🚀 Performance Considerations
🔐 Security Analysis
🧪 Test Coverage
📋 Detailed Code ReviewNew
|
📝 WalkthroughWalkthroughThis PR introduces an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
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
Comment on lines
+46
to
+49
| /// Number of accounts to display (max 20) | ||
| #[arg(long, default_value_t = 10)] | ||
| accounts: usize, | ||
| } |
There was a problem hiding this comment.
accounts max is documented but not enforced at input boundary.
Current behavior silently clamps later; consider validating at parse-time (or adjust help text) so CLI behavior matches the contract.
🤖 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 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.
| .to_str() | ||
| .expect("valid path") | ||
| .to_string(); | ||
|
|
There was a problem hiding this comment.
cargo fmt is still needed in this file (CI lint failure).
Formatting mismatch is reported at Line 148.
🧰 Tools
🪛 GitHub Actions: lint
[error] 148-148: rustfmt formatting changes detected. Run 'cargo fmt' to fix formatting in this file.
🤖 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 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.
Comment on lines
+192
to
+194
| if let Err(err) = Cli::<EvolveChainSpecParser, EvolveArgs>::try_parse_from(args) | ||
| .expect("valid CLI args") | ||
| .run(|builder, _evolve_args| async move { |
There was a problem hiding this comment.
Avoid panic on nested CLI parse errors; return a user-facing error instead.
Using .expect("valid CLI args") can crash the launcher on invalid composed args instead of failing gracefully.
🛠️ Proposed error-handling 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);
}🤖 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 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.
| .pool | ||
| .pending_transactions() | ||
| .into_iter() | ||
| .map(|tx| tx.transaction.clone_into_consensus().into_inner()) |
There was a problem hiding this comment.
cargo fmt is still required in this file (CI is failing).
Lint already flags formatting mismatches at Line 189, Line 310, Line 319, and Line 411.
Also applies to: 310-310, 319-319, 411-411
🧰 Tools
🪛 GitHub Actions: lint
[error] 189-189: rustfmt formatting changes detected. Run 'cargo fmt' to fix formatting in this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/node/src/payload_service.rs` at 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Release Notes
New Features
Bug Fixes