ralexstokes / ssz-rs

Implementation of ethereum's `ssz`
Apache License 2.0
103 stars 41 forks source link

2 issues in test_basic_proof function. #103

Closed readygo67 closed 7 months ago

readygo67 commented 1 year ago

In merkleization>proof.rs file, the test_basic_proof function is confusing.

  1. because the depth is 3, the branch should only has 3 elements.
  2. the index does not follow the Generalized Merkle tree index in https://github.com/protolambda/eth2.0-ssz/blob/master/specs/navigation/generalized_indices.md. I guess the index = 2^3 +2 = 10.
    fn test_basic_proof() {
        let leaf = decode_node_from_hex(
            "94159da973dfa9e40ed02535ee57023ba2d06bad1017e451055470967eb71cd5",
        );
        let branch = [
            "8f594dbb4f4219ad4967f86b9cccdb26e37e44995a291582a431eef36ecba45c",
            "f8c2ed25e9c31399d4149dcaa48c51f394043a6a1297e65780a5979e3d7bb77c",
            "382ba9638ce263e802593b387538faefbaed106e9f51ce793d405f161b105ee6",
            "c78009fdf07fc56a11f122370658a353aaa542ed63e44c4bc15ff4cd105ab33c",
        ]
        .into_iter()
        .map(decode_node_from_hex)
        .collect::<Vec<_>>();
        let depth: usize = 3;
        let index = 2;
        let root = decode_node_from_hex(
            "27097c728aade54ff1376d5954681f6d45c282a81596ef19183148441b754abb",
        );

        assert!(is_valid_merkle_branch(&leaf, branch.iter(), depth, index, &root))
    }

after fix these 2 issue, the test case is as following, and can pass.

    fn test_basic_proof() {
        let leaf = decode_node_from_hex(
            "94159da973dfa9e40ed02535ee57023ba2d06bad1017e451055470967eb71cd5",
        );
        let branch = [
            "8f594dbb4f4219ad4967f86b9cccdb26e37e44995a291582a431eef36ecba45c",
            "f8c2ed25e9c31399d4149dcaa48c51f394043a6a1297e65780a5979e3d7bb77c",
            "382ba9638ce263e802593b387538faefbaed106e9f51ce793d405f161b105ee6",
            // "c78009fdf07fc56a11f122370658a353aaa542ed63e44c4bc15ff4cd105ab33c",
        ]
        .into_iter()
        .map(decode_node_from_hex)
        .collect::<Vec<_>>();
        let depth: usize = 3;
        let index = 10; 
        let root = decode_node_from_hex(
            "27097c728aade54ff1376d5954681f6d45c282a81596ef19183148441b754abb",
        );

        assert!(is_valid_merkle_branch(&leaf, branch.iter(), depth, index, &root))
    }
ralexstokes commented 1 year ago

this test case is testing the functionality in is_valid_merkle_branch which is an implementation of this spec function: https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/beacon-chain.md#is_valid_merkle_branch

given that this test passes, im inclined to leave it as-is

the generalized index view is a bit different from the one assumed here and I have a partial implementation here: https://github.com/ralexstokes/ssz-rs/pull/88

if you wanted to make an additional test to reflect your code here, I'd accept a PR :)

ralexstokes commented 7 months ago

have been working on the Merkle proving code lately so how this works is fresh:

the is_valid_merkle_branch works for a 0-indexed depth, and a 0-indexed index for the leaf's position in the bottom layer of the tree. so then, it makes sense that a merkle proof this verifier expects would have 4 nodes for a depth of 3 (which implies 4 layers of the tree, and the four siblings needed to verify each height starting from the leaf are given in branch).

I wouldn't bring in the concern of generalized indices here -- if you want to see how to map from them to the is_valid_merkle_branch setting, see here: https://github.com/ralexstokes/ssz-rs/blob/d42b0bb5cbd9277abe4c8c71c931c6cfcf64f4bd/ssz-rs/src/merkleization/proofs.rs#L184

I've also updated this particular test to use both verification routes, see here and here

I'll go ahead and close this issue as I expect things are self-consistent and I'll be testing against the Merkle proof test cases in the consensus spec tests soon