feat: require chain_id be 32 bytes#436
Merged
Merged
Conversation
59d0b0f to
53014b4
Compare
0d8bc9a to
5e9c9b3
Compare
5e9c9b3 to
c50cd78
Compare
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
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
Make chain IDs 32 bytes and construct them from sha256-hashed names.
Background
At the moment chain IDs sent as part of sequence actions can be arbitrarily long. The only place where an upper limit on chain IDs is enforced is in
SequencerBlockData, which however is constructed in sequencer relayer and after cometbft/sequencer have come to a consensus over new data.Using a chain ID of 32 bytes as opposed to a vec is desirable for the these reasons:
Copyso that unnecessary vec-allocations are avoidedChanges
ChainIda think wrapper over[u8; 32](away fromVec<u8>)ChainIdtoastria-protoChainIdbeing a part ofSequenceAction, whereas before it was just a byte vector)Testing
All tests still pass or have been adjusted. The snapshot test for calculating the merkle tree hash over the actions had to be updated.
Breaking Changelist
This breaks all public APIs because chain IDs are now expected to be 32 bytes (whereas they were arbitrary length before).