-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(sidecar): constraints client #212
feat(sidecar): constraints client #212
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Requested some small changes
fn total_leaves(&self) -> usize { | ||
4 + self.constraints.len() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we start at 4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, I took it from the bolt-boost impl by @mempirate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad I haven't noticed it in his PR, but I'm pretty sure it is because the way you encode the struct (which has
@mempirate could you please confirm and document the number in the bolt-boost
impl as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I'm not sure about this computation since it should take into account chunking and padding: https://github.com/ethereum/consensus-specs/blob/dev/ssz/simple-serialize.md#merkleization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have 3 "unit" types + a list, which will add another leaf for the length + leaves for each individual elements! Will document
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized I haven't tested this - @namn-grg could you add a quick test that generates a random ConstraintsMessage
and calculates the hash tree root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mempirate added lmk how it looks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, we can improve it with the use of arbitrary
and arbtest
for prop testing but random is good enough for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test only asserts that hash_root
is 32 bytes long but that's trivial due to the signature of the hash_tree_root
method. @mempirate how do we intend to test this?
In my opinion we should compare the output of this implementation with what fastssz outputs once having generated the hash tree root implementation for this struct using codegen: https://github.com/ferranbt/fastssz/blob/main/sszgen/main.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some final perf improvements requested
Context
This PR update the mevboost client to constraint client with support of the new updated constraints api
Closes #208