Use lazy-loaded cached tree implementation #73
Conversation
misc. cleanup
test empty root expand orphan tests
test empty root improve old tests partial caching w/ stub nodes cache node digest
and clean up, change method sigs, nits
Publicize names
- keep path/node/value hashing distinct - Allow arbitrary leaf value data
rename save -> commit count common path prefix from current depth
Codecov Report
@@ Coverage Diff @@
## master #73 +/- ##
==========================================
+ Coverage 85.62% 86.46% +0.83%
==========================================
Files 6 8 +2
Lines 466 650 +184
==========================================
+ Hits 399 562 +163
- Misses 39 53 +14
- Partials 28 35 +7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
adlerjohn
requested review from
musalbas,
adlerjohn,
tzdybal and
Manav-Aggarwal
|
Thanks @roysc! I've pulled the changes from the vulcanize repo a few weeks into the "lazy" branch here (https://github.com/celestiaorg/smt/tree/lazy), as we need to implement the things in #72 first before merging to master. Is our lazy branch already up to date or does this PR introduce anything new not in the lazy branch (if so can this PR be against the lazy branch rather than master)? For context though, we've currently decided to put the SMT work on hold too, as we're currently implementing deep trees for IAVL instead (rollkit/iavl#1). This is because due to the assumptions made by store V1 including range queries and versioning etc, making store V1 work well with SMT is more work than expected so we've decided to pursue IAVL for now. |
Sorry, I didn't see that branch - this branch is just somewhat cleaned up, I removed a caching MapStore which isn't quite relevant, renamed some minor things and added some comments. This is a rebase, so targeting the original head might be a mess, but I will check. |
|
@roysc does this PR remove value storage? I recall in the original lazy implementation in vulcanize, value storage was removed. But it looks like you added back in this PR, if I understand correctly (from a cursory look)? Would the code in the README still work? |
There was a problem hiding this comment.
Did not do a deep dive into this PR but just left a few flyby NIT comments.
@roysc Do you plan on pushing this through? I could do a deep review or just submit a follow-up PR after this is merged if I see any areas of improvement.
does this PR remove value storage?
Given the lack of value mapStore I believe the answer is yes, but again, I did not dive in.
Would the code in the README still work?
It does node. The value MapStore no longer needs to be passed in, the method signature change to NewSMT, and the options aren't captured.
As someone who will use this after the PR is merged, I could add clarifying examples on the options available and such.
rename BaseSMT => TreeSpec
{New,Import}{SMT => SparseMerkleTree}
|
Sorry for the delay on this, it was sidelined by prioritization and time off. I'd like to wrap it up if possible, and to that end I've pushed an additional cleanup/refactor with a README update.
Yes it does, but we can easily add a type that wraps value storage with the tree. (Alternatively, the raw value can be stored directly in the leaf by using an identity function as the value hasher.) What's the likelihood of this being reviewed in the near future? If no movement is likely for now (or you just plan to use your own branch), I will probably just close this PR. |
|
Thanks for this @roysc. Given we're not currently using SMT at Celestia, there's no one really currently maintaining this repo. My suggestion is that given that your PR breaks the current interface, that you merge this into vulcanize/smt, and then perhaps I can add to the README of this repo that this repo isn't currently being maintained, and point them to vulcanize/smt as a more recently maintained version. Is vulcanize currently using and maintaining vulcanize/smt? Alternatively, I think it's fine to just leave this PR open in case we start maintaining this repo again, and for future users to see. |
|
Thanks @musalbas. I think the SMT is basically shelved for vulcanize for now as well, but could possibly be experimented with in the future, so pointing to that fork makes sense. I'll close this PR though, just for tidiness purposes. |
|
@Olshansk That works, we will maintain our fork at Vulcanize and I can take up your PR once it's ready. |
|
We intend to deprecate/archive this repo as we don't have bandwidth to review PRs and maintain this repo. So @Olshansk if you plan to actively use and maintain SMT, I suggest maintaining your own fork for now. |
👍 I've created https://github.com/pokt-network/smt. Will revisit potential collaboration opportunities when our fork is more mature & battle-tested. |
This introduces a largely rewritten implementation of the SMT which operates on a cached, lazily-loaded tree structure, as opposed to the current paradigm of reading from/writing to the DB directly for each operation. This gives a significant performance improvement on all operations while preserving full compatibility of the produced hashes.
This also refactors the hasher objects to allow tree paths and stored values to be hashed independently, and
Options to configure them. #40 can be satisfied by passing an identity function as the path hasher.As noted here, the
DeepSubtreecode supporting state-transition fraud proofs was not compatible as-is with this implementation and so is removed; but there should be no technical block to adapting that code to this impl.; it was simply outside the scope of Cosmos-SDK ADR-40 for which this was originally built.