turbofish-org / merk

High-performance Merkle key/value store
Apache License 2.0
226 stars 36 forks source link

Verify feature #52

Closed cwlittle closed 3 years ago

cwlittle commented 3 years ago

Splits merk into full and verify feature, default import being full + verify. This allows for use of verify logic without heavier dependencies.

codecov-commenter commented 3 years ago

Codecov Report

Merging #52 (92acf33) into develop (c6c9307) will decrease coverage by 0.53%. The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #52      +/-   ##
===========================================
- Coverage    92.01%   91.47%   -0.54%     
===========================================
  Files           24       24              
  Lines         3157     3157              
===========================================
- Hits          2905     2888      -17     
- Misses         252      269      +17     
Impacted Files Coverage Δ
src/lib.rs 100.00% <ø> (ø)
src/proofs/chunk.rs 96.33% <ø> (ø)
src/proofs/query/mod.rs 98.35% <ø> (ø)
src/proofs/tree.rs 94.80% <ø> (ø)
src/tree/link.rs 83.41% <ø> (-6.46%) :arrow_down:
src/tree/mod.rs 91.18% <ø> (-1.33%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c6c9307...92acf33. Read the comment docs.

cwlittle commented 3 years ago

Do you think this would feel cleaner by maybe moving the proof generation stuff (e.g. the things gated as full inside src/proofs/query/mod.rs) either into 1 file inside e.g. src/proofs/create.rs then feature-gating the create module in src/proofs/mod.rs?

I think anything that we can do to localize the flags would help make it cleaner. Ideally we would be able to handle all of the gating in src/lib.rs. I think that would be the cleanest solution, but we shouldn't build around this specific feature unless we plan to refactor for feature flags for new features in the future. The majority of the flags in src/proofs/query/mod.rs were to suppress dead_code warnings, so pulling those into the binary are superfluous for the light client in any case.

And also maybe feature-gating the chunk module in src/proofs/query/mod.rs? (Then inside both of those files we don't need to feature-gate anything since we're basically just gating the entire files).

I'm not seeing any reference to chunk in src/proofs/query/mod.rs other than Decoder and Op, but we need both of those for verify. Am I missing an indirect reference to chunk in src/proofs/query/mod.rs? If you mean src/proofs/mod.rs, adding a feature flag on that module will add additional dead_code warnings in src/proofs/query/mod.rs that we'll need to flag to suppress (I tried it out). I think the issue here is that we have to use "pub mod proofs" in src/lib.rs in order to be able to access merk::proofs::query::verify in the first place. Maybe if we break out query to src/query we can conditionally import the things that we need and follow those imports down the line, which will hopefully localize the flags to the import blocks if we're lucky. However, I don't see this as a likely solution given that the "pub mod proofs" in src/lib.rs seemed to pull in all of the functions from proofs/query even though the use statement in src/lib.rs is only explicitly asking for verify when not attached to the full feature.

mappum commented 3 years ago

I'm not seeing any reference to chunk in src/proofs/query/mod.rs other than Decoder and Op, but we need both of those for verify. Am I missing an indirect reference to chunk in src/proofs/query/mod.rs?

Oh my mistake, I did mean src/proofs/mod.rs.

Maybe if we break out query to src/query we can conditionally import the things that we need and follow those imports down the line, which will hopefully localize the flags to the import blocks if we're lucky.

I think I'm against that because it's a slightly less intuitive organization of the code - the query stuff really are just a kind of proofs so it could be confusing for it to exist outside of proofs.

I think this is good how it is then since you've tried things out and it's really not that many flags. In the future we can organize code while keeping the features in mind to make things as clean as possible.