bugfix(rpc): Fix proof RPC #496
Conversation
notlesh
added
bug
| mod tests { | ||
| use bitvec::bits; | ||
|
|
||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test_path_to_felt() { | ||
| let path = bits![u8, Msb0; 0, 0]; | ||
| assert_eq!(path.len(), 2); | ||
| let felt = path_to_felt(path); | ||
| assert_eq!(felt, Felt::ZERO); | ||
|
|
||
| let path = bits![u8, Msb0; 1]; | ||
| assert_eq!(path.len(), 1); | ||
| let felt = path_to_felt(path); | ||
| assert_eq!(felt, Felt::ONE); | ||
| } | ||
| } |
There was a problem hiding this comment.
Please add some tests verifying the format of the getProof endpoint so this kind of error does not happen again in the future. Perhaps there is a way to test this against SNOS directly? Anyway, even if we do not go that far we should definitely have tests for this case.
There was a problem hiding this comment.
i wish the specs came with test vectors :(
There was a problem hiding this comment.
Perhaps there is a way to test this against SNOS directly?
Testing this against SNOS was how we discovered this problem to begin with.
More testing would be great, but I could use some tips. Test vectors would be perfect (e.g. something like (existing_mpt, nodes_requested) -> proof). Is there anything close to this already (in other parts of Madara or in Bonsai Trie)?
If not, I imagine something like this:
- Start with empty MPT
- insert some leaves
- get proof
- assert exact structure of proof
- write a quick verify_proof fn, something like what we have in SNOS
There was a problem hiding this comment.
I could also argue that this is out of scope for this PR -- testing this could be very thorough. For example, we should also test non-existence proofs...
| @@ -30,6 +30,7 @@ pub struct NodeHashToNodeMappingItem { | |||
| pub struct ContractLeavesDataItem { | |||
| pub nonce: Felt, | |||
| pub class_hash: Felt, | |||
| pub storage_root: Felt, | |||
There was a problem hiding this comment.
I think we should add some doc comments which provide an overview of the get_proof endpoint format. Given that the get_storage_proof function can seem quite arcane I believe this would be a good idea and help with maintainability.
| @@ -150,6 +151,24 @@ pub fn get_storage_proof( | |||
| bonsai_identifier::CLASS, | |||
| class_hashes, | |||
| )?; | |||
|
|
|||
| let mut contract_root_hashes = std::collections::HashMap::new(); | |||
There was a problem hiding this comment.
ah yes, it looks like this was added into the specs
thanks!
| @@ -22,7 +22,8 @@ fn saturating_sum(iter: impl IntoIterator<Item = usize>) -> usize { | |||
| fn path_to_felt(path: &BitSlice<u8, Msb0>) -> Felt { | |||
| let mut arr = [0u8; 32]; | |||
| let slice = &mut BitSlice::from_slice_mut(&mut arr)[5..]; | |||
| slice[..path.len()].copy_from_bitslice(path); | |||
| let slice_len = slice.len(); | |||
| slice[slice_len - path.len()..].copy_from_bitslice(path); | |||
There was a problem hiding this comment.
makes sense
maybe add a tiny comment saying that slice_len is always equal to 251 and this minus sign is fine because path cannot be larger than 251, it took me a minute to understand :)
| mod tests { | ||
| use bitvec::bits; | ||
|
|
||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test_path_to_felt() { | ||
| let path = bits![u8, Msb0; 0, 0]; | ||
| assert_eq!(path.len(), 2); | ||
| let felt = path_to_felt(path); | ||
| assert_eq!(felt, Felt::ZERO); | ||
|
|
||
| let path = bits![u8, Msb0; 1]; | ||
| assert_eq!(path.len(), 1); | ||
| let felt = path_to_felt(path); | ||
| assert_eq!(felt, Felt::ONE); | ||
| } | ||
| } |
There was a problem hiding this comment.
i wish the specs came with test vectors :(
| let pending_block = finalized_block_zero(header); | ||
| backend.store_block( | ||
| pending_block, | ||
| finalized_state_diff_zero(), |
There was a problem hiding this comment.
I think passing in an empty state diff is causing this to fail, even though I'm inserting values manually into the bonsai_trie. Currently we get back an empty proof.
This fixes the proof RPC, specifically:
felts properlyWhat is the current behavior?
Currently the RPC copies the bits from an edge path incorrectly into a felt. This fixes that and adds a couple trivial tests.
Also, the storage root for a contract was not accessible (other than as one of the random proof nodes), this adds it to the
ContractLeavesData.What is the new behavior?
RPC is now providing valid proofs, which are passing
fn verify_proof()in SNOS.Does this introduce a breaking change?
No