fix: wrap block builder to fix PoL sender recovery #203
Conversation
BerachainBlockBuilder wraps the inner block builder to fix sender count mismatch for PoL transactions. The reth block builder doesn't recover senders for transactions that bypass the pool (system txs), so we extract senders from PoL transactions directly. Invariants are strictly enforced based on Prague1 activation: - Pre-Prague1: num_transactions == num_senders (no PoL injection) - Post-Prague1: num_transactions > num_senders (PoL always injected) Violations in either direction indicate a bug in block assembly. Fixes: #129
WalkthroughAdds reth-storage-api dependency and introduces BerachainBlockBuilder, a wrapper around generic BlockBuilder that fixes sender/transaction mismatches from PoL injections. Implements sender reconstruction logic across Prague1 activation boundaries and exposes create_block_builder method in EVM configuration to integrate the new builder into block construction. Changes
Sequence Diagram(s)sequenceDiagram
actor BlockProducer
participant BerachainBB as BerachainBlockBuilder
participant InnerBB as Inner BlockBuilder
participant Executor
participant StateProvider
participant PolTxs as PoL Transactions
BlockProducer->>BerachainBB: finish(state_provider)
BerachainBB->>InnerBB: finish(state_provider)
InnerBB->>Executor: execute transactions
InnerBB->>StateProvider: finalize state
InnerBB-->>BerachainBB: BlockBuilderOutcome
Note over BerachainBB: Inspect tx & sender counts
alt Post-Prague1 && txs > senders
rect rgb(200, 220, 255)
BerachainBB->>PolTxs: Extract senders from PoL txs at block start
PolTxs-->>BerachainBB: Recovered senders
BerachainBB->>BerachainBB: Prepend recovered senders
end
else Pre-Prague1 && txs > senders
BerachainBB->>BerachainBB: Error: invariant violated
else Counts match or mismatch allowed
BerachainBB->>BerachainBB: Use senders as-is
end
BerachainBB->>BerachainBB: Reconstruct block with corrected senders
BerachainBB-->>BlockProducer: BlockBuilderOutcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 review |
✅ Actions performedReview triggered.
|
![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
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlsrc/node/evm/builder.rssrc/node/evm/config.rssrc/node/evm/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.rs: No comments in code unless explicitly requested
Use custom error types over generic errors
Extract magic numbers into documented constants
Write succinct documentation focusing on 'why' rather than 'what'
Files:
src/node/evm/mod.rssrc/node/evm/builder.rssrc/node/evm/config.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: rezbera
Repo: berachain/bera-reth PR: 152
File: src/pool/transaction.rs:3-4
Timestamp: 2025-09-10T05:49:07.298Z
Learning: In reth v1.7.0 upgrade, BerachainPooledTransactionVariant type alias should use PooledTransactionVariant directly instead of EthereumTxEnvelope<TxEip4844WithSidecar<BlobTransactionSidecarVariant>>.
🧬 Code graph analysis (2)
src/node/evm/builder.rs (3)
src/pool/transaction.rs (1)
transaction(151-153)src/engine/mod.rs (1)
inner(210-212)src/node/evm/config.rs (1)
chain_spec(75-77)
src/node/evm/config.rs (2)
src/node/evm/builder.rs (1)
new(55-57)src/node/evm/assembler.rs (1)
new(34-36)
🔇 Additional comments (8)
src/node/evm/mod.rs (1)
5-5: LGTM!The module declaration is properly placed and uses appropriate private visibility, consistent with other internal modules like
assemblerandblock_context.Cargo.toml (1)
65-65: LGTM!The dependency is correctly added with the same git revision as other reth crates, maintaining version consistency. It's required for the
StateProvidertrait used in the newBerachainBlockBuilder.src/node/evm/builder.rs (4)
18-49: Well-documented struct with clear problem statement and solution.The struct-level documentation thoroughly explains the sender/transaction mismatch problem, the solution approach, and the invariants enforced. This provides excellent context for future maintainers.
107-127: Clarify or enforce the single PoL injection invariant.The comment on line 108 questions whether to assert
num_injected == 1. If only a single PoL transaction is ever injected post-Prague1, consider enforcing this invariant with an explicit check:🔎 Suggested enforcement
// Post-Prague1 with mismatch = expected, fix it (true, true) => { - // There should only be 1 injected tx -> assert this? let num_injected = num_txs - num_senders; + if num_injected != 1 { + return Err(BlockExecutionError::msg(format!( + "expected exactly 1 PoL injection post-Prague1, found {} at timestamp {}", + num_injected, timestamp + ))); + } let mut fixed_senders = Vec::with_capacity(num_txs);Alternatively, if multiple PoL injections are valid, remove the comment to avoid confusion.
129-133: Sender reconstruction logic is correct.The approach of extending the fixed senders with existing senders and reconstructing the block is sound. The
clone_block()call is necessary sinceRecoveredBlock::new_unhashedtakes ownership.
150-160: LGTM!The executor accessor methods correctly delegate to the inner builder.
src/node/evm/config.rs (2)
7-7: LGTM!The import of
BerachainBlockBuilderis correctly added alongsideBerachainReceiptBuilder.
20-20: LGTM!The new imports support the
create_block_buildermethod implementation.Also applies to: 29-30
| fn create_block_builder<'a, DB, I>( | ||
| &'a self, | ||
| evm: EvmFor<Self, &'a mut State<DB>, I>, | ||
| parent: &'a SealedHeader<HeaderTy<Self::Primitives>>, | ||
| ctx: <Self::BlockExecutorFactory as BlockExecutorFactory>::ExecutionCtx<'a>, | ||
| ) -> impl reth_evm::execute::BlockBuilder< | ||
| Primitives = Self::Primitives, | ||
| Executor: reth_evm::block::BlockExecutorFor<'a, Self::BlockExecutorFactory, DB, I>, | ||
| > | ||
| where | ||
| DB: Database, | ||
| I: InspectorFor<Self, &'a mut State<DB>> + 'a, | ||
| { | ||
| let inner: BasicBlockBuilder<'a, Self, _, BerachainBlockAssembler, BerachainPrimitives> = | ||
| BasicBlockBuilder { | ||
| executor: BlockExecutorFactory::create_executor(self, evm, ctx.clone()), | ||
| ctx, | ||
| assembler: self.block_assembler().clone(), | ||
| parent, | ||
| transactions: Vec::new(), | ||
| }; | ||
| BerachainBlockBuilder::new(inner, self.spec.clone()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine imports at the top of the file
head -50 src/node/evm/config.rs | grep -E "use|import|BasicBlockBuilder"Repository: berachain/bera-reth
Length of output: 617
🏁 Script executed:
# Check where BasicBlockBuilder is imported from
rg "BasicBlockBuilder" src/node/evm/config.rs -B 5 -A 5Repository: berachain/bera-reth
Length of output: 1041
🏁 Script executed:
# Search for BasicBlockBuilder definition in the repository
fd "*.rs" | xargs rg "struct BasicBlockBuilder|impl BasicBlockBuilder" -lRepository: berachain/bera-reth
Length of output: 348
🏁 Script executed:
# Check if there's a constructor or factory method for BasicBlockBuilder
rg "impl.*BasicBlockBuilder" --type rsRepository: berachain/bera-reth
Length of output: 89
🏁 Script executed:
rg "BasicBlockBuilder" src/ -A 3 -B 3Repository: berachain/bera-reth
Length of output: 1668
🏁 Script executed:
# Also search for any factory methods or builder patterns in block-related code
rg "BlockBuilder|block_builder|create_block" src/node/evm/ -lRepository: berachain/bera-reth
Length of output: 109
🏁 Script executed:
cat src/node/evm/builder.rs | head -100Repository: berachain/bera-reth
Length of output: 3960
🏁 Script executed:
# Search for BerachainBlockBuilder definition
rg "struct BerachainBlockBuilder|impl BerachainBlockBuilder" src/ -A 10Repository: berachain/bera-reth
Length of output: 577
🏁 Script executed:
sed -n '240,270p' src/node/evm/config.rsRepository: berachain/bera-reth
Length of output: 1292
🏁 Script executed:
# Check broader context for any comments near the method
sed -n '235,275p' src/node/evm/config.rsRepository: berachain/bera-reth
Length of output: 1787
Add documentation explaining the direct BasicBlockBuilder construction.
The method lacks documentation explaining why BasicBlockBuilder is directly constructed with struct initialization rather than using an alternative approach. Since the coding guideline requires succinct documentation focusing on 'why', add a doc comment clarifying the design rationale—for example, whether this explicit construction is necessary to configure specific fields or if it's the only available approach from the reth dependency.
🤖 Prompt for AI Agents
In src/node/evm/config.rs around lines 247 to 269, add a short doc comment above
create_block_builder explaining why BasicBlockBuilder is constructed directly
via struct literal instead of an alternative (e.g., no public constructor in
reth, need to set fields explicitly, or to inject specific
executor/assembler/ctx/parent/transactions), describing the design rationale and
any implications for future changes; keep it succinct (one or two sentences) and
reference the relevant dependency behavior (reth API) so readers know this is
intentional.
BerachainBlockBuilder wraps the inner block builder to fix sender count mismatch for PoL transactions. The reth block builder doesn't recover senders for transactions that bypass the pool (system txs), so we extract senders from PoL transactions directly.
Invariants are strictly enforced based on Prague1 activation:
Violations in either direction indicate a bug in block assembly.
Fixes: #129
TODO: Rebase onto #200 once merged