-
Notifications
You must be signed in to change notification settings - Fork 28
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: da verifier #129
feat: da verifier #129
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
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, as always!
I'll go ahead and merge this into my feat/da-verifier
branch, clean up the TODOs + comments, and change the types as mentioned in the comments
|
||
// TODO: Error naming & other naming: to enum? | ||
// I am not sure what is better practice, so far we have been using mostly |
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 made this comment because the error names were so long they wouldn't fit into a felt string. But it is probably better to stick to const felts and use abbreviations, like how it is set up now.
// TODO: data_root_tuple -> data_root & data_root_tuple_root -> data_root? | ||
// not sure what you mean here |
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 referred to changing the corresponding names to make errors and function names shorter. I think it is fine to leave it as it is now, though.
//TODO: to u32? | ||
let mut number_of_shares_in_proofs: u256 = 0; | ||
// TODO: to u32? | ||
// currently max square size is 128 => extended square size 256 |
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.
Thanks for looking into this! Changing the key fields also would be best ( I can do this ); fewer felts == better.
@@ -130,6 +179,10 @@ mod DAVerifier { | |||
) -> (bool, felt252) { | |||
// check that the data root was commited to by the Blobstream smart contract | |||
// TODO: safe unwrap? | |||
// a choice was made to use a u64 instead of a u256 (from the solidity contract) | |||
// for the `state_proof_nonce` in the blobstreamx contract | |||
// I suppose the `commit_nonce` field should actually be a u64 then which would |
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 agree
// currently max square size is 128 => extended square size is 256 | ||
// max number of side nodes in a a binary tree with 256 leaves is log2(256) = 8 | ||
let mut i: u32 = 0; | ||
// same for `extended_square_row_size`, but defining them as u32 may provide some |
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.
Defining them as u32 is probably best because it provides extra room, as you mentioned, and makes a comparison with len()
easier.
Also, there aren't really any performance improvements there since both u8
and u32
are interpreted as 1 felt on the back end.
ab995e5
into
keep-starknet-strange:feat/da-verifier
* Initial work for DA verifier and rollup inclusion proof test * cleanup todos * Scarb fmt * feat: da verifier (#129) * Added doc * test_verifier WIP * test_verifier done * Update * Added doc * test_rollup_inclusion_proofs WIP * test_rollup_inclusion_proofs done * TODOs comments * Scarb fmt * Fix types on indexes and things to reduce number of felts needed, cleaned up TODOs * Use sol_abi encode_packed --------- Co-authored-by: 0xK2 <[email protected]>
Hey, I have implemented
test_rollup_inclusion_proofs
andtest_da_verifier
.test_rollup_inclusion_proofs::test_invalid_data
is ignored because it exceeds default max n steps and needs to be run with--max-n-steps
flag.I also made a few changes to
da_verifier
. This is 95% done, I left comments below the TODOs for you to review. I will make necessary changes afterwards.