Add checkpoint artifacts digest with merkle trees and gRPC support #22431
Conversation
- Add grpc-reader crate for gRPC communication - Introduce merkle tree implementation in shared-crypto - Implement checkpoint commitment changes in sui-core - Update proto files for checkpoint summary - Update various test files and dependencies
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
| @@ -647,12 +647,12 @@ anemo-cli = { git = "https://github.com/mystenlabs/anemo.git", rev = "9c52c3c794 | |||
| anemo-tower = { git = "https://github.com/mystenlabs/anemo.git", rev = "9c52c3c7946532163a79129db15180cdb984bab4" } | |||
|
|
|||
| # core-types from the new sdk for use in gRPC | |||
| sui-sdk-types = { git = "https://github.com/MystenLabs/sui-rust-sdk.git", rev = "7d485ababecf7282dc7d772316ccdb07b9b24753", features = [ | |||
| sui-sdk-types = { git = "https://github.com/MystenLabs/sui-rust-sdk.git", rev = "6820f04ef77f050a0a892bed40a537861cd295f9", features = [ | |||
There was a problem hiding this comment.
just a note that the changes from that commit will need to be merged into the sdk repo before this pr lands
| /// The data of the leaves is prefixed with `0x00` before hashing and hashes of inner nodes are | ||
| /// computed over the concatenation of their children prefixed with `0x01`. Hashes of empty | ||
| /// subtrees (i.e. subtrees without data at their leaves) are replaced with all zeros. |
There was a problem hiding this comment.
do we want to use different prefixes to ensure domain separation from other domains in Sui (dynamic children objects, address derivation, etc)?
There was a problem hiding this comment.
Hmm, not sure I follow. The above is mostly an internal detail of the Merkle Tree library, so not sure I see the problem. Are you saying we need to domain separate leaves of different types? But that is taken care of by the enums + bcs, no?
There was a problem hiding this comment.
The way hashes are calculated in the various places in Sui is quite complicated to say the least...
| @@ -390,7 +390,7 @@ pub trait TransactionEffectsAPI { | |||
| fn unsafe_add_object_tombstone_for_testing(&mut self, obj_ref: ObjectRef); | |||
| } | |||
|
|
|||
| #[derive(Clone)] | |||
| #[derive(Clone, Debug, Serialize, Deserialize)] | |||
| pub struct ObjectChange { | |||
There was a problem hiding this comment.
This PR is making the struct ObjectChange a new public data structure since its included in the merkle tree now. We should make sure we're happy with doing this. Right now this type is only an internal library type.
| // Object state before and after this checkpoint for all touched objects | ||
| AccumulatedObjectChange(ObjectChange), | ||
| // Execution digests for all transactions in this checkpoint | ||
| ExecutionDigests(ExecutionDigests), |
There was a problem hiding this comment.
Do we need execution digests? this is already committed to via the checkpoint itself (checkpoint contents digest)
There was a problem hiding this comment.
Indeed, but the benefits of adding it here are:
- shorter inclusion proofs (for tx / event / object) as we avoid touching checkpoint contents. Mainly useful if you want to post this proof on another chain like Ethereum.
- this one is more of a guess as I don't know of a concrete app that wants it: non-inclusion proofs to prove that a certain tx did not happen.
There was a problem hiding this comment.
At that point I'd argue we should just introduce a contents V2 because this is wasteful to have it replicated twice
There was a problem hiding this comment.
Hmm, fair enough. At this point, I am not quite sure if it makes sense to include execution digests in a sorted merkle tree as it seems a little unlikely that someone would want a non-inclusion proof for a tx digest. Also we lose the causal order of transactions which seems important (if we modify contents like you suggest). So perhaps we should have two trees:
- A sorted tree for things where both non-inclusion and inclusion proofs are possible, e.g., objects, events
- A normal tree where only inclusion proofs are supported, e.g., execution digests
Perhaps contents could take on the role of the second tree (only inclusion proofs) and the new DS I am planning to add could take on the role of the tree that supports both inclusion and non-inclusion proofs. What do you think?
Description
Test plan
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.