-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add batch submission policy that uses Blob transactions #8769
Add batch submission policy that uses Blob transactions #8769
Conversation
WalkthroughWalkthroughThe Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
cba8e4f
to
2f359fd
Compare
c7f1748
to
7386ec8
Compare
e0792f1
to
ba8c192
Compare
ba8c192
to
fb93afa
Compare
16649ae
to
62a676b
Compare
b5a03d8
to
53c6ddb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we adding an e2e test in a different PR that tests that the batcher creates correct blob batch txs?
yes, we have an e2e test already that is doing exactly this, but it is from before all the l1 cost function related complexity was added, so it's going to have to be reworked and be resubmitted after this gets merged. |
53c6ddb
to
f96516d
Compare
3beb719
to
8ff4974
Compare
…hat uses blob transactions
8ff4974
to
3f3d118
Compare
// likely result in the chain spending more in gas fees than it is tuned for, so best | ||
// to just fail. We do not expect this error to trigger unless there is a serious bug | ||
// or configuration issue. | ||
return fmt.Errorf("could not create blob tx candidate: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roberto-bayardo was just reading the code and realized that if this ever happens, the frames are forever gone, as they aren't put back in the channel.
are we missing a l.state.TxFailed(txdata.ID())
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is one of those errors that should probably just panic or log.Fatal since there's no good way to recover. It's only triggered if you get a frame that is larger than the blob, so if you put the data back, it will just get re-triggered.
Description
Once Dencun / 4844 is activated on the L1, we wish to allow batches to be posted using Blob transactions. This PR adds a batcher configuration option to trigger submitting batches using blobs instead of calldata.
Tests
This is primarily a configuration change and will be ultimately be tested via e2e tests in a subsequent PR.