wip: type for slice root #518
Conversation
|
Obviously it's unfinished 🐸 struggling with cyclic dependencies right now. |
akhi3030
force-pushed
the
kobi-merkle-root-type
branch
from
dff580a to
cca4098
Compare
slice-root/src/lib.rs
Outdated
| use solana_hash::Hash; | ||
|
|
||
| #[derive(Copy, Clone, Debug, PartialEq, Eq)] | ||
| pub struct SliceRoot(pub Hash); |
There was a problem hiding this comment.
| pub struct SliceRoot(pub Hash); | |
| pub struct SliceRoot(Hash); |
It will be cleaner to not make the internal field pub. And to have a constructor with appropriate visibility to ensure that "valid" SliceRoots can be constructed.
There was a problem hiding this comment.
Is there an advantage to using a struct here rather than a type alias?
There was a problem hiding this comment.
a type alias serves just as documentation and doesn't offer actual type safety. It is still possible to accidentally pass in the wrong type and the compiler will not complain.
There was a problem hiding this comment.
Right, type alias would be like the mentioned comment that says "this argument means this". I want to make sure and trace correct methods get/return SliceRoot or AlpenglowBlockId and Rust to guide my hand. Is it just me or this is how it should be? 😅
There was a problem hiding this comment.
With this codebase I will get a stroke without Rust holding my hand lol
There was a problem hiding this comment.
Yeah that's fine, i'm cool with the struct approach
slice-root/src/lib.rs
Outdated
| @@ -0,0 +1,7 @@ | |||
| use solana_hash::Hash; | |||
There was a problem hiding this comment.
To reviewers, this crate introduced to break the cyclic dependency. Once, the work here is done, the type definition ca be moved to solana-sdk which is probably the right place for defining SliceRoot.
There was a problem hiding this comment.
Generally SDK represents types that will be used on chain, perhaps it would make sense for this to live in votor-messages or ledger/src/shred.rs instead
There was a problem hiding this comment.
Tried shred.rs, there's a cyclic dep with runtime.
Can try votor-messages, but pretty sure there's a cyclic dependency :(
There was a problem hiding this comment.
votor-messages is designed to be a lightweight leaf crate for types that everyone can depend on. AFAIK it shouldn't depend on any non SDK crates, maybe give it a try?
|
Thought about this some more, since the majority of the Double Merkle work will be in code outside of agave-votor it would be best to do this first upstream in Agave and then cherry-pick it back here if needed. We're almost done upstreaming votor, and the size of the divergence is growing each day so best to work directly on agave every chance we can get. Also will make it easier to get reviews from the code owners for each of the parts, networking team for shreds & steve for blockstore. |
3 participants
Problem
Hash type is ubiquitous and a bit abused. For example, sometimes there's a comment next to some argument clarifying the the Hash type means slice merkle root. This makes it hard to introduce the double merkle block id and check for correctness.
Summary of Changes
Distinguish slice merkle roots with a new type.
Simplified the way the merkle tree implementation is indexed.