feat: redefine sequencer blocks, celestia blobs as protobuf #395

SuperFluffy · 2023-09-19T13:10:30Z

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.

SuperFluffy · 2023-09-19T13:11:23Z

Opened a new PR because something broke #355. No changes with respect to the most recent review.

crates/astria-proto/proto/astria/sequencer/v1alpha2/celestia.proto

noot

looks good! are you planning to do #20 (and move the types to astria-proto) in a follow up?

also is #31 still relevant or can it be closed?

needs more discussionin light of #401

SuperFluffy · 2023-09-20T14:34:23Z

@noot yeah, #20 should be the immediate follow-up to this PR

#31 will probably be outdated with this change.

I have dismissed your review because I think this PR needs more discussion in light of #401 (to ensure this does not accidentally sneak in).

noot · 2023-09-20T17:39:37Z

we can put #401 on hold for now - don't think we need to change the verification stuff yet, probably going to close #401 and open another broader issue. so this should be good to go in!

crates/astria-proto/proto/astria/sequencer/v1alpha1/block.proto

SuperFluffy · 2023-09-22T16:28:11Z

The proto lint will fail because this is a breaking proto buf change (which is ok, because the proto definitions were never used)

joroshiba · 2023-11-30T00:10:44Z

crates/astria-conductor/src/conductor.rs

+                .await
+                .wrap_err("failed constructing optimism hook")?;
+
+            let executor = Executor::builder()


noot · 2023-11-30T00:34:33Z

crates/astria-proto/proto/astria/sequencer/v1alpha1/block.proto

+    // The proof that the rollup transactions are included in the CometBFT block this
+    // sequencer block is derived form. This proof together with
+    // `Sha256(MTH(rollup_transactions))` must match `header.data_hash`.
+    // `MTH(rollup_transactions)` is the Merkle Tree Hash derived from the
+    // rollup transactions.
+    astria.sequencer.v1alpha1.Proof rollup_transactions_proof = 3;
+    // The proof that the rollup IDs listed in `rollup_transactions` are included
+    // in the CometBFT block this sequencer block is derived form.
+    //
+    // This proof is used to verify that the relayer that posts to celestia
+    // includes all rollup IDs and does not censor any.
+    //
+    // This proof together with `Sha256(MTH(rollup_ids))` must match `header.data_hash`.
+    // `MTH(rollup_ids)` is the Merkle Tree Hash derived from the rollup IDs listed in
+    // the rollup transactions.
+    astria.sequencer.v1alpha1.Proof rollup_ids_proof = 4;


the proofs are in here but not the commitments? the proof can't be used without the commitment (leaf) value?

The leaves can be (and are) reconstructed ad hoc from the transactions. This used to be checked in the replaced sequencer block data (the leaf was reconstructed and checked against the root in the object).

I think the comment mentions this.

ok, i see you are planning to just reconstruct the commitments everywhere

noot · 2023-11-30T00:36:39Z

crates/astria-proto/proto/astria/sequencer/v1alpha1/celestia.proto

+    // Tree Hash deriveed from the rollup transactions.
+    // Always 32 bytes.
+    bytes rollup_transactions_root = 3;
+    // The proof that the rollup transactions are included in sequencer block.


Suggested change

// The proof that the rollup transactions are included in sequencer block.

// The proof that the rollup transactions are included in sequencer block header.

noot · 2023-11-30T00:36:49Z

crates/astria-proto/proto/astria/sequencer/v1alpha1/celestia.proto

+    astria.sequencer.v1alpha1.Proof rollup_transactions_proof = 4;
+    // The proof that the rollup IDs are included in sequencer block.
+    // Corresponds to `astria.sequencer.v1alpha.SequencerBlock.rollup_ids_proof`.
+    astria.sequencer.v1alpha1.Proof rollup_ids_proof = 5;


no rollup_ids_root? because you plan to reconstruct?

Can be reconstructed ad hoc from the list of IDs.

In fact one needs to do this always anyways to ensure the stored is correct. But then we can just skip it.

See the comment and the constructor of the idiomatic native type.

noot · 2023-11-30T00:42:19Z

crates/astria-proto/src/native/sequencer/v1alpha1.rs

+    /// The original `CometBFT` header that was the input to this sequencer block.
+    header: tendermint::block::header::Header,
+    /// The collection of rollup transactions that were included in this block.
+    rollup_transactions: IndexMap<RollupId, Vec<Vec<u8>>>,


how does IndexMap's ordering work compared to BTreeMap? assuming it's different, that's a breaking change

This new grouping mechanism is used inside sequencer as well. You can see that the snapshot test is unaffected.

When the indexmap is constructed it is ordered by its keys after the fact. But this is the same order as that used for the btreemap.

What is the purpose of IndexMap over BTreeMap?

BtreeMap is balancing while it's being constructed. That gives nlogn overhead. Insertion the indexmap is linear and the final ordering has the same nlogn

Access on the other hand is O(1) where it is log n for the btreemap.

The indexmap is cache efficient, the btreemap is not.

So for our purpose the index map is strictly better.

noot · 2023-11-30T00:44:29Z

crates/astria-proto/src/native/sequencer/v1alpha1.rs

+    // The proof that the rollup transactions are included in the `CometBFT` block this
+    // sequencer block is derived form. This proof together with
+    // `Sha256(MTH(rollup_transactions))` must match `header.data_hash`.
+    // `MTH(rollup_transactions)` is the Merkle Tree Hash derived from the
+    // rollup transactions.
+    rollup_transactions_proof: merkle::Proof,
+    // The proof that the rollup IDs listed in `rollup_transactions` are included
+    // in the `CometBFT` block this sequencer block is derived form. This proof together
+    // with `Sha256(MTH(rollup_ids))` must match `header.data_hash`.
+    // `MTH(rollup_ids)` is the Merkle Tree Hash derived from the rollup IDs listed in
+    // the rollup transactions.
+    rollup_ids_proof: merkle::Proof,


same question as on the proto - how come the proofs are here but the commitments aren't? is it because you plan to always reconstruct the rollup_transactions_root from rollup_transactions?

Yeah, but this was always done anyway to ensure these data types uphold invariants and that the stored root and the data it is constructed from is correct.

But it need not be written to the wire format.

If it ever becomes necessary to give repeated access to this root we can add it to the idiomatic types as is done for the cometbft header block hash.

noot · 2023-11-30T00:53:22Z

crates/astria-proto/src/native/sequencer/v1alpha1.rs

+        })
+    }
+
+    /// Converts from the raw decoded protobuf representatin of this type.


Suggested change

/// Converts from the raw decoded protobuf representatin of this type.

/// Converts from the raw decoded protobuf representation of this type.

noot · 2023-11-30T00:55:39Z

crates/astria-proto/src/native/sequencer/v1alpha1.rs

+        let rollup_transactions_proof = 'proof: {
+            let Some(rollup_transactions_proof) = rollup_transactions_proof else {
+                break 'proof Err(SequencerBlockError::FieldNotSet(
+                    "rollup_transactions_proof",
+                ));


why the 'proof syntax over just returning the error?

There are two steps in this block: one ensures the protobuf value was set, the next one actually does the conversion.

These two steps belong together logically and I didn't want to declare variables at the function level namespace.

I also think this is neat.

break 'proof Err(SequencerBlockError::FieldNotSet( "rollup_transactions_proof", ));

this is logically the same as:

return Err(SequencerBlockError::FieldNotSet( "rollup_transactions_proof", ));

in this scenario right? just checking for my own sanity lol

noot · 2023-11-30T01:01:01Z

crates/astria-proto/src/native/sequencer/v1alpha1.rs

+}
+
+// TODO: This can all be done in-place once https://github.com/rust-lang/rust/issues/80552 is stabilized.
+pub fn group_sequence_actions_in_signed_transaction_transactions_by_rollup_id(


Suggested change

pub fn group_sequence_actions_in_signed_transaction_transactions_by_rollup_id(

pub fn group_sequence_actions_in_signed_transactions_by_rollup_id(

joroshiba · 2023-11-30T00:12:10Z

crates/astria-conductor/src/data_availability/block_verifier.rs

+        UncheckedCelestiaSequencerBlob,
    };
-    use sequencer_types::ChainId;
+    // use sequencer_types::ChainId;


nit: remove

joroshiba · 2023-11-30T00:24:49Z

crates/astria-proto/proto/astria/sequencer/v1alpha1/block.proto

+    // The original CometBFT header that was the input to this sequencer block.
+    tendermint.types.Header header = 1;
+    // The collection of rollup transactions that were included in this block.
+    repeated RollupTransactions rollup_transactions = 2;


Is there an order that matters here? The order of rollups shouldn't matter?

joroshiba · 2023-11-30T00:45:29Z

crates/astria-sequencer-relayer/src/relayer.rs

    let height = client
        .submit_sequencer_blocks(
-            sequencer_block_data,
+            sequencer_blocks,


I think this is fine right now, but I wonder if we should have an earlier step to process the blobs. So it's simply "publish the blobs" since we don't need to block creating the blobs for each sequencer block on celestia post finishing.

joroshiba · 2023-11-30T00:48:33Z

crates/astria-conductor/src/conductor.rs

+            // XXX: This log is very misleading. The field called `sequencer_chain_id` is actually
+            // the rollup ID.
+            //
+            // info!(
+            //     celestia_namespace = %Base64Display::new(sequencer_namespace.as_bytes(),
+            // &STANDARD),     sequencer_chain_id = %cfg.chain_id,
+            //     "celestia namespace derived from sequencer chain id",
+            // );


nit: remove/improve

joroshiba · 2023-11-30T00:53:00Z

crates/astria-proto/src/native/sequencer/v1alpha1.rs

 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[cfg_attr(feature = "serde", serde(transparent))]
-pub struct ChainId {
+pub struct RollupId {


We should follow up with changes to any configs + helm charts

joroshiba · 2023-11-30T00:58:00Z

crates/astria-proto/src/native/sequencer/v1alpha1.rs

+    /// The original `CometBFT` header that was the input to this sequencer block.
+    header: tendermint::block::header::Header,
+    /// The collection of rollup transactions that were included in this block.
+    rollup_transactions: IndexMap<RollupId, Vec<Vec<u8>>>,


What is the purpose of IndexMap over BTreeMap?

joroshiba · 2023-11-30T01:01:39Z

crates/astria-proto/src/native/sequencer/v1alpha1.rs

+///
+/// At the moment there are no invariants upheld by [`CelestiaRollupBlob`] so
+/// they can be converted directly into one another. This can change in the future.
+pub struct UncheckedCelestiaRollupBlob {


Why the "unchecked" verbiage here? There is nothing checked when converting? It's just pub access vs not?

Yeah, thats the case here.

Honestly no reason other than being symmetric with the sequencer blob.

I'm not a big fan of the unchecked versions. But yes, it's to give pub access to the fields to avoid allocations.

noot · 2023-11-30T01:04:16Z

crates/astria-sequencer-types/src/tendermint.rs

@@ -1,24 +1 @@
-// Calculates the `last_commit_hash` given a Tendermint [`Commit`].


delete this file?

noot · 2023-11-30T01:07:00Z

crates/astria-conductor/src/conductor.rs

        let signals = spawn_signal_handler();

+        let rollup_id =
+            proto::native::sequencer::v1alpha1::RollupId::from_unhashed_bytes(&cfg.chain_id);


rename the config value also to rollup_id

I wasn't sure about this. It's m technically not the ID but just the name. The ID are the 32 bytes and I don't want to cause confusion between these two.

Is rollup_name good? Also CC @joroshiba

noot · 2023-11-30T01:07:36Z

crates/astria-conductor/src/conductor.rs

-                sequencer_chain_id = %cfg.chain_id,
-                "celestia namespace derived from sequencer chain id",
-            );
+            // XXX: This log is very misleading. The field called `sequencer_chain_id` is actually


yeah, do you wanna delete/update it?

noot · 2023-11-30T01:11:58Z

crates/astria-celestia-client/src/client.rs

+///
+/// Drops a blob under the following conditions:
+/// + the blob's namespace does not match the provided [`Namespace`]
+/// + cannot be decode/convert to [`CelestiaRollupBlob`]


Suggested change

/// + cannot be decode/convert to [`CelestiaRollupBlob`]

/// + cannot decode/convert to [`CelestiaRollupBlob`]

noot · 2023-11-30T01:12:12Z

crates/astria-celestia-client/src/client.rs

+/// + the blob's namespace does not match the provided [`Namespace`]
+/// + cannot be decode/convert to [`CelestiaRollupBlob`]
+/// + block hash does not match that of [`CcelestiaSequencerBlob`]
+/// + the proof, ID, and transactions recorded in the blob cannot be verified against the seuencer


Suggested change

/// + the proof, ID, and transactions recorded in the blob cannot be verified against the seuencer

/// + the proof, ID, and transactions recorded in the blob cannot be verified against the sequencer

SuperFluffy requested review from joroshiba and noot as code owners September 19, 2023 13:10

github-actions bot added the proto pertaining to the Astria Protobuf spec label Sep 19, 2023

SuperFluffy temporarily deployed to BUF September 19, 2023 13:10 — with GitHub Actions Inactive

SuperFluffy changed the title ~~proto: define sequencer v1alpha2~~ proto: extract sequencer and sequencer-relayer types as protobuf Sep 19, 2023

SuperFluffy force-pushed the superfluffy/sequencer-types-as-protos branch 2 times, most recently from 5416bda to 4d09a0b Compare September 19, 2023 13:38

noot reviewed Sep 19, 2023

View reviewed changes

crates/astria-proto/proto/astria/sequencer/v1alpha2/celestia.proto Outdated Show resolved Hide resolved

SuperFluffy temporarily deployed to BUF September 19, 2023 19:09 — with GitHub Actions Inactive

noot previously approved these changes Sep 19, 2023

View reviewed changes

SuperFluffy mentioned this pull request Sep 20, 2023

conductor: remove unnecessary commit verification #401

Closed

SuperFluffy added the question Further information is requested label Sep 20, 2023

noot approved these changes Sep 20, 2023

View reviewed changes

SuperFluffy force-pushed the superfluffy/sequencer-types-as-protos branch from c24fccb to c857752 Compare September 22, 2023 10:04

SuperFluffy temporarily deployed to BUF September 22, 2023 10:04 — with GitHub Actions Inactive

joroshiba reviewed Sep 22, 2023

View reviewed changes

crates/astria-proto/proto/astria/sequencer/v1alpha1/block.proto Show resolved Hide resolved

SuperFluffy temporarily deployed to BUF September 22, 2023 16:25 — with GitHub Actions Inactive

SuperFluffy requested a review from joroshiba September 22, 2023 16:28

SuperFluffy temporarily deployed to BUF September 25, 2023 11:59 — with GitHub Actions Inactive

SuperFluffy force-pushed the superfluffy/sequencer-types-as-protos branch from 87c569e to fee81b9 Compare September 25, 2023 13:29

SuperFluffy temporarily deployed to BUF September 25, 2023 13:29 — with GitHub Actions Inactive

SuperFluffy mentioned this pull request Sep 26, 2023

Use plain strings as chain IDs #432

Closed

jbowen93 mentioned this pull request Nov 23, 2023

Add celestia command(s) to astria-cli #605

Closed

SuperFluffy marked this pull request as draft November 23, 2023 14:39

SuperFluffy force-pushed the superfluffy/sequencer-types-as-protos branch from fee81b9 to 316f304 Compare November 23, 2023 14:40

SuperFluffy changed the title ~~feat: extract sequencer and sequencer-relayer types as protobuf~~ feat: redefine sequencer blocks, celestia blobs as protobuf Nov 29, 2023

sequencer-relayer: regression, don't submit the same blocks to celestia

bcf7e74

SuperFluffy temporarily deployed to BUF November 29, 2023 21:28 — with GitHub Actions Inactive

joroshiba reviewed Nov 30, 2023

View reviewed changes

noot reviewed Nov 30, 2023

View reviewed changes

joroshiba reviewed Nov 30, 2023

View reviewed changes

noot reviewed Nov 30, 2023

View reviewed changes

noot approved these changes Nov 30, 2023

View reviewed changes

joroshiba approved these changes Nov 30, 2023

View reviewed changes

Merge branch 'main' into superfluffy/sequencer-types-as-protos

770ff7e

joroshiba temporarily deployed to BUF November 30, 2023 01:20 — with GitHub Actions Inactive

joroshiba merged commit f8a490c into main Nov 30, 2023

joroshiba deleted the superfluffy/sequencer-types-as-protos branch November 30, 2023 01:27

SuperFluffy mentioned this pull request Nov 30, 2023

define common astria types for sequencer protos #444

Closed

noot mentioned this pull request Nov 30, 2023

flatten SequencerBlock protos #31

Closed

SuperFluffy mentioned this pull request Nov 30, 2023

proto: define sequencer v1alpha1 #443

Closed

noot mentioned this pull request Nov 30, 2023

sequencer: use serialization for data that isn't json #20

Closed

This was referenced Nov 30, 2023

Extract sequencer APIs as protobuf and implemented public interfaces in terms of these #356

Closed

Move sequencer block data, namespace to proto #265

Closed

	// The proof that the rollup transactions are included in sequencer block.
	// The proof that the rollup transactions are included in sequencer block header.

	/// Converts from the raw decoded protobuf representatin of this type.
	/// Converts from the raw decoded protobuf representation of this type.

	pub fn group_sequence_actions_in_signed_transaction_transactions_by_rollup_id(
	pub fn group_sequence_actions_in_signed_transactions_by_rollup_id(

		@@ -1,24 +1 @@
		// Calculates the `last_commit_hash` given a Tendermint [`Commit`].

	/// + cannot be decode/convert to [`CelestiaRollupBlob`]
	/// + cannot decode/convert to [`CelestiaRollupBlob`]

	/// + the proof, ID, and transactions recorded in the blob cannot be verified against the seuencer
	/// + the proof, ID, and transactions recorded in the blob cannot be verified against the sequencer

feat: redefine sequencer blocks, celestia blobs as protobuf #395

feat: redefine sequencer blocks, celestia blobs as protobuf #395

Uh oh!

Conversation

SuperFluffy commented Sep 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Background

Changes

Testing

Related Issues

Uh oh!

SuperFluffy commented Sep 19, 2023

Uh oh!

Uh oh!

noot left a comment

Choose a reason for hiding this comment

Uh oh!

SuperFluffy commented Sep 20, 2023

Uh oh!

noot commented Sep 20, 2023

Uh oh!

Uh oh!

SuperFluffy commented Sep 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noot Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noot Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperFluffy Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noot Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

SuperFluffy commented Sep 19, 2023 •

edited

Loading

noot Nov 30, 2023 •

edited

Loading

noot Nov 30, 2023 •

edited

Loading

SuperFluffy Nov 30, 2023 •

edited

Loading

noot Nov 30, 2023 •

edited

Loading