FuelLabs / fuel-merkle

Fuel Merkle trees in Rust.
Apache License 2.0
7 stars 5 forks source link

feat: Data-driven BMT test generation and framework #125

Closed bvrooman closed 1 year ago

bvrooman commented 2 years ago

Related issues:

This PR introduces a test suite for generating BMT proofs and their expected verification result. The test suite includes a number of positive test cases (verification is expected to be true) and some negative test cases (verification is expected to be false).

The test suite can be generated using cargo run --bin write-bmt-test-suite. Test data is randomly sampled from the set of hashed integers. The RNG is seeded with a constant value such that each time the test suite is generated, the test data is consistent.

This PR also adds these tests to the fuel-merkle test suite executed during cargo test. The framework used by SMT data-driven tests has been refactored to apply to BMTs as well.

bvrooman commented 2 years ago

Thanks for the review!

If I understand your suggestion correctly, you're proposing:

My concern with this approach is that it is coupling a proof with the tree that generated it. In this format, it seems I need to have a valid tree to begin with so that the coordinates point to a spot in the tree. How do I specify an invalid root hash or invalid proof hash?

I would also argue that the test vectors should align syntactically with the system being tested. In this case, the contract's verify function takes in the parameters supplied by the test, almost as-is. If we go with an alternative format, the test implementor is responsible for transforming the test vector into a format that it can consume.

Perhaps if you can elaborate on the suggested format, I can understand better how it meets the goals of the test suite.

bvrooman commented 2 years ago

This PR in fuel-merkle-sol demonstrates how the generated test suite is used to validate the solidity implementation. This would need final confirmation on the format of the generated tests before we can commit the tests in fuel-merkle-sol.

freesig commented 2 years ago

Yeh maybe my idea was not the best. I guess the bigger picture is it would be nice if I could look at the fixtures and understand why it should be expected to verify or not. Currently it's not clear why each fixture would be valid or not. Maybe even something as simple as a comment in the yaml stating why it should validate or not?

bvrooman commented 2 years ago

I agree that it would certainly be helpful to have the extra context as to why the test case should verify or not. I updated the test cases to include a description field that briefly describes why the test case should result in a passed or failed verification.

xgreenx commented 1 year ago

Hmm, is it okay that yaml files are not the same as in TS PR?

bvrooman commented 1 year ago

I will copy the files over to the TS PR once the files here are finalized and merged. Thanks for the review @xgreenx. I will wait today for anyone else on the client team to review if they wish.