vm: Fix BlockBuilder bug#4277
Draft
ScottyPoi wants to merge 8 commits into
Draft
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 Bundle Size Analysis
Values are minified+gzipped bundles of each package entry. Workspace deps are bundled; external deps are excluded. Generated by bundle-size workflow |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a BlockBuilder atomicity bug and closes a fork-validation gap for EIP-4844/EIP-7594 blob transactions by validating fork-sensitive constraints earlier during network-wrapper deserialization and by making BlockBuilder.addTransaction() roll back cleanly on errors.
Changes:
- Add fork-sensitive blob/network-wrapper validation during network-wrapper deserialization and reuse it in
Blob4844Txvalidation. - Refactor
BlockBuilder.addTransaction()to be builder-atomic via per-transaction rollback + precomputing minimal blob tx. - Add regression tests for builder rollback behavior and pre-7594 deserialization rejection.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/vm/src/buildBlock.ts | Makes addTransaction() atomic by checkpointing and rolling back builder accounting/state on failures. |
| packages/vm/test/api/buildBlock.spec.ts | Adds regression test ensuring builder state is reverted when bookkeeping throws. |
| packages/tx/src/4844/tx.ts | Factors fork/network-wrapper checks into a shared validator and reuses it in constructor/validation paths. |
| packages/tx/src/4844/constructors.ts | Runs fork/network-wrapper validation during network-wrapper deserialization before mutating the decoded tx. |
| packages/tx/test/eip7594.spec.ts | Adds regression coverage for rejecting EIP-7594 wrappers under pre-7594 Common and adjusts timeouts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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
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.
Implemented the fix for #4271.
I was able to reproduce the user's sceanario and confirm that the issue is valid.
The current BlockBuilder flow does
runTx(...), then mutates builder accounting, then reconstructs the minimal blob tx.That means a throw from minimal tx reconstruction can happen after execution has already committed into the builder journal and after
blobGasUsedhas been mutated, but before the tx/result arrays andgasUsedare updated.The decode path in
4844/constructors.tsalso constructs the tx first and only later assignsdecodedTx.networkWrapperVersion = networkWrapperVersionInt, so the constructor checks intx.tsdo not run against the wire wrapper version during initial decode.Fix:
In packages/tx/src/4844/tx.ts and packages/tx/src/4844/constructors.ts, I factored the fork-sensitive blob checks into a shared validator and now run that validation during network-wrapper deserialization before mutating the decoded tx. That closes the split-validation gap where a wrapper_version = 1 payload could be decoded under a pre-7594 Common and only fail later.
In
packages/vm/src/buildBlock.ts,BlockBuilder.addTransaction()is now atomic at the builder level. It creates a nested checkpoint per transaction attempt, precomputes the minimal blob tx before execution, and restores builder counters/arrays on any error. That prevents partial state,blobGasUsed, and tx bookkeeping from diverging if something throws mid-flow.