Implement decoupled state commitments and storage #11

roysc · 2021-05-04T12:46:20Z

Introduces a new CommitKVStore to implement ADR-040 proposal of decoupled commitment/data concerns.

Adapted from store/ll-smt branch (see cosmos#8507).

Description

Introduces the decoupled.Store type composed of:

A tm-db interface for mapping state storage (SS) (as key → value)
A "state commitment" (SC) store for computing Merkle root hashes (storing hash(key) → hash(key, value))
- This is an iavl.Store held as a CommitKVStore reference, which will later be refactored to use an SMT store
A tm-db interface for an inverted index, mapping SC values back to SS keys (hash(key, value) → key)

closes: #1

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards. N/A, no modules
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

i-norden

Looking good. In addition to the comments, as you know we need to replace tm-db with tm-dbv2 and remove IAVL usage. In addition to that the next step is to flesh out how the decoupling will be handled at the multistore level. I think what we want to do is have the multistore map StoreKeys to a Store tuple (SCKVStore, SSKVStore).

i-norden · 2021-05-06T12:55:45Z

store/decoupled/store.go

+	if err != nil {
+		panic(err.Error())
+	}
+	s.sc.Set(key, kvHash[:]) // TODO: key or hash(key)?


Need an err := here. But actually I don't think we need this Set, the key, value and hash(key, value), key will be sufficient

This is where the state commitment mapping is set; Set() has no return value

Got it, we don't need the err check below then.

i-norden · 2021-05-06T12:56:58Z

store/decoupled/store.go

+	// Direct KV mapping (SS)
+	data dbm.DB
+	// Inverted index of SC values to SS keys
+	inv dbm.DB


Does the second bucket need to move through a separate tm-db interface?

I'm using PrefixDBs to separate the two into different namespaces. Collisions are unlikely since the inverted index uses a hash as key, but I think it's still desired.

i-norden · 2021-05-11T12:30:52Z

store/decoupled/store.go

+	if err != nil {
+		panic(err.Error())
+	}
+	s.sc.Set(key, kvHash[:]) // TODO: key or hash(key)?


Got it, we don't need the err check below then.

i-norden · 2021-05-11T12:47:29Z

store/decoupled/store.go

+	if err != nil {
+		panic(err.Error())
+	}
+	err = s.inv.Set(kvHash[:], key)


We're deviating pretty hard from the simple behavior expected from a KVStore. A KVStore Set is expected to Set that key-value, it isn't expected to generate and Set two other kv pairs. I think this logic makes more sense at a higher level.

Per standup discussion, let's run this by Cosmos to see if this is the intended semantics, or where it's more appropriate to put this logic.

i-norden · 2021-05-11T12:49:34Z

store/decoupled/store.go

+}
+
+func (s *Store) Iterator(start, end []byte) types.Iterator {
+	iter, err := s.data.Iterator(start, end)


I think methods such as this one also highlight the issue with wrapping 3 distinct KVStores with a single KVStore- when we ask for an Iterator on this wrapping KVStore we are only getting the iterator for one of the underlying KVStores.

i-norden · 2021-05-12T16:22:37Z

store/decoupled/store.go

+	if err != nil {
+		panic(err.Error())
+	}
+	kvHash := sha256.Sum256(append(key, value...))


Instead of looking up the value and hashing it and the key to reconstruct hash(key, value) we could lookup the hash(key, value) from the SC before deleting it. This appears preferable since both approaches require a DB lookup but this approach doesn't require a hashing step. What do you think?

That does indeed make more sense

* 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".

Outline of new store following ADR-040 pattern of decoupled commitment/data concerns Largely adapted from store/ll-smt branch

roysc · 2021-05-28T11:20:19Z

This should be a MVP state store for #1.
Requires changes from the corresponding branch on tm-db: https://github.com/roysc/tm-db/tree/implement-adr-40 - (which just patches in minimal versioning support)

- Move tm-db code in from external repo - Refactor store/ and types/ packages - leaves out IAVL and multistore code pending confirmed deprecation - some hacks to glue existing tmdb usage and get compile/tests working

- Update to new db interface - Use smt as SC structure - Refactor smt to BasicKVStore

roysc · 2021-06-09T16:34:01Z

Moving this work to a PR on the main repo: cosmos#9485

i-norden reviewed May 6, 2021

View reviewed changes

roysc requested a review from ashwinphatak May 6, 2021 13:30

roysc changed the title ~~[WIP] skeleton of decoupled.Store~~ [WIP] Implement decoupled state commitments and storage May 6, 2021

i-norden reviewed May 11, 2021

View reviewed changes

i-norden reviewed May 12, 2021

View reviewed changes

tzdybal and others added 13 commits May 14, 2021 19:36

Merge branch 'master' into store/ll-smt

6698424

[WIP] skeleton of decoupled.Store

53a0d40

Outline of new store following ADR-040 pattern of decoupled commitment/data concerns Largely adapted from store/ll-smt branch

[WIP] decoupled store; basic tests

08c86e7

[wip] rf decoupled store into multistore

8b64bde

fix test

3b2d360

test case for bucket independence

e7095d5

test basic merkle hash

98d0377

no need to re-hash when deleting from index

33528d6

fixup

1811541

replace tm-db mod for interface additions, refactor

80cb898

decoupled store query, versioning

7d7b8b9

go mod tidy

d58c8e7

roysc requested a review from i-norden May 28, 2021 11:20

roysc changed the base branch from master to adr-40 June 1, 2021 13:39

roysc added 3 commits June 9, 2021 18:08

Refactor for new DB interface

03aed4d

- Move tm-db code in from external repo - Refactor store/ and types/ packages - leaves out IAVL and multistore code pending confirmed deprecation - some hacks to glue existing tmdb usage and get compile/tests working

Update modules

91ca92c

[wip] decoupled.Store

878a1eb

- Update to new db interface - Use smt as SC structure - Refactor smt to BasicKVStore

roysc closed this Jun 9, 2021

roysc reopened this Jun 9, 2021

github-actions bot added the Type: Build label Jun 9, 2021

roysc closed this Jun 11, 2021

roysc reopened this Jun 11, 2021

roysc merged commit 4667744 into vulcanize:adr-40 Jun 11, 2021

roysc deleted the implement-adr-040 branch June 11, 2021 18:39

i-norden mentioned this pull request Jun 15, 2021

Epic 1: Hook state store into SDK #1

Closed

i-norden changed the title ~~[WIP] Implement decoupled state commitments and storage~~ Implement decoupled state commitments and storage Aug 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement decoupled state commitments and storage #11

Implement decoupled state commitments and storage #11

roysc commented May 4, 2021 •

edited

Loading

i-norden left a comment

i-norden May 6, 2021

roysc May 6, 2021

i-norden May 11, 2021

i-norden May 6, 2021

roysc May 6, 2021

i-norden May 11, 2021

i-norden May 11, 2021

roysc May 12, 2021

i-norden May 11, 2021

i-norden May 12, 2021

roysc May 12, 2021

roysc commented May 28, 2021

roysc commented Jun 9, 2021

Implement decoupled state commitments and storage #11

Implement decoupled state commitments and storage #11

Conversation

roysc commented May 4, 2021 • edited Loading

Description

i-norden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roysc commented May 28, 2021

roysc commented Jun 9, 2021

roysc commented May 4, 2021 •

edited

Loading