Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SMT store type #8507

Merged
merged 15 commits into from
May 14, 2021

Conversation

tzdybal
Copy link
Contributor

@tzdybal tzdybal commented Feb 3, 2021

This PR introduces SMT store. It doesn't support Queryable interface yet, and it's not used anywhere.

This work is related to #8430 and #8297.

SMT (Sparse Merkle Tree) is intended to replace IAVL. New type
implements same interfaces as iavl.Store.
Sparse Merkle Tree does not support iteration over keys in order.
To provide drop-in replacement for IAVL, Iterator and ReverseIterator
has to be implemented.
SMT Store implementation use the underlying KV store to:
 - maintain a list of keys (under a prefix)
 - iterate over a keys
Values are stored only in SMT.
@tzdybal tzdybal marked this pull request as ready for review February 4, 2021 09:40
@robert-zaremba robert-zaremba self-assigned this Feb 5, 2021
@tac0turtle
Copy link
Member

@robert-zaremba is it okay to merge this as is for now?

@robert-zaremba
Copy link
Collaborator

@robert-zaremba is it okay to merge this as is for now?

Yes, all changes must go to ll-smt branch. Let's review the changeset.

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left few comments.


var (
indexPrefix = []byte("smt-ordering-idx-")
afterIndex = []byte("smt-ordering-idx.") // '.' is next after '-' in ASCII
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be more beneficial to have the prefix keys shorter?

Copy link
Contributor Author

@tzdybal tzdybal Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect this to be compressed away by DB.
Single-byte, numerical value would be much cleaner.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, let's use single byte values.

}

func plainKey(key []byte) []byte {
return key[len(indexPrefix):]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's store cache len(indexPrefix) in a module scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If prefix is single-byte, we can even hardcode this ;)

assert.Equal(t, p.key, iter.Key())
assert.Equal(t, p.val, iter.Value())
iter.Next()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that LL SMT is hashing the keys before inserting, so how possible it preserves a pre-image order?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we're testing cosmos-sdk SMT store, not LL SMT data structure. I'm using ordering from key->value mapping (https://github.com/cosmos/cosmos-sdk/pull/8507/files/6fd860d514daa4c9caf55df383d81b1a1e671ede#diff-0d018f820578ed2dd08d3bd1cb6f854aead4e2b32333b15105743eecb21bba77R41).

for _, p := range pairs {
require.True(t, iter.Valid())
assert.Equal(t, p.key, iter.Key())
assert.Equal(t, p.val, iter.Value())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should use require here. It will make lot of error messages if something is wrong and it will be messy. Assert doesn't stop the exectuion.
Tip: I'm usually declaring at the top of the function:

require := require.New(t)  // works with assert as well

Then in the function I don't need to pass t any more: require.Equal(a, b).

func (s *Store) Get(key []byte) []byte {
defer telemetry.MeasureSince(time.Now(), "store", "smt", "get")
s.mtx.RLock()
defer s.mtx.RUnlock()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to lock? Isn't the underlying DB solving this?

defer telemetry.MeasureSince(time.Now(), "store", "smt", "get")
s.mtx.RLock()
defer s.mtx.RUnlock()
val, err := s.tree.Get(key)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we query data through a tree, rather than directly from the DB?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably leftover from version with only SMT tree (no plain-kv).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DB is used only for iteration in this version. It was implemented before finalization of ADR-040 and deciding that values goes to plain-KV.

if err != nil {
panic(err.Error())
}
s.mtx.Unlock()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should use defer s.mtx.Unlock() right after the Lock.
Unfortunately SDK is using a bad pattern to panic to quickly wind up the stack, rather than passing errors. So, we don't want to leave the store locked in case of panic.

@robert-zaremba
Copy link
Collaborator

shall we update the PR and merge into the branch?

@tac0turtle
Copy link
Member

shall we update the PR and merge into the branch?

lets merge and follow up

@aaronc
Copy link
Member

aaronc commented Apr 9, 2021

Not sure we want to add more new features into this release.

@tac0turtle
Copy link
Member

Not sure we want to add more new features into this release.

This would be merged into a feature branch store/ll-smt. It will not be merged into master until at a later date.

@robert-zaremba
Copy link
Collaborator

@tzdybal , there are few small changes to do in this PR. Will you like to finish it and merge?

@tzdybal
Copy link
Contributor Author

tzdybal commented Apr 13, 2021

Yes, I'll fix this today.

@robert-zaremba
Copy link
Collaborator

Hi @tzdybal - if you need a help let me know - I can clean up this PR for you and merge it.

@tac0turtle
Copy link
Member

Hi @tzdybal - if you need a help let me know - I can clean up this PR for you and merge it.

can you take over it please. The LL team is busy at the moment, dont want to add more to their plate.

@tzdybal
Copy link
Contributor Author

tzdybal commented Apr 23, 2021

Sorry for late reply and fix. I addressed all review comments.

SMT stores:
 * key -> hash(key, value)

KV store stores:
 * key->value in "bucket 1",
 * hash(key, value) -> key in "bucket 2".
@tzdybal
Copy link
Contributor Author

tzdybal commented Apr 23, 2021

OK, so I changed the implementation to use ADR-040 data layout for KV and SMT.

Please re-review @robert-zaremba, @marbar3778.

@robert-zaremba
Copy link
Collaborator

@tzdybal - thanks for handling this, let me merge it so we can move forward.

@robert-zaremba robert-zaremba merged commit 5ef2364 into cosmos:store/ll-smt May 14, 2021
roysc pushed a commit to vulcanize/cosmos-sdk that referenced this pull request Jun 23, 2021
* Initial SMT store type

SMT (Sparse Merkle Tree) is intended to replace IAVL. New type
implements same interfaces as iavl.Store.

* Add iteration support to SMT

Sparse Merkle Tree does not support iteration over keys in order.
To provide drop-in replacement for IAVL, Iterator and ReverseIterator
has to be implemented.
SMT Store implementation use the underlying KV store to:
 - maintain a list of keys (under a prefix)
 - iterate over a keys
Values are stored only in SMT.

* Migrate to smt v0.1.1

* Extra test for SMT iterator

* CommitStore implementation for SMT store

* Use interface instead of concrete type

* Add telemetry to SMT store

* SMT: version->root mapping, cleanup

* SMT proofs - initial code

* Tests for SMT store ProofOp implementation

* Fix linter errors

* Use simple 1 byte KV-store prefixes

* Improve assertions in tests

* Use mutex properly

* Store data in ADR-040-compatible way

SMT stores:
 * key -> hash(key, value)

KV store stores:
 * key->value in "bucket 1",
 * hash(key, value) -> key in "bucket 2".
roysc pushed a commit to vulcanize/cosmos-sdk that referenced this pull request Oct 12, 2021
* Initial SMT store type

SMT (Sparse Merkle Tree) is intended to replace IAVL. New type
implements same interfaces as iavl.Store.

* Add iteration support to SMT

Sparse Merkle Tree does not support iteration over keys in order.
To provide drop-in replacement for IAVL, Iterator and ReverseIterator
has to be implemented.
SMT Store implementation use the underlying KV store to:
 - maintain a list of keys (under a prefix)
 - iterate over a keys
Values are stored only in SMT.

* Migrate to smt v0.1.1

* Extra test for SMT iterator

* CommitStore implementation for SMT store

* Use interface instead of concrete type

* Add telemetry to SMT store

* SMT: version->root mapping, cleanup

* SMT proofs - initial code

* Tests for SMT store ProofOp implementation

* Fix linter errors

* Use simple 1 byte KV-store prefixes

* Improve assertions in tests

* Use mutex properly

* Store data in ADR-040-compatible way

SMT stores:
 * key -> hash(key, value)

KV store stores:
 * key->value in "bucket 1",
 * hash(key, value) -> key in "bucket 2".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants