Merkle Proof Verification

[bitzoic]

Type of change

• New Library

Changes

The following changes have been made:

• Added Merkle proof library
  • Process Merkle Proof Function
  • Verify Merkle Proof Function
  • Leaf Digest Function
  • Node Digest Function
  • Key Length function
• Added Merkle proof tests

Notes

• There is commented out code until Vec and heap type support fuels-rs#353 is resolved
• The current use of Fuel-Merkle fails due to padding of u8 on Sway side. Issue to track is here: sha256 treats u8 as u64 sway#2594

Related Issues

Closes #6

[Braqzen]

1. README.md
   • The opening sentence is duplicated in the use cases section of the spec. I don't think that this duplication is needed. I think that it makes more sense to have that sentence in the readme rather than the spec too. Furthremove, a short explanation of what the components of the merkle tree are and how these parts can be used (leafs, nodes, roots) would be beneficial.
   • I've tried to make 2 suggestions however github is getting confused with the parsing. There are 3 sections that use a single backtick ( ` ) instead of the full code block while other sections use code blocks. Could you make the following 3 sections into full code blocks?
     • sway_libs = { git = "https://github.com/FuelLabs/sway-libs", branch = "master" }
     • use sway_libs::binary_merkle_proof::verify_proof
     • fuel-merkle = { version = "0.3" }
   • There is a comment regarding pointing at a master branch of the repo. I suggest creating an issue to track this in advance
   • I'm unsure about the verify_proof_example code block being hardcoded into the readme and having just that 1 example there. It also has the postfix of _example at the end for some reason. I would either delete the section of change it somehow. I'm leaning towards the change rather than removal but it's not immediately obvious what I'd do here.
   • When generating a tree there is a loop and inside that loop some data is pushed into the tree while assigning the return output to a variable that is disregarded. Should that assignment be shown or can we just push without assignment?
2. SPECIFICATION.md
   • There's a bit of duplication in regards to the hashing. It's mentioned twice in the notes and something similar is also in the README. Perhaps a temporary section should be added where all the relevant comments are collated so that there is no duplication? I would probably put it in the README since the SPECIFICATION is the next step to read after the README if you care about the content, or even click on it in the first place.
3. Could you add syntax highlighting?
4. binary_merkle_proof.sw
   • I have not mapped out the steps in path_length_from_key however that while true is worrisome. I hope that there are sufficient tests to make sure that each part of the control flow is tested to make sure that it definitely breaks out when it's meant to. I'm no expert here so I hope you can add / have added tests to make sure that this happens
   • I think there might be something wrong with CI or the compiler since line 100 has a return expression without the use of a semi-colon at the end. Is this valid? I don't think it is. Can you verify that this builds and works as expected? Should an issue be raised?
5. Testing
   • I would split the tests into their own function modules similar to how sway-apps has done it
   • I don't really know what's going on with the tests so I've only skim read them however given my comments about the use of loops I'm unsure if all the conditions are tested (simply by looking at the line length and inferring that somewhere around this many lines of code would be just for testing the intended functionality rather than coverage testing).

[bitzoic]

• I have not mapped out the steps in path_length_from_key however that while true is worrisome. I hope that there are sufficient tests to make sure that each part of the control flow is tested to make sure that it definitely breaks out when it's meant to. I'm no expert here so I hope you can add / have added tests to make sure that this happens

The function itself is not public and will only ever be accessed by process_proof. There are checks ahead of calling this function to ensure good data is being passed to it. While my testing is currently limited to do the constraint of using an array, I have a number of tests in process_proof.rs that use different key and num_leaves values.

• I think there might be something wrong with CI or the compiler since line 100 has a return expression without the use of a semi-colon at the end. Is this valid? I don't think it is. Can you verify that this builds and works as expected? Should an issue be raised?

Any return statement does not need a semi-colon in rust. The return keyword however is required as there is code that comes after it. No issue required.

• I don't really know what's going on with the tests so I've only skim read them however given my comments about the use of loops I'm unsure if all the conditions are tested (simply by looking at the line length and inferring that somewhere around this many lines of code would be just for testing the intended functionality rather than coverage testing).

As mentioned above, what I am able to test is pretty limited without the use of a Vec. Changes in the length of a proof is the only thing that is not tested here explicitly. This will be added after FuelLabs/fuels-rs#353 is resolved.

[Braqzen]

1. Could you label each codeblock so that there is syntax highlighting? This would be done by putting the word (without quotes) "rust" right after the first three backticks that open the codeblock. Note that this might have to be done without a space between the ticks and the language.
2. I would personally update the spec to list each function exposed to the user (pub) and inform them of its usage. This would be easier to follow than the ingestion of the spec imo
3. As for the testing, I would not comment out parts of a test that make it fail. I would ignore that test since it's clearly not finished. Commented out sections create a false impression that everything is fine until you see that the test has sections missing. Alternatively, I'd split up those test(s) if possible. Furthermore, since each function is a module I wouldn't wrap it in a mod function_name and instead I would use a mod success and mod revert at the top. Separating by function names was only necessary when we had many functions in 1 file.

[bitzoic]

3. As for the testing, I would not comment out parts of a test that make it fail. I would ignore that test since it's clearly not finished. Commented out sections create a false impression that everything is fine until you see that the test has sections missing.

The commented out section was left to show the use of a u8 which causes other tests to fail, but this clearly doesn't make sense to be here. Removed and updated the comment above the test to inform the reader.

[matt-user]

lgtm

[Braqzen]

cc @simonr0204 for notification.
Github is being stupid and it's not providing an option to re-request a review from you hence the tag.