refactor: execute PoL in normal transaction pipeline - #271
Conversation
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
✨ 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 |
Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
f27efa3 to
d5fe0db
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors Proof-of-Liquidity (PoL) execution to run as transaction #0 in the normal transaction pipeline during payload building, removing prior PoL-specific execution/import/assembly workarounds (including the custom block builder sender fix).
Changes:
- Execute PoL as tx #0 via the standard
execute_transactionpath during payload building; remove executor-side PoL special-casing. - Update block assembly to stop injecting PoL and instead validate PoL is already present at index 0 post-Prague1.
- Remove the now-unneeded Berachain block builder wrapper and associated dependency entries.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/node/evm/mod.rs | Removes the PoL-specific builder module from the EVM node module. |
| src/node/evm/executor.rs | Removes PoL system-call execution/dummy results/commit skips; keeps only Prague1 proposer pubkey validation in pre-exec. |
| src/node/evm/error.rs | Removes the error variant that was only needed for the old PoL execution path. |
| src/node/evm/config.rs | Uses BasicBlockBuilder directly (removes wrapper builder). |
| src/node/evm/builder.rs | Deletes the PoL sender/tx mismatch workaround builder wrapper and its tests. |
| src/node/evm/assembler.rs | Stops injecting PoL; validates PoL is already tx #0 post-Prague1. |
| src/evm/mod.rs | Ensures PoL system-call txs consume zero gas even on revert/halt. |
| src/engine/builder.rs | Executes PoL as tx #0 during payload building (post-Prague1). |
| Cargo.toml | Removes a dependency no longer needed after deleting the builder wrapper. |
| Cargo.lock | Updates lockfile to reflect dependency removal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Execute PoL as tx #0 post-Prague1. prev_proposer_pubkey was validated in | ||
| // apply_pre_execution_changes. | ||
| if chain_spec.is_prague1_active_at_timestamp(attributes.timestamp()) { | ||
| let prev_proposer_pubkey = attributes | ||
| .prev_proposer_pubkey | ||
| .expect("prev_proposer_pubkey validated by validate_proposer_pubkey_prague1"); |
There was a problem hiding this comment.
opened #272
Fixed the expect, it now returns a MissingProposerPubkey error instead of panicking.
The RLP length isn't something this PR changed. PoL was never counted in block_transactions_rlp_length before either, and the check after finish() already rejects any block over MAX_RLP_BLOCK_SIZE. PoL is only ~150-200 bytes and the loop already keeps a 1024 buffer, so leaving that as is.
| // Post-Prague1, PoL must already be tx #0 with a matching receipt. | ||
| if self.chain_spec.is_prague1_active_at_timestamp(timestamp) { | ||
| let prev_proposer_pubkey = ctx.prev_proposer_pubkey.unwrap(); | ||
|
|
||
| // Synthesize POL transaction and prepend to transactions list | ||
| let base_fee = evm_env.block_env.basefee(); | ||
| let pol_transaction = create_pol_transaction( | ||
| self.chain_spec.clone(), | ||
| prev_proposer_pubkey, | ||
| evm_env.block_env.number(), | ||
| base_fee, | ||
| )?; | ||
|
|
||
| transactions.insert(0, pol_transaction); | ||
|
|
||
| // Validate that we have receipts after POL transaction execution | ||
| if receipts.is_empty() { | ||
| return Err(BerachainExecutionError::MissingPolReceipts.into()); | ||
| } |
There was a problem hiding this comment.
#272
Updated the comment to match what's actually checked (PoL at index 0, receipts not empty).
Not adding receipt/tx alignment checks here since receipts are built in order during execution and a mismatch would fail on the root computation right below.
Closes #64.
Summary
PoL was previously executed in
apply_pre_execution_changes, then handled again during import with dummy results and commit skips, and injected into the block during assembly. That required extra workarounds, includingBerachainBlockBuilderandfix_pol_senders(#129).This PR moves PoL into the normal transaction pipeline:
execute_transactionduring payload buildingBerachainBlockBuilderandfix_pol_senderstransact_rawTest plan
cargo buildcargo +nightly clippy --all-targets --all-features -- -D warningscargo nextest run --locked