add sequencer-relayer protos #12
Merged
Conversation
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
* Fix using extern dependencies * Fix lint * Test cleanup * update ci
SuperFluffy
reviewed
May 19, 2023
crates/astria-proto/proto/astria/sequencer_relayer/v1/data.proto
Outdated
Show resolved
Hide resolved
crates/astria-proto/proto/astria/sequencer_relayer/v1/block.proto
Outdated
Show resolved
Hide resolved
crates/astria-proto/proto/astria/sequencer_relayer/v1/data.proto
Outdated
Show resolved
Hide resolved
crates/astria-proto/proto/astria/sequencer_relayer/v1/data.proto
Outdated
Show resolved
Hide resolved
crates/astria-proto/proto/astria/sequencer_relayer/v1/data.proto
Outdated
Show resolved
Hide resolved
| bytes block_hash = 1; | ||
| tendermint.types.Header header = 2; | ||
| repeated IndexedTransaction sequencer_txs = 3; | ||
| repeated NamespacedIndexedTransactions rollup_txs = 4; |
There was a problem hiding this comment.
So these deeply nested types are a bit tricky form a performance point of view:
- protobuf does not contain length information in the wire-format for
repeatedmessages (which includesmap, because these are justrepeated(key,value)tuples); - protobuf does not guarantee that
repeatedmessages are contiguous on the wire (the first element might be in the beginning, the last element at the end of the bytestream);
This means there is no way to pre-allocate sufficiently sized Vecs. And so you will end up with a ton of small allocations in the child-vecs which cannot get amortized.
As an alternative route I would suggest making use of a technique frequently encountered in game dev by inlining all the fields, i.e.:
message NamespacedIndexedTransaction {
bytes namespace = 1;
// Maybe with a clearer name here what the index exactly refers to.
uin64 index = 2;
bytes transaction = 3;
}
crates/astria-proto/proto/astria/sequencer_relayer/v1/block.proto
Outdated
Show resolved
Hide resolved
Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>
|
@itamarreif the fork off of which this PR is based has experienced quite a bit of churn. Can you fork off recent |
|
LGTM |
SuperFluffy
approved these changes
May 22, 2023
noot
approved these changes
May 22, 2023
noot
reviewed
May 22, 2023
This was referenced Sep 12, 2023
SuperFluffy
added a commit
that referenced
this pull request
Sep 18, 2023
## Summary Replace all implicit eyre errors by context specific typed errors ## Background Same reasoning as for #375 as part of the work for #332: `sequencer-types` has to be made a dependency of `sequencer-client`, which only uses typed errors. Therefore `sequencer-types` must have typed errors (because otherwise `sequencer-client` would be "infected" by eyre, which would require changing all of sequencer-client to also use eyre. ## Changes - Introduced one error variant per distinct eyre error. ## Testing N/A. Nothing changed on the code level. Testing that the errors are actually triggered could be done in a follow-up PR, but big parts of the code will be replaced by the work in #355 and follow-up PRs to it, moving the types exchanged over RPCs to the astria-proto crate. ## Breaking Changelist - No observable breaking changes in the astria services. Breaking changes in the library crates because their public API changed (errors), but we are not considering them right now ## Related Issues #12 #86 #366 #332
joroshiba
added a commit
that referenced
this pull request
Nov 30, 2023
## Summary This patch defines protobuf versions of various types that are currently only defined in Rust and exchange as json-serialized text. ## Background `astria-proto` is intended to be the single source of truth for the wire format of data that is exchanged between services. At the moment `SequencerBlockData`, `SequencerNamespaceData`, `RollupNamespaceData` and `InclusionProof` are only defined in Rust. Some older versions of these exist as protobuf but were never used in the public interfaces. ## Changes - Replace `SequencerBlockData` by a protobuf `SequencerBlock` (rewriting this protobuf type is technically a breaking change, but it was never used, so in practice it is not) - Replace `SequencerNamespaceData` and `RollupNamespaceData` by protobuf `CelestiaSequencerBlob` and `CelestiaRollupBlob`. - Introduce other protobuf types like `RollupTransactions`, `Proof` to decode/encode all required objects - Write idiomatic rust types for all generated protobuf representations to uphold invariants ## Testing - all tests have been updated and still pass - testing this is in a devnet to check that the data still flows is open ## Related Issues #12 originally defined sequencer blocks and transactions as protobuf, but these were never actually implemented. #86 is the reason this effort was started. --------- Co-authored-by: Jordan Oroshiba <jordan@astria.org>
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.
addresses issue #20