-
Notifications
You must be signed in to change notification settings - Fork 77
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(conductor, relayer)!: batch multiple Sequencer blocks to save on Celestia fees #1045
Conversation
7bd6e3a
to
1f753bc
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.
LGTM - mostly just nitpicks and a couple of questions.
crates/astria-sequencer-relayer/src/relayer/write/conversion.rs
Outdated
Show resolved
Hide resolved
crates/astria-sequencer-relayer/src/relayer/write/conversion.rs
Outdated
Show resolved
Hide resolved
crates/astria-sequencer-relayer/src/relayer/write/conversion.rs
Outdated
Show resolved
Hide resolved
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.
I haven't ever loved the naming of these, and I think the new names make it if anything worse.
The core issue is that these are Celestia
prefixed but the data has nothing to do with celestia. Especially the new CelestiaHeader
is not great because Celestia has headers and have nothing to do with these.
I propose we get rid of the Celestia
prefix entirely and have RollupData
and SequencerBlockMetadata
, and their list forms. The Sequencer Block one is hard because it's the SequencerBlockHeader
plus some information. I think metadata encompasses this alright.
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.
Unfortunately RollupData
was already taken in the same protobuf package. I chosen the following nomenclature instead in 401233a:
SubmittedMetadata
: since the full type name is actuallyastria.sequencerblock.v1alpha1.SubmittedMetadata
it's clear that this pertains to asequencerblock
.SubmittedRollupData
crates/astria-sequencer-relayer/src/relayer/write/conversion.rs
Outdated
Show resolved
Hide resolved
50ffab0
to
401233a
Compare
@joroshiba I have added a histogram metric |
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.
API approval, new names look good.
Summary
Batch mutiple Sequencer blocks into single Celestia blobs to reduce fee payments.
Background
Until now, each Sequencer block was turned into multiple blobs (one for overall sequencer block metadata, and one blob per rollup that had transactiosn in the sequencer block). This wasn't as efficient as it could be because the new compression scheme introduced in #1006 can only come to bear with more bytes to compress.
Relayer will collect sequencer blocks up to a total (compressed) size of 1MB (1/2 of the current max of 2MB that Celestia blocks can be).
Changes
astria.sequencerblock.v1alpha1.CelestiaHeaderList
andastria.sequencerblock.v1alpha1.CelestiaRollupDataList
astria.sequencerblock.v1alpha1.CelestiaRollupBlob
toastria.sequencerblock.v1alpha1.CelestiaRollupData
astria.sequencerblock.v1alpha1.CelestiaSequencerBlob
toastria.sequencerblock.v1alpha1.CelestiaHeader
*List
protobuf messages before posting them to Celestia (instead of splitting up each Sequencer block into mutiple blobs and posting them one by one).Testing
Add unit tests around the next submission aggregation logic. Update conductor blackbox tests.
Metrics
CELESTIA_PAYLOAD_CREATION_LATENCY
: histogram with microsecond units to track the time it takes to create a payload of Celestia blobs (encoding + compressing all protobufs)Breaking Changelist
Related Issues
Closes #1042
Closes #1049