feat(actions): Wire Production Payload Builder#1861
Conversation
refcell
added
K-feature
refcell
added
K-feature
🟡 Heimdall Review Status
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
refcell
force-pushed
the
feat/actions-wire-production-payload-builder
branch
from
cb494fe to
4b29e90
Compare
refcell
force-pushed
the
feat/actions-wire-production-payload-builder
branch
from
2408906 to
a4b6082
Compare
| if let Some(executed) = built.executed_block() { | ||
| let execution_output = executed.execution_output; |
There was a problem hiding this comment.
executed.execution_output is an Arc<ExecutionOutput>, and on the next lines execution_output.state.clone() and execution_output.result.receipts.clone() deep-clone the state bundle and receipts. Since ExecutionOutcome owns these fields by value, this is unavoidable with the current API — but built is not used after this branch, so consider using Arc::try_unwrap on the execution output to take ownership without cloning:
let execution_output = Arc::try_unwrap(executed.execution_output)
.unwrap_or_else(|arc| (*arc).clone());This avoids the redundant clones when the Arc refcount is 1 (which it will be for non-shared payloads).
| // In derivation mode (`TestRollupNode`) the payload is constructed with a zeroed | ||
| // `block_hash` placeholder because the engine is expected to fill it in. When we see | ||
| // B256::ZERO we treat the block-number lookup alone as sufficient — the block was | ||
| // pre-built by the sequencer and its state is already committed to the DB. | ||
| if let Some(existing) = inner.executed_headers.get(&payload.block_number) { | ||
| let existing_hash = existing.hash_slow(); | ||
| if payload.block_hash == B256::ZERO || payload.block_hash == existing_hash { |
There was a problem hiding this comment.
The block_hash == B256::ZERO fallback means that any external caller (e.g. a future test utility) that happens to pass B256::ZERO as the block hash will skip execution entirely and return a stale header from a different block at the same number.
If the intent is specifically to support TestRollupNode's derivation path, consider using a dedicated flag or method (execute_from_attrs, which this PR already provides) rather than overloading B256::ZERO as a sentinel. The derivation node already uses execute_from_attrs in execute_and_advance, so this fallback in execute_v1_inner may be dead code in practice.
| // 8. Update L2 provider state for next iteration. | ||
| self.l2_provider.insert_block(self.head); | ||
| // The system config is updated via the attributes builder's internal | ||
| // L2 chain provider when the epoch changes. For the sequencer's | ||
| // L2 provider copy, inherit the genesis config — the attributes | ||
| // builder reads the correct config from its own provider clone. | ||
|
|
||
| Ok(block) | ||
| } |
There was a problem hiding this comment.
The L2 provider is updated with the new block (line 584), but the system config is never updated for new epochs. The comment says "the attributes builder reads the correct config from its own provider clone" — but StatefulAttributesBuilder takes l2_provider.clone() at construction time (harness.rs:277). If l2_provider is cloned, it's a separate instance, and the sequencer's self.l2_provider updates are not visible to the attributes builder's clone.
This works today because ActionL2ChainProvider::system_config_by_number walks back from number to find the nearest config, which always finds the genesis config. But this means system config updates from L1 deposits (e.g. gas limit changes, fee scalar updates) are never reflected in the sequencer's attribute construction. If a test ever relies on mid-sequence system config changes, it will silently use stale values.
| let genesis_header = inner.chain_spec.genesis_header().clone(); | ||
| let genesis_hash = genesis_header.hash_slow(); | ||
| (genesis_hash, genesis_header) | ||
| }); |
There was a problem hiding this comment.
Silent genesis fallback for unknown parent hashes: when parent_hash doesn't match any entry in executed_headers, build_and_commit unconditionally falls back to the genesis header. This is intentional for the first block (where the rollup config has B256::ZERO as genesis hash), but it means that any unrecognized parent hash — including one from a bug in the caller or a missed block — silently builds on top of genesis rather than returning an error.
Consider making this fallback explicit with a genesis-hash check:
| }); | |
| .unwrap_or_else(|| { | |
| // Genesis case: derive the real hash from the chain spec genesis header so the | |
| // builder can locate the committed state in the DB. | |
| let genesis_header = inner.chain_spec.genesis_header().clone(); | |
| let genesis_hash = genesis_header.hash_slow(); | |
| debug_assert!( | |
| parent_hash == B256::ZERO || parent_hash == genesis_hash, | |
| "unknown parent hash {parent_hash}, expected genesis {genesis_hash}", | |
| ); | |
| (genesis_hash, genesis_header) | |
| }); |
This would catch misuse in tests without silently building an incorrect chain.
Review SummaryThis PR replaces the bespoke New findingSilent genesis fallback for unknown parent hashes ( Notes on prior review commentsThe existing comment about "partial headers making |
2 participants
Summary
Replaces the bespoke
StatefulL2ExecutorinsideActionEngineClientwith the realOpPayloadBuilderstack.L2Sequencernow uses the productionL1OriginSelectorandStatefulAttributesBuilderto produce payload attributes, then delegates block building toActionEngineClientviastart_build_block/get_sealed_payload/insert_unsafe_payload. This eliminates the divergence between the test harness and the production block-building code path, so action tests exercise the real execution pipeline end-to-end.get_next_payload_l1_originandbuild_attributesonPayloadBuilderare madepubto support this wiring.