fix: restrict txpool fallback to dev mode only#155
Open
Conversation
The payload builder was pulling transactions from the txpool whenever Engine API attributes had none, which could happen in production and break consensus (non-deterministic block contents). This change: - Adds a dev_mode boolean to EvolvePayloadBuilderConfig - Sets it to true when running with --dev flag (detected via ctx.is_dev()) - Guards the txpool fallback behind this flag In production (without --dev), the payload builder always uses only the transactions from Engine API attributes, preventing any non-deterministic behavior.
|
Claude finished @randygrok's task —— View job Code Review for txpool fallback restriction
|
|
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 Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant PayloadBuilder
participant EngineAPI_Attrs
participant TxPool
Caller->>PayloadBuilder: build_payload(ctx)
PayloadBuilder->>EngineAPI_Attrs: fetch attributes (may include txs)
alt attributes contain transactions
EngineAPI_Attrs-->>PayloadBuilder: return attributes.transactions
else attributes empty
PayloadBuilder->>PayloadBuilder: check dev_mode flag
alt dev_mode == true
PayloadBuilder->>TxPool: query pending transactions
TxPool-->>PayloadBuilder: return pool transactions
else dev_mode == false
EngineAPI_Attrs-->>PayloadBuilder: return empty tx list
end
end
PayloadBuilder-->>Caller: return built payload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
The payload builder was pulling transactions from the txpool whenever Engine API attributes had none, which could break consensus by producing non-deterministic block contents in production. Guard the txpool fallback behind ctx.is_dev() so it only activates when the node is running with --dev flag.
randygrok
force-pushed
the
randygrok/fix-txpool-dev-only
branch
from
1844841 to
75746fa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/node/src/payload_service.rs (1)
184-203:⚠️ Potential issue | 🟠 MajorAdd explicit behavior tests for tx source selection in dev vs non-dev mode.
This branch is consensus-sensitive but current coverage in this file only validates tracing spans. Please add tests that assert: non-dev never pulls pool txs, dev pulls pool txs only when attributes are empty, and provided attribute txs always take precedence.
Based on learnings: Applies to crates/tests/**/*.rs : Write integration tests in crates/tests/ covering Engine API interactions, payload building, state execution, 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 184 - 203, Add integration tests under crates/tests that exercise the payload selection logic in payload_service.rs: create tests that call the payload-building path (the method that uses self.config.dev_mode and attributes.transactions) with a mocked pool (pool.pending_transactions returning TransactionSigned items) and varying Engine API attributes to assert three cases: (1) non-dev mode never pulls pool txs even if attributes.transactions is empty, (2) dev mode pulls pool txs only when attributes.transactions is empty, and (3) when attributes.transactions is non-empty those provided txs always take precedence over pool.pending_transactions. Use the same types referenced in the file (attributes.transactions, self.config.dev_mode, pool.pending_transactions, TransactionSigned) and implement setup/teardown to control dev_mode and pool contents so the assertions verify the chosen transactions match the expected source.
🤖 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/config.rs`:
- Around line 125-127: CI is failing because rustfmt formatting in
crates/node/src/config.rs is out of date; run the workspace formatter (just fmt)
to apply rustfmt to this file (and the workspace), review the resulting changes,
and commit the formatted file so the lint passes; ensure you run `just fmt`
locally and include the updated crates/node/src/config.rs in your commit.
---
Outside diff comments:
In `@crates/node/src/payload_service.rs`:
- Around line 184-203: Add integration tests under crates/tests that exercise
the payload selection logic in payload_service.rs: create tests that call the
payload-building path (the method that uses self.config.dev_mode and
attributes.transactions) with a mocked pool (pool.pending_transactions returning
TransactionSigned items) and varying Engine API attributes to assert three
cases: (1) non-dev mode never pulls pool txs even if attributes.transactions is
empty, (2) dev mode pulls pool txs only when attributes.transactions is empty,
and (3) when attributes.transactions is non-empty those provided txs always take
precedence over pool.pending_transactions. Use the same types referenced in the
file (attributes.transactions, self.config.dev_mode, pool.pending_transactions,
TransactionSigned) and implement setup/teardown to control dev_mode and pool
contents so the assertions verify the chosen transactions match the expected
source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce714976-d8fe-4f2e-b4b5-abd420cda51d
📒 Files selected for processing (2)
crates/node/src/config.rscrates/node/src/payload_service.rs
crates/node/src/config.rs
Outdated
Comment on lines
+125
to
+127
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
Formatting check is currently blocking CI.
rustfmt is failing in lint; please run formatter before merge.
As per coding guidelines: Use just fmt for code formatting in Rust workspace.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/node/src/config.rs` around lines 125 - 127, CI is failing because
rustfmt formatting in crates/node/src/config.rs is out of date; run the
workspace formatter (just fmt) to apply rustfmt to this file (and the
workspace), review the resulting changes, and commit the formatted file so the
lint passes; ensure you run `just fmt` locally and include the updated
crates/node/src/config.rs in your commit.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/node/src/config.rs (1)
121-123:⚠️ Potential issue | 🟡 MinorRun workspace formatting to unblock CI lint.
cargo fmt --all --checkis failing, and this region is part of the reported formatting drift. Please runjust fmtand commit the formatted output.As per coding guidelines: Use
just fmtfor code formatting in Rust workspace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/config.rs` around lines 121 - 123, Formatting drift detected in config.rs causing CI lint to fail; run the workspace formatter and commit the results. Execute the workspace format command (just fmt), verify cargo fmt --all --check passes, and commit the updated formatting for crates/node/src/config.rs (and any other modified files) so CI lint is unblocked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/node/src/config.rs`:
- Around line 121-123: Formatting drift detected in config.rs causing CI lint to
fail; run the workspace formatter and commit the results. Execute the workspace
format command (just fmt), verify cargo fmt --all --check passes, and commit the
updated formatting for crates/node/src/config.rs (and any other modified files)
so CI lint is unblocked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d190401-3eb3-4671-b012-9853b4dc8148
📒 Files selected for processing (2)
crates/node/src/config.rscrates/node/src/payload_service.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/node/src/payload_service.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/tests/src/e2e_tests.rs (1)
2057-2089: Tighten assertion to prove the fallback used the intended txpool transaction.The check on Line 2086 only asserts non-empty output. Strengthen it by preserving
raw_txand asserting the built payload contains that exact transaction (or its hash).Proposed assertion hardening
- let raw_tx = TransactionTestContext::transfer_tx_bytes(chain_id, sender).await; + let raw_tx = TransactionTestContext::transfer_tx_bytes(chain_id, sender).await; + let expected_tx = raw_tx.clone(); EthApiClient::<TransactionRequest, Transaction, Block, Receipt, Header, Bytes>::send_raw_transaction( &env.node_clients[0].rpc, raw_tx, ) .await?; @@ assert!( !block_txs.is_empty(), "dev mode should pull transaction from txpool when attributes are empty" ); + assert!( + block_txs.iter().any(|tx| tx == &expected_tx), + "expected block to include the tx submitted to txpool" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tests/src/e2e_tests.rs` around lines 2057 - 2089, The test currently only checks that payload_envelope.execution_payload.payload_inner.payload_inner.transactions (block_txs) is non-empty; instead preserve raw_tx and assert that the payload contains that exact transaction (or its hash) to prove the txpool fallback was used. Locate the raw_tx variable produced by TransactionTestContext::transfer_tx_bytes and after build_block_with_transactions completes, either (a) compare the raw_tx byte sequence against one of the entries in block_txs or (b) compute the transaction hash from raw_tx and assert that an entry in block_txs matches that hash; add a clear assert that fails if the exact raw_tx (or its hash) is not found. Use the existing symbols raw_tx, block_txs, and build_block_with_transactions to find where to add the check.
🤖 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/tests/src/e2e_tests.rs`:
- Around line 2034-2092: Add a non-dev counterpart of
test_e2e_dev_mode_txpool_fallback that uses the same setup but sets
with_dev_mode(false) (e.g., name it test_e2e_non_dev_txpool_no_fallback), send a
signed transaction into the txpool exactly as in
test_e2e_dev_mode_txpool_fallback, call build_block_with_transactions the same
way, then assert that the resulting payload's transactions
(payload_envelope.execution_payload.payload_inner.payload_inner.transactions)
are empty to verify txpool fallback is disabled in production mode; reuse
Setup::<EvolveEngineTypes>, Wallet::new(...).with_chain_id(...).wallet_gen(),
EthApiClient::send_raw_transaction and build_block_with_transactions to mirror
the dev test flow.
---
Nitpick comments:
In `@crates/tests/src/e2e_tests.rs`:
- Around line 2057-2089: The test currently only checks that
payload_envelope.execution_payload.payload_inner.payload_inner.transactions
(block_txs) is non-empty; instead preserve raw_tx and assert that the payload
contains that exact transaction (or its hash) to prove the txpool fallback was
used. Locate the raw_tx variable produced by
TransactionTestContext::transfer_tx_bytes and after
build_block_with_transactions completes, either (a) compare the raw_tx byte
sequence against one of the entries in block_txs or (b) compute the transaction
hash from raw_tx and assert that an entry in block_txs matches that hash; add a
clear assert that fails if the exact raw_tx (or its hash) is not found. Use the
existing symbols raw_tx, block_txs, and build_block_with_transactions to find
where to add the check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71dff13a-86e4-4182-aba8-945b36343028
📒 Files selected for processing (2)
crates/node/src/config.rscrates/tests/src/e2e_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/node/src/config.rs
Comment on lines
+2034
to
+2092
| async fn test_e2e_dev_mode_txpool_fallback() -> Result<()> { | ||
| reth_tracing::init_test_tracing(); | ||
|
|
||
| let chain_spec = create_test_chain_spec(); | ||
| let chain_id = chain_spec.chain().id(); | ||
|
|
||
| let mut setup = Setup::<EvolveEngineTypes>::default() | ||
| .with_chain_spec(chain_spec) | ||
| .with_network(NetworkSetup::single_node()) | ||
| .with_dev_mode(true) | ||
| .with_tree_config(e2e_test_tree_config()); | ||
|
|
||
| let mut env = Environment::<EvolveEngineTypes>::default(); | ||
| setup.apply::<EvolveNode>(&mut env).await?; | ||
|
|
||
| let parent_block = env.node_clients[0] | ||
| .get_block_by_number(BlockNumberOrTag::Latest) | ||
| .await? | ||
| .expect("parent block should exist"); | ||
| let mut parent_hash = parent_block.header.hash; | ||
| let mut parent_timestamp = parent_block.header.inner.timestamp; | ||
| let mut parent_number = parent_block.header.inner.number; | ||
|
|
||
| // Create a signed transaction and send it to the txpool | ||
| let wallets = Wallet::new(1).with_chain_id(chain_id).wallet_gen(); | ||
| let sender = wallets.into_iter().next().unwrap(); | ||
| let raw_tx = TransactionTestContext::transfer_tx_bytes(chain_id, sender).await; | ||
|
|
||
| EthApiClient::<TransactionRequest, Transaction, Block, Receipt, Header, Bytes>::send_raw_transaction( | ||
| &env.node_clients[0].rpc, | ||
| raw_tx, | ||
| ) | ||
| .await?; | ||
|
|
||
| // Build a block with empty transactions via Engine API. | ||
| // In dev mode, the payload builder pulls from the txpool. | ||
| let payload_envelope = build_block_with_transactions( | ||
| &mut env, | ||
| &mut parent_hash, | ||
| &mut parent_number, | ||
| &mut parent_timestamp, | ||
| None, | ||
| vec![], | ||
| Address::random(), | ||
| ) | ||
| .await?; | ||
|
|
||
| let block_txs = &payload_envelope | ||
| .execution_payload | ||
| .payload_inner | ||
| .payload_inner | ||
| .transactions; | ||
| assert!( | ||
| !block_txs.is_empty(), | ||
| "dev mode should pull transaction from txpool when attributes are empty" | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Add a non-dev regression test for the safety-critical branch.
This test validates the dev-mode path, but it does not verify the production behavior (with_dev_mode(false)) where txpool fallback must stay disabled. Without that companion assertion, the core safety guarantee from this PR can regress silently. See Line 2043 and Line 2068-Line 2089.
Proposed test addition
+#[tokio::test(flavor = "multi_thread")]
+async fn test_e2e_non_dev_mode_does_not_txpool_fallback() -> Result<()> {
+ reth_tracing::init_test_tracing();
+
+ let chain_spec = create_test_chain_spec();
+ let chain_id = chain_spec.chain().id();
+
+ let mut setup = Setup::<EvolveEngineTypes>::default()
+ .with_chain_spec(chain_spec)
+ .with_network(NetworkSetup::single_node())
+ .with_dev_mode(false)
+ .with_tree_config(e2e_test_tree_config());
+
+ let mut env = Environment::<EvolveEngineTypes>::default();
+ setup.apply::<EvolveNode>(&mut env).await?;
+
+ let parent_block = env.node_clients[0]
+ .get_block_by_number(BlockNumberOrTag::Latest)
+ .await?
+ .expect("parent block should exist");
+ let mut parent_hash = parent_block.header.hash;
+ let mut parent_timestamp = parent_block.header.inner.timestamp;
+ let mut parent_number = parent_block.header.inner.number;
+
+ let wallets = Wallet::new(1).with_chain_id(chain_id).wallet_gen();
+ let sender = wallets.into_iter().next().expect("wallet exists");
+ let raw_tx = TransactionTestContext::transfer_tx_bytes(chain_id, sender).await;
+ EthApiClient::<TransactionRequest, Transaction, Block, Receipt, Header, Bytes>::send_raw_transaction(
+ &env.node_clients[0].rpc,
+ raw_tx,
+ )
+ .await?;
+
+ let payload_envelope = build_block_with_transactions(
+ &mut env,
+ &mut parent_hash,
+ &mut parent_number,
+ &mut parent_timestamp,
+ None,
+ vec![],
+ Address::ZERO,
+ )
+ .await?;
+
+ let block_txs = &payload_envelope
+ .execution_payload
+ .payload_inner
+ .payload_inner
+ .transactions;
+ assert!(
+ block_txs.is_empty(),
+ "non-dev mode must not pull from txpool when attributes are empty"
+ );
+
+ Ok(())
+}Based on learnings: Transactions should bypass the mempool and be submitted directly via Engine API payload attributes.
📝 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.
Suggested change
| async fn test_e2e_dev_mode_txpool_fallback() -> Result<()> { | |
| reth_tracing::init_test_tracing(); | |
| let chain_spec = create_test_chain_spec(); | |
| let chain_id = chain_spec.chain().id(); | |
| let mut setup = Setup::<EvolveEngineTypes>::default() | |
| .with_chain_spec(chain_spec) | |
| .with_network(NetworkSetup::single_node()) | |
| .with_dev_mode(true) | |
| .with_tree_config(e2e_test_tree_config()); | |
| let mut env = Environment::<EvolveEngineTypes>::default(); | |
| setup.apply::<EvolveNode>(&mut env).await?; | |
| let parent_block = env.node_clients[0] | |
| .get_block_by_number(BlockNumberOrTag::Latest) | |
| .await? | |
| .expect("parent block should exist"); | |
| let mut parent_hash = parent_block.header.hash; | |
| let mut parent_timestamp = parent_block.header.inner.timestamp; | |
| let mut parent_number = parent_block.header.inner.number; | |
| // Create a signed transaction and send it to the txpool | |
| let wallets = Wallet::new(1).with_chain_id(chain_id).wallet_gen(); | |
| let sender = wallets.into_iter().next().unwrap(); | |
| let raw_tx = TransactionTestContext::transfer_tx_bytes(chain_id, sender).await; | |
| EthApiClient::<TransactionRequest, Transaction, Block, Receipt, Header, Bytes>::send_raw_transaction( | |
| &env.node_clients[0].rpc, | |
| raw_tx, | |
| ) | |
| .await?; | |
| // Build a block with empty transactions via Engine API. | |
| // In dev mode, the payload builder pulls from the txpool. | |
| let payload_envelope = build_block_with_transactions( | |
| &mut env, | |
| &mut parent_hash, | |
| &mut parent_number, | |
| &mut parent_timestamp, | |
| None, | |
| vec![], | |
| Address::random(), | |
| ) | |
| .await?; | |
| let block_txs = &payload_envelope | |
| .execution_payload | |
| .payload_inner | |
| .payload_inner | |
| .transactions; | |
| assert!( | |
| !block_txs.is_empty(), | |
| "dev mode should pull transaction from txpool when attributes are empty" | |
| ); | |
| Ok(()) | |
| } | |
| async fn test_e2e_non_dev_mode_does_not_txpool_fallback() -> Result<()> { | |
| reth_tracing::init_test_tracing(); | |
| let chain_spec = create_test_chain_spec(); | |
| let chain_id = chain_spec.chain().id(); | |
| let mut setup = Setup::<EvolveEngineTypes>::default() | |
| .with_chain_spec(chain_spec) | |
| .with_network(NetworkSetup::single_node()) | |
| .with_dev_mode(false) | |
| .with_tree_config(e2e_test_tree_config()); | |
| let mut env = Environment::<EvolveEngineTypes>::default(); | |
| setup.apply::<EvolveNode>(&mut env).await?; | |
| let parent_block = env.node_clients[0] | |
| .get_block_by_number(BlockNumberOrTag::Latest) | |
| .await? | |
| .expect("parent block should exist"); | |
| let mut parent_hash = parent_block.header.hash; | |
| let mut parent_timestamp = parent_block.header.inner.timestamp; | |
| let mut parent_number = parent_block.header.inner.number; | |
| let wallets = Wallet::new(1).with_chain_id(chain_id).wallet_gen(); | |
| let sender = wallets.into_iter().next().expect("wallet exists"); | |
| let raw_tx = TransactionTestContext::transfer_tx_bytes(chain_id, sender).await; | |
| EthApiClient::<TransactionRequest, Transaction, Block, Receipt, Header, Bytes>::send_raw_transaction( | |
| &env.node_clients[0].rpc, | |
| raw_tx, | |
| ) | |
| .await?; | |
| let payload_envelope = build_block_with_transactions( | |
| &mut env, | |
| &mut parent_hash, | |
| &mut parent_number, | |
| &mut parent_timestamp, | |
| None, | |
| vec![], | |
| Address::ZERO, | |
| ) | |
| .await?; | |
| let block_txs = &payload_envelope | |
| .execution_payload | |
| .payload_inner | |
| .payload_inner | |
| .transactions; | |
| assert!( | |
| block_txs.is_empty(), | |
| "non-dev mode must not pull from txpool when attributes are empty" | |
| ); | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/tests/src/e2e_tests.rs` around lines 2034 - 2092, Add a non-dev
counterpart of test_e2e_dev_mode_txpool_fallback that uses the same setup but
sets with_dev_mode(false) (e.g., name it test_e2e_non_dev_txpool_no_fallback),
send a signed transaction into the txpool exactly as in
test_e2e_dev_mode_txpool_fallback, call build_block_with_transactions the same
way, then assert that the resulting payload's transactions
(payload_envelope.execution_payload.payload_inner.payload_inner.transactions)
are empty to verify txpool fallback is disabled in production mode; reuse
Setup::<EvolveEngineTypes>, Wallet::new(...).with_chain_id(...).wallet_gen(),
EthApiClient::send_raw_transaction and build_block_with_transactions to mirror
the dev test flow.
tac0turtle
reviewed
Mar 4, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 387-388: Add a unit test in crates/node/src/payload_service.rs
that exercises the new branch in try_build by constructing the relevant struct
with dev_mode: true and injecting a mocked pool that returns a known transaction
source, then call try_build and assert the transaction sourcing came from the
pool; also add a complementary test with dev_mode: false that injects mocked
Engine API attributes and assert try_build used those attributes. Target the
symbols try_build and the struct/constructor that accepts dev_mode so the test
explicitly toggles dev_mode, injects test doubles for the pool and Engine API,
and asserts the correct branch behavior (pool when dev_mode is true, Engine API
attributes when false).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c95232bb-e076-4c0f-9851-8a8aecafaf10
📒 Files selected for processing (2)
crates/node/src/payload_service.rscrates/tests/src/e2e_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/tests/src/e2e_tests.rs
Comment on lines
+387
to
388
| dev_mode: false, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add a unit test that exercises dev_mode: true transaction sourcing.
Both updated unit tests pin dev_mode: false; the new branch in try_build (Line 187) is not unit-covered here. Please add a focused unit test asserting dev mode pulls from pool and production mode uses Engine API attributes.
As per coding guidelines: "Write unit tests for individual components in Rust workspace."
Also applies to: 478-479
🤖 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 387 - 388, Add a unit test
in crates/node/src/payload_service.rs that exercises the new branch in try_build
by constructing the relevant struct with dev_mode: true and injecting a mocked
pool that returns a known transaction source, then call try_build and assert the
transaction sourcing came from the pool; also add a complementary test with
dev_mode: false that injects mocked Engine API attributes and assert try_build
used those attributes. Target the symbols try_build and the struct/constructor
that accepts dev_mode so the test explicitly toggles dev_mode, injects test
doubles for the pool and Engine API, and asserts the correct branch behavior
(pool when dev_mode is true, Engine API attributes when false).
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/tests/src/e2e_tests.rs (1)
2034-2092:⚠️ Potential issue | 🟠 MajorAdd the non-dev regression counterpart for this safety-critical behavior.
Line 2034 adds the positive dev-mode fallback test, but there is still no direct negative-path assertion that
with_dev_mode(false)+ empty attributes + populated txpool yields an empty payload tx list. That companion test is needed to lock in the consensus-safety guarantee.Based on learnings: Transactions should bypass the mempool and be submitted directly via Engine API payload attributes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tests/src/e2e_tests.rs` around lines 2034 - 2092, Add a new negative-path test mirroring test_e2e_dev_mode_txpool_fallback but with dev mode disabled: create a test (e.g., test_e2e_non_dev_mode_txpool_no_fallback) that sets Setup::<EvolveEngineTypes>::default().with_dev_mode(false), seeds the txpool by sending a signed raw transaction using EthApiClient::send_raw_transaction (same Wallet/TransactionTestContext code), then call build_block_with_transactions with empty attributes and assert that payload_envelope.execution_payload.payload_inner.payload_inner.transactions is empty; reuse the same setup/Environment, parent_block retrieval, and build_block_with_transactions call patterns to ensure the non-dev path guarantees no txpool fallback.
🧹 Nitpick comments (1)
crates/tests/src/e2e_tests.rs (1)
2081-2089: Make the assertion specific to the submitted tx, not just non-empty.
assert!(!block_txs.is_empty())can pass on unrelated pending txs. Prefer asserting the tx submitted at Line 2062 is actually included.Proposed tightening
- EthApiClient::<TransactionRequest, Transaction, Block, Receipt, Header, Bytes>::send_raw_transaction( + let submitted_hash = EthApiClient::<TransactionRequest, Transaction, Block, Receipt, Header, Bytes>::send_raw_transaction( &env.node_clients[0].rpc, - raw_tx, + raw_tx, ) .await?; @@ - assert!( - !block_txs.is_empty(), - "dev mode should pull transaction from txpool when attributes are empty" - ); + let contains_submitted_tx = block_txs.iter().any(|raw| { + let mut slice = raw.as_ref(); + TxEnvelope::decode_2718(&mut slice) + .map(|env| *env.tx_hash() == submitted_hash) + .unwrap_or(false) + }); + assert!( + contains_submitted_tx, + "dev mode fallback block should include the submitted txpool transaction" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tests/src/e2e_tests.rs` around lines 2081 - 2089, The test currently only checks block_txs is non-empty, which can pass for unrelated pending transactions; instead assert that the specific transaction submitted earlier in this test is present in payload_envelope.execution_payload.payload_inner.payload_inner.transactions. Locate the variable used to submit the tx in this test (the submitted tx value used when calling the mempool/submit helper) and replace the assert with a membership/assert-equality check comparing that submitted transaction (serialized bytes or its hash) against block_txs (e.g., assert!(block_txs.contains(&submitted_tx) or assert_eq!(block_txs.iter().find(...), Some(&submitted_tx))). Ensure you compare the same representation (raw tx bytes or computed tx hash) as stored in transactions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/tests/src/e2e_tests.rs`:
- Around line 2034-2092: Add a new negative-path test mirroring
test_e2e_dev_mode_txpool_fallback but with dev mode disabled: create a test
(e.g., test_e2e_non_dev_mode_txpool_no_fallback) that sets
Setup::<EvolveEngineTypes>::default().with_dev_mode(false), seeds the txpool by
sending a signed raw transaction using EthApiClient::send_raw_transaction (same
Wallet/TransactionTestContext code), then call build_block_with_transactions
with empty attributes and assert that
payload_envelope.execution_payload.payload_inner.payload_inner.transactions is
empty; reuse the same setup/Environment, parent_block retrieval, and
build_block_with_transactions call patterns to ensure the non-dev path
guarantees no txpool fallback.
---
Nitpick comments:
In `@crates/tests/src/e2e_tests.rs`:
- Around line 2081-2089: The test currently only checks block_txs is non-empty,
which can pass for unrelated pending transactions; instead assert that the
specific transaction submitted earlier in this test is present in
payload_envelope.execution_payload.payload_inner.payload_inner.transactions.
Locate the variable used to submit the tx in this test (the submitted tx value
used when calling the mempool/submit helper) and replace the assert with a
membership/assert-equality check comparing that submitted transaction
(serialized bytes or its hash) against block_txs (e.g.,
assert!(block_txs.contains(&submitted_tx) or
assert_eq!(block_txs.iter().find(...), Some(&submitted_tx))). Ensure you compare
the same representation (raw tx bytes or computed tx hash) as stored in
transactions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8d1e90d-fc3a-4c4a-9363-8a433b3b091d
📒 Files selected for processing (3)
crates/tests/src/e2e_tests.rscrates/tests/src/test_deploy_allowlist.rscrates/tests/src/test_evolve_engine_api.rs
✅ Files skipped from review due to trivial changes (1)
- crates/tests/src/test_deploy_allowlist.rs
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
2 participants
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.
The payload builder was pulling transactions from the txpool whenever Engine API attributes had none, which could happen in production and break consensus (non-deterministic block contents).
This change:
In production (without --dev), the payload builder always uses only the transactions from Engine API attributes, preventing any non-deterministic behavior.
Summary by CodeRabbit
New Features
Improvements
Tests