feat(sequencer): implement proving of action tree root inside `Header::data_hash` #286

noot · 2023-08-18T22:57:50Z

Summary

implement generation of merkle inclusion proof of the action tree commitment (first "tx" in a sequencer block) inside the block header's data_hash (aka transactions root)

Background

required for validation of data inside conductor

Changes

update SequencerBlockData and SequencerNamespaceData to contain action_tree_root and action_tree_root_inclusion_proof
SequencerBlockData::from_tendermint_block now generates the inclusion proof
SequencerBlockData::new() checks the inclusion proof

Testing

unit tests

Related Issues

#153 (does not close yet, follow up will close it)

…r-commitments

…ria into noot/sequencer-proofs

…r-commitments

…ria into noot/sequencer-proofs

…r-commitments

joroshiba · 2023-08-22T19:40:29Z

crates/astria-conductor/src/types.rs

+        // we don't need to verify the action tree root here,
+        // as [`SequencerBlockData`] would not be constructable
+        // if it was invalid
+


nit: remove empty line between code + comment?

joroshiba · 2023-08-22T19:42:16Z

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

+        action_tree_root_inclusion_proof
+            .verify(action_tree_root.as_bytes(), data_hash)
+            .wrap_err("failed to verify action tree root inclusion proof")?;
+        // TODO: also verify last_commit_hash (#270)


nit: full link preferable

Please provide a full link to the issue or PR, like so:

Suggested change

// TODO: also verify last_commit_hash (#270)

// TODO(https://github.com/astriaorg/astria/issues/270): also verify last_commit_hash

joroshiba · 2023-08-22T19:43:31Z

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

@@ -97,12 +122,23 @@ impl SequencerBlockData {

    #[allow(clippy::type_complexity)]
    #[must_use]
-    pub fn into_values(self) -> (Hash, Header, Option<Commit>, HashMap<Namespace, RollupData>) {
+    pub fn into_values(
+        self,


Is this formatting correct? Feels weird to push the self to new line because the return is

this is what rustfmt wants :|

joroshiba · 2023-08-22T19:53:45Z

crates/astria-sequencer-validation/Cargo.toml

@@ -4,7 +4,7 @@ version = "0.1.0"
 edition = "2021"

 [dependencies]
-ct-merkle = "0.1"
+ct-merkle = { version = "0.1", features = ["serde"] }


Should this be using the workspace version?

the workspace version is under a patch, not an actual import, so it overrides this w/ the patch

SuperFluffy

Saw a few things that should be fixed prior to accepting.

Orthogonal to this PR I had a chance to read the wrapper of CtMerkle. I was wondering why you chose to implement this as a newtype wrapper instead of using an extension trait (or simply utility functions)?

The difficulty with wrappers is often that one ends up reimplementing more and more of the methods of the wrapped type, without providing proper documentation. That could be avoided by simply using the type directly.

EDIT: After reading #256 and thinking it through I agree with not directly exposing ctmerkle and its types.

SuperFluffy · 2023-08-23T14:11:09Z

crates/astria-sequencer-validation/Cargo.toml

@@ -4,7 +4,7 @@ version = "0.1.0"
 edition = "2021"

 [dependencies]
-ct-merkle = "0.1"
+ct-merkle = { version = "0.1", features = ["serde"] }


ct-merkle is imported here, but there is a ct-merkle in the workspace cargo toml that refers to a git repo under the astria org.

yeah that's under the patch section (i had to make some changes to the lib, see here rozbb/ct-merkle#2)

SuperFluffy · 2023-08-23T14:12:29Z

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

 pub struct SequencerBlockData {
    pub(crate) block_hash: Hash,
    pub(crate) header: Header,
    /// This field should be set for every block with height > 1.
    pub(crate) last_commit: Option<Commit>,
    /// namespace -> rollup data (chain ID and transactions)
    pub(crate) rollup_data: HashMap<Namespace, RollupData>,
+    /// The root of the action tree for this block.
+    pub(crate) action_tree_root: Hash,
+    /// The inclusion proof that the action tree root in included


typo:

Suggested change

/// The inclusion proof that the action tree root in included

/// The inclusion proof that the action tree root is included

SuperFluffy · 2023-08-23T14:13:44Z

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

+        action_tree_root_inclusion_proof
+            .verify(action_tree_root.as_bytes(), data_hash)
+            .wrap_err("failed to verify action tree root inclusion proof")?;
+        // TODO: also verify last_commit_hash (#270)


Please provide a full link to the issue or PR, like so:

Suggested change

// TODO: also verify last_commit_hash (#270)

// TODO(https://github.com/astriaorg/astria/issues/270): also verify last_commit_hash

SuperFluffy · 2023-08-23T14:23:17Z

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

@@ -97,12 +122,23 @@ impl SequencerBlockData {

    #[allow(clippy::type_complexity)]
    #[must_use]
-    pub fn into_values(self) -> (Hash, Header, Option<Commit>, HashMap<Namespace, RollupData>) {
+    pub fn into_values(


This is a code smell. We should differentiate between a "raw" SequencerBlockData as a (de)serialization format (where it's ok to make all fields pub), and "verified" SequencerBlockData that upholds invariants (where you want to prevent anyone pulling the rug from under you). Permitting clippy::type_complexity proves that point.

My suggestion: keep the SequencerBlockData as is, but introduce a RawSequencerBlockData that has exactly the same fields, but comes with a RawSequencerBlockData::verify(self) -> eyre::Result<SequencerBlockData>. This way you can keep using SequencerBlockData everywhere as is.

Next, scrap SequencerBlockData::into_values and introduce SequencerBlockData::into_raw. This way users can do the following instead of destructuring tuples:

fn do_something(data: SequencerBlockData) { let RawSequencerBlockData { block_hash, ... } = data.into_raw(); further_processing(block_has); }

We then have an easier path forward to move this into astria-proto.

yeah I didn't really like this either but wasn't sure if I should just make another type with all pub fields. will update!

SuperFluffy · 2023-08-23T14:24:15Z

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

@@ -136,19 +172,36 @@ impl SequencerBlockData {
    /// # Errors
    ///
    /// - if the block has no data hash
+    /// - if the block has no transactions


This list is great when eventually moving to typed errors, which we should really do...

SuperFluffy · 2023-08-23T14:28:48Z

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

+        );
+        let action_tree_root_inclusion_proof = tx_tree
+            .prove_inclusion(0) // action tree root is always the first tx in a block
+            .expect("failed to generate inclusion proof for action tree root");


Looks like this whould have been a wrap_err

…r-proofs

…h-proof

SuperFluffy

Looks good to me. Please address the comment re deserialization so that there is no backdoor into creating types where invariants are violated (in this case inclusion proofs).

SuperFluffy · 2023-08-23T20:54:25Z

crates/astria-sequencer/Cargo.toml

@@ -10,6 +10,7 @@ astria-sequencer-validation = { path = "../astria-sequencer-validation" }
 telemetry = { package = "astria-telemetry", path = "../astria-telemetry" }

 anyhow = "1"
+digest = "0.10"


SuperFluffy · 2023-08-23T20:59:41Z

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

@@ -63,15 +72,29 @@ impl SequencerBlockData {
        header: Header,
        last_commit: Option<Commit>,
        rollup_data: HashMap<Namespace, RollupData>,
+        action_tree_root: Hash,
+        action_tree_root_inclusion_proof: InclusionProof,


Comment on this entire new constructor: now that you have RawSequencerBlockData, I would replace this ever-growing function with SequencerBlockData::try_from_raw or smth like that.

SuperFluffy · 2023-08-23T21:01:56Z

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

-            self.last_commit,
-            self.rollup_data,
-        )
+    pub fn into_raw(self) -> RawSequencerBlockData {


Please add a comment on top of this function.

Nit: destructuring self looks slightly nicer (less self.)

let Self { block_hash, header, // and the rest } = self; RawSequencerBlockData { block_hash, header, // and the rest }

SuperFluffy · 2023-08-23T21:03:42Z

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

@@ -42,14 +46,19 @@ pub struct RollupData {

 /// `SequencerBlockData` represents a sequencer block's data
 /// to be submitted to the DA layer.
-#[derive(Serialize, Deserialize, Clone, Debug, Eq, PartialEq)]
+#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]
 pub struct SequencerBlockData {
    pub(crate) block_hash: Hash,


Do you still need all of this to be pub(crate)?

SuperFluffy · 2023-08-23T21:09:39Z

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

@@ -42,14 +46,19 @@ pub struct RollupData {

 /// `SequencerBlockData` represents a sequencer block's data
 /// to be submitted to the DA layer.
-#[derive(Serialize, Deserialize, Clone, Debug, Eq, PartialEq)]
+#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)]


serde provides this attribute: https://serde.rs/container-attrs.html#try_from

Can you implement a TryFrom<RawSequencerBlockData> (I'd prefer to not impl a single-use TryFrom and instead use a custom deserializer, but in this case it's probably fine).

This way you can still deserialize, but in addition you get verification. Right now you still serialize directly into a SequencerBlockData and have no assurance that it's legitimate (invariants upheld, etc).

Also see https://serde.rs/container-attrs.html#into for the inverse

nice, added!

* main: fix(sequencer): fix `deliver_tx` to accept the action tree root (#291) feat(composer): adding more docs (#295) feat(sequencer): implement proving of action tree root inside `Header::data_hash` (#286) ci: Update to reusable success, nested names (#306) feat(sequencer, relayer): generate and write merkle inclusion proofs as part of `RollupNamespaceData` (#256) ci: broad restructuring of worklows (#280)

noot added 30 commits August 2, 2023 17:46

impl generation of action tree commitment

04b07ea

implement process proposal verification

a7e63cb

fix

e6dd52e

clarify comment

7316d67

comment

d639e3b

unit tests

85d0f14

Merge branch 'main' of github.com:astriaorg/astria into noot/sequence…

ab254bd

…r-commitments

wip

d40dd4e

address comments, add more tests

d10c8a5

more tests, test leaf ordering

9b80306

add spec document

d2e0d2a

update generate_sequence_actions_commitment to not error

282847a

Merge branch 'main' of github.com:astriaorg/astria into noot/sequence…

6b52b1e

…r-commitments

update handle_prepare_proposal

42d2a72

return filtered transactions from generate_sequence_actions_commitment

12149cd

merge

1c99a6e

Merge branch 'main' of github.com:astriaorg/astria into noot/sequence…

6abaf7b

…r-commitments

update spec doc

c918ea4

add snapshot test

e22fe2e

Merge branch 'noot/sequencer-commitments' of github.com:astriaorg/ast…

c8f233c

…ria into noot/sequencer-proofs

begin inclusion proofs w ct_merkle lib

1525a24

include inclusion proof when writing rollup namespace data

c0f0a42

Merge branch 'main' of github.com:astriaorg/astria into noot/sequence…

1b0df8e

…r-commitments

Merge branch 'noot/sequencer-commitments' of github.com:astriaorg/ast…

ef58375

…ria into noot/sequencer-proofs

lint

c38f671

Merge branch 'noot/sequencer-commitments' of github.com:astriaorg/ast…

744dca6

…ria into noot/sequencer-proofs

Merge branch 'main' of github.com:astriaorg/astria into noot/sequence…

243a67a

…r-commitments

remove namespace usage from sequencer

74b5b42

update docs

e8fcd9e

Merge branch 'main' of github.com:astriaorg/astria into noot/sequence…

4215bd8

…r-commitments

joroshiba reviewed Aug 22, 2023

View reviewed changes

SuperFluffy requested changes Aug 23, 2023

View reviewed changes

noot added 6 commits August 23, 2023 12:12

Merge branch 'main' of github.com:astriaorg/astria into noot/sequence…

774bcc2

…r-proofs

use serde for ct-merkle

5325bc3

update verify to take value

09cad77

remove tendermint dep from sequencer-validation

3aa2d8b

add more comments

2a9de23

merge

84c0d4b

noot temporarily deployed to BUF August 23, 2023 16:56 — with GitHub Actions Inactive

create RawSequencerBlocKData

a516d87

noot temporarily deployed to BUF August 23, 2023 17:05 — with GitHub Actions Inactive

fix expect

b4291c3

noot temporarily deployed to BUF August 23, 2023 17:06 — with GitHub Actions Inactive

noot requested review from SuperFluffy and joroshiba August 23, 2023 17:06

Base automatically changed from noot/sequencer-proofs to main August 23, 2023 17:16

github-actions bot added the sequencer pertaining to the astria-sequencer crate label Aug 23, 2023

merge

2f5c3da

noot temporarily deployed to BUF August 23, 2023 17:58 — with GitHub Actions Inactive

Merge branch 'main' of github.com:astriaorg/astria into noot/data-has…

da15a6e

…h-proof

noot temporarily deployed to BUF August 23, 2023 19:33 — with GitHub Actions Inactive

SuperFluffy approved these changes Aug 23, 2023

View reviewed changes

address comments

61ba7e4

github-actions bot removed the sequencer pertaining to the astria-sequencer crate label Aug 23, 2023

noot temporarily deployed to BUF August 23, 2023 22:07 — with GitHub Actions Inactive

fix test

8d8e3f2

noot temporarily deployed to BUF August 23, 2023 22:54 — with GitHub Actions Inactive

noot merged commit 9ea7269 into main Aug 24, 2023

noot deleted the noot/data-hash-proof branch August 24, 2023 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sequencer): implement proving of action tree root inside `Header::data_hash` #286

feat(sequencer): implement proving of action tree root inside `Header::data_hash` #286

noot commented Aug 18, 2023

joroshiba Aug 22, 2023

joroshiba Aug 22, 2023

SuperFluffy Aug 23, 2023

joroshiba Aug 22, 2023

noot Aug 23, 2023

joroshiba Aug 22, 2023

noot Aug 23, 2023

SuperFluffy left a comment •

edited

Loading

SuperFluffy Aug 23, 2023

noot Aug 23, 2023

SuperFluffy Aug 23, 2023

SuperFluffy Aug 23, 2023

SuperFluffy Aug 23, 2023

noot Aug 23, 2023

SuperFluffy Aug 23, 2023

SuperFluffy Aug 23, 2023

SuperFluffy left a comment

SuperFluffy Aug 23, 2023

noot Aug 23, 2023

SuperFluffy Aug 23, 2023

SuperFluffy Aug 23, 2023

SuperFluffy Aug 23, 2023

noot Aug 23, 2023

SuperFluffy Aug 23, 2023

noot Aug 23, 2023

	// TODO: also verify last_commit_hash (#270)
	// TODO(https://github.com/astriaorg/astria/issues/270): also verify last_commit_hash

	/// The inclusion proof that the action tree root in included
	/// The inclusion proof that the action tree root is included

feat(sequencer): implement proving of action tree root inside Header::data_hash #286

feat(sequencer): implement proving of action tree root inside Header::data_hash #286

Conversation

noot commented Aug 18, 2023

Summary

Background

Changes

Testing

Related Issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperFluffy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperFluffy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(sequencer): implement proving of action tree root inside `Header::data_hash` #286

feat(sequencer): implement proving of action tree root inside `Header::data_hash` #286

SuperFluffy left a comment •

edited

Loading