refactor: enforce sequencer blob invariants, simplify conductor #576
Conversation
This file contains hidden or 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
SuperFluffy
commented
Nov 10, 2023
| return Err(Error::ChainIdsCommitmentVerification); | ||
| } | ||
|
|
||
| // genesis and height 1 do not have a last commit |
Contributor
Author
SuperFluffy
commented
Nov 10, 2023
| return Err(Error::ActionTreeRootVerification); | ||
| } | ||
|
|
||
| let chain_ids_commitment_hash = sha2::Sha256::digest(chain_ids_commitment); |
Contributor
Author
There was a problem hiding this comment.
note: all of this is rolled into assert_chain_ids_are_included because that's used elsewhere.
SuperFluffy
commented
Nov 10, 2023
| /// | ||
| /// # Errors | ||
| /// Returns one of the variants described in [`ChainIdsVerificationFailure`] as an error. | ||
| pub fn assert_chain_ids_are_included<'a, TChainIds: 'a>( |
Contributor
Author
There was a problem hiding this comment.
This function is now also used when constructing SequencerBlockData.
I also noticed that we were not ensuring that the chain ids commitment/root was checked, so it felt natural to also add that here. This essentially extends #548
aff5474 to
5796f0e
Compare
da8aca0 to
72794ca
Compare
noot
reviewed
Nov 17, 2023
noot
reviewed
Nov 17, 2023
noot
reviewed
Nov 17, 2023
noot
reviewed
Nov 17, 2023
noot
reviewed
Nov 17, 2023
noot
reviewed
Nov 17, 2023
noot
approved these changes
Nov 17, 2023
Contributor
noot
left a comment
There was a problem hiding this comment.
looks good, just a few smaller comments!
+ sequencer-types: make data_hash being a valid sha256 hash invariant + sequencer-types: provide a utility fn to check chain IDs + conductor: simplify block verifier by enforcing sequencer blob invariants
dd3b730 to
71c549c
Compare
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.
Summary
Enforce invariants when constructing sequencer blobs.
Background
The sequencer blob type
SequencerNamespaceDatathat was written to and read from celestia was treated as a dumb container, delegating consistency checks to downstream users (for example, to theblock_verifiermodule inastria-conductor). In the spirit of making invalid states unrepresentable, this patch introdues aRawSequencerNamespaceDataas the dumb (de)serialization container, from which a verifiedSequencerNamespaceDatacan be built. In particular, the following invariants are upheld at construction, rejecting a conversion otherwise (SMDstands forSequencerNamespaceDataandSMD.fieldrefers to thefieldof the same):SMD.header.data_hashmust be setSMD.block_hashmust match the result ofSMD.header.hash()SMD.action_tree_rootandSMD.action_tree_root_inclusion_proofmust verify againstSMD.data_hashSMD.rollup_chain_idsmust matchSMD.chain_ids_commitmentSMD.chain_ids_commitmentandSMD.chain_ids_commitment_proofmust verifiy againstSMD.data_hashWith these changes conductor's
BlockVerifiercan be simplified significantly and is now only responsible for checking the validity of a given sequencer blob against the remote sequencer.Changes
RawSequencerNamespaceDatatypeSequencerNamespaceDatafields private, add read-only gettersSequencerNamespaceDataserialization to be in terms of theRaw*typeSequencerNamespaceData::try_from_rawas the only fallible constructorSMD::data_hashgetter for convenience which panics if the invariants are violatedBlockVerifierby removing all checks that now happen duringSequencerNamespaceDataconstructionTesting
How are these changes tested?
Related Issues
This is done as part of #356
Blocked on #554 being merged