forked from ethereum-optimism/optimism
-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat(batcher): multi-frame altda channels #310
Merged
gastonponti
merged 5 commits into
celo-rebase-12
from
eigenda/feat--multiframe-altda-channel
Feb 14, 2025
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
ed9ec99
feat(batcher): multi-frame altda channels
samlaf dbd9ffb
docs(batcher): add documentation for DaType and txData.daType
samlaf 0f4d7ef
docs: fix NextTxData comment
samlaf 99d00f2
Merge branch 'celo-rebase-12' into eigenda/feat--multiframe-altda-cha…
gastonponti 8f9c63d
Merge branch 'celo-rebase-12' into eigenda/feat--multiframe-altda-cha…
gastonponti File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
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.
Hey @gastonponti, I'm wondering how do you set
target-num-frames
akaMaxFramesPerTx
to make sure that your frames don't exceed the max frame size of 128KB?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.
There's a flag that you can use to set that:
Here are two different things.
The frame size from one side, and the "tx for the L1 data size" on the other.
The frame size is also configurable, and we should reduce it. We strictly don't need to have it as 128kb because we are not switching to blobs on eth, but to txData on eth. But if we set it to 128kb, it will be easier to switch to a eth blob scenario if we want, because we won't need to think about that size. De downside of setting it to 128kb and not 200kb is that you will need more txs in a full blocks scenario, but this is a fallback scenario that we shouldn't see often if eigenda works as it should.
The maxFramesPerTx is used for all the other types that are NOT the txDataType (or DaTypeCalldata from this PR)
with that, you are guaranteeing, that in the case of switching to ethDa, you will have only one frame in the tx, and you won't exceed the max L1 tx data size
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.
So we need to set
TARGET_NUM_FRAMES
or do we want to change the default in that flag?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.
We only need to set that
TARGET_NUM_FRAMES
flagThere 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.
Can we document those values somewhere?
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.
The frames are capped by this flag:
actually is
MaxL1TxSizeByte-1
It's not a moving thing that depends on the channel. The channel depends on the Frame size x number of Frames
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.
The ignored for blobs, is for the ethDa for blobs, which we are not using. So our frame cap is actually set in the configurations
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.
Actually I think I've understood.
Channel contains whatever unsafe data from L2
Channels produce a stream of txData objects (as many as required)
TxData objects contain TARGET_NUM_FRAMES frames
Frames are up to MaxFrameSize big
But now I'm wondering, how is it enforced that the txdata is within the required size limits?
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.
Yeah, that txData name is not the best to be honest.
But, with that txData, if it goes to the altDa, it builds a blob with N frames. But if it goes to ethda as tx-data (not blobs), it sends a tx per frame. So the frame size is the one limiting that
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.
@piersy
optimism/op-batcher/batcher/channel_builder.go
Lines 363 to 383 in f90b97b
Within the
OutputFrame()
the passed in size is used as the size of a buffer, where then data from the compressionstage is written to. The passed in buffer is then used as frameData in the batcher.