feat: ADR-040: Add `RootStore` implementation #10430

roysc · 2021-10-25T09:18:30Z

Description

Part of: #10192

Introduces a new RootStore type in the store/v2 package and an implementation, without yet replacing the MultiStore or refactoring its use within the SDK (which will happen in the follow up: #10174).
Specified by ADR-040.

Fixes #10651
Fixes #10263

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

robert-zaremba

part-1 review

db/memdb/db.go

db/meta/backend.go

internal/db/legacy_adapter.go

robert-zaremba

Part 2 review

let's rename flat package to root

store/v2/dbadapter/store.go

robert-zaremba · 2021-11-10T15:46:30Z

store/v2/flat/root_store.go

+)
+
+// RootStoreConfig is used to define a schema and pass options to the RootStore constructor.
+type RootStoreConfig struct {


I think we don't need to store the config at all. The idea of RootStore was to remove the functionality of Multistore (dynamically adding / removing sub stores) and only provide a minimum functionality to keep the compatibility with existing modules (notably IBC).
In other words, RootStore:

is configured by app

doesn't need to load a dynamic configuration from its persistent state.

Let's review this approach.

cc: @alexanderbez , @aaronc
ref:

docs: adr-40: reduce multistore and make it atomic #9355 (comment)

docs: adr-40: reduce multistore and make it atomic #9355 (comment)

store/v2/flat/store.go

store/v2/flat/store_view.go

lgtm-com · 2021-11-11T12:39:16Z

This pull request introduces 1 alert when merging ad9ab87 into 8fc9f76 - view on LGTM.com

new alerts:

1 for Expression has no effect

store/v2/flat/root_store.go

roysc · 2021-11-17T17:16:54Z

Okay, this is now reworked to store an independent SMT for each store (and combine each tree into a "namespace" Merkle map to produce a root hash, like the Multistore). This will allow us to implement proofs in an IBC-compatible way. The actual proofs are not implemented, but that can be done on top of #10015. Since that PR has yet to be reviewed, I propose we focus on getting this merged and then rebase that branch and add RootStore proofs there.

cc @robert-zaremba @aaronc @marbar3778

codecov · 2021-11-18T03:39:57Z

Codecov Report

Merging #10430 (198c1a5) into master (3495691) will decrease coverage by 0.70%.
The diff coverage is 57.46%.

❗ Current head 198c1a5 differs from pull request most recent head 644cef8. Consider uploading reports for the commit 644cef8 to get more accurate results

@@            Coverage Diff             @@
##           master   #10430      +/-   ##
==========================================
- Coverage   65.42%   64.72%   -0.71%     
==========================================
  Files         634      624      -10     
  Lines       60702    60354     -348     
==========================================
- Hits        39716    39065     -651     
- Misses      18686    19056     +370     
+ Partials     2300     2233      -67

Impacted Files	Coverage Δ
client/tx/factory.go	`24.64% <0.00%> (-0.73%)`	⬇️
db/memdb/db.go	`72.72% <ø> (ø)`
db/rocksdb/batch.go	`67.56% <ø> (ø)`
db/rocksdb/db.go	`72.25% <ø> (ø)`
db/rocksdb/iterator.go	`83.33% <ø> (ø)`
store/types/store.go	`64.70% <0.00%> (ø)`
store/v2/multi/cache_store.go	`0.00% <0.00%> (ø)`
types/module/configurator.go	`15.55% <0.00%> (-0.73%)`	⬇️
x/auth/client/testutil/suite.go	`97.33% <ø> (ø)`
x/authz/client/testutil/tx.go	`98.22% <ø> (ø)`
... and 126 more

i-norden

Nothing insightful to add, just a few nitpicks. Didn't find any functional issues.

db/prefix/prefix.go

store/v2/dbadapter/store.go

store/v2/dbadapter/store_test.go

robert-zaremba

Let's update the documentation by explaining when and how the substores are created
There is lot of code duplication with v1. Would it be possible to import it rather than copying? Or maybe moving to it's own package, but it's a breaking change which we would like to avoid...

db/prefix/prefix.go

docs/core/store.md

robert-zaremba · 2021-12-01T01:16:25Z

store/v2/types.go

+const StoreTypeTransient = v1.StoreTypeTransient
+const StoreTypeDB = v1.StoreTypeDB
+const StoreTypeSMT = v1.StoreTypeSMT
+const StoreTypePersistent = v1.StoreTypePersistent


I think StoreTypeSMT is not needed in v1 package and we can move it here.

Unfortunately if we do that we can't include it in the (StoreType) String() function, because it's in a separate package.

store/v2/root/store.go

GetVersion rename to databucket, indexbucket

- GetVersion - migration (StoreUpgrades) - flat.Store query uses getversion

robert-zaremba

utACK.

Let's clarify https://github.com/cosmos/cosmos-sdk/pull/10430/files#r754657414 before merging.

i-norden

Excellent work! I left some rather nit-picky comments, I can't find anything substantial left to change.

docs/core/store.md

i-norden · 2021-12-14T19:43:57Z

store/v2/root/store.go

+	if tlm.TracingEnabled() {
+		store = tracekv.NewStore(store, tlm.TraceWriter, tlm.TraceContext)
+	}
+	if wls, has := tlm.listeners[skey.Name()]; has && len(wls) != 0 {


Use ListeningEnabled method here?

i-norden · 2021-12-14T19:53:42Z

store/v2/root/store.go

+	if ls, has := tlm.listeners[key]; has {
+		tlm.listeners[key] = append(ls, listeners...)
+	} else {
+		tlm.listeners[key] = listeners


We can append to tlm.listeners[key] whether or not that key already exists in the map (as long as the entire map isn't nil, which it isn't)

robert-zaremba · 2021-12-15T14:43:24Z

Tiny things to update:

rename root package to multi
re-review documentation for updates. (see Ian review, maybe there are other things to update as well).

Co-authored-by: Ian Norden <iansnorden@gmail.com>

roysc mentioned this pull request Oct 25, 2021

feat!: ADR-040: Refactor App to use v2.MultiStore #10174

Closed

19 tasks

roysc force-pushed the roysc/adr-040-create-rootstore branch from 91eb724 to 6b983e5 Compare October 27, 2021 14:00

roysc marked this pull request as ready for review October 27, 2021 14:42

roysc requested review from aaronc and alexanderbez as code owners October 27, 2021 14:42

uwezukwechibuzor approved these changes Oct 28, 2021

View reviewed changes

robert-zaremba mentioned this pull request Nov 3, 2021

ADR-040 Implementation #9816

Closed

38 tasks

robert-zaremba reviewed Nov 10, 2021

View reviewed changes

db/memdb/db.go Outdated Show resolved Hide resolved

db/meta/backend.go Outdated Show resolved Hide resolved

db/meta/backend.go Outdated Show resolved Hide resolved

db/meta/backend.go Outdated Show resolved Hide resolved

internal/db/legacy_adapter.go Outdated Show resolved Hide resolved

robert-zaremba reviewed Nov 10, 2021

View reviewed changes

roysc force-pushed the roysc/adr-040-create-rootstore branch from 436fdaa to ad9ab87 Compare November 11, 2021 12:14

robert-zaremba reviewed Nov 11, 2021

View reviewed changes

store/v2/flat/root_store.go Outdated Show resolved Hide resolved

roysc force-pushed the roysc/adr-040-create-rootstore branch from ad9ab87 to b90fadf Compare November 17, 2021 17:09

roysc changed the title ~~feat: ADR-040: Add RootStore implementation~~ feat: ADR-040: Add RootStore implementation Nov 17, 2021

roysc force-pushed the roysc/adr-040-create-rootstore branch from 5df15dc to 325e448 Compare November 23, 2021 04:52

i-norden reviewed Nov 24, 2021

View reviewed changes

db/prefix/prefix.go Show resolved Hide resolved

store/v2/dbadapter/store.go Outdated Show resolved Hide resolved

store/v2/dbadapter/store_test.go Outdated Show resolved Hide resolved

github-actions bot added C:Store Type: Build labels Nov 24, 2021

robert-zaremba reviewed Dec 1, 2021

View reviewed changes

roysc force-pushed the roysc/adr-040-create-rootstore branch 3 times, most recently from dbb2dfa to 04b51a5 Compare December 1, 2021 17:16

roysc added 6 commits December 2, 2021 14:13

add store/v2 types, RootStore

0b732a4

store/v2 dbadapter

2ffea14

store/v2 mem & transient stores

66aec81

db iterator adapter

4d1735d

flat store additions

5f8d494

GetVersion rename to databucket, indexbucket

add RootStore implementation

4afd095

- GetVersion - migration (StoreUpgrades) - flat.Store query uses getversion

set rocksdb_build tag in CI submodule tests

f4edb06

roysc force-pushed the roysc/adr-040-create-rootstore branch from 857d09a to eab5875 Compare December 3, 2021 13:30

github-actions bot removed the T: CI label Dec 3, 2021

roysc force-pushed the roysc/adr-040-create-rootstore branch from eab5875 to c04cd39 Compare December 3, 2021 13:33

Update gorocksdb replace directive to use cosmos fork

ad227e0

roysc force-pushed the roysc/adr-040-create-rootstore branch from c04cd39 to ad227e0 Compare December 3, 2021 13:33

robert-zaremba approved these changes Dec 5, 2021

View reviewed changes

Merge branch 'master' into roysc/adr-040-create-rootstore

15b9b5c

roysc mentioned this pull request Dec 7, 2021

Fork RocksDB into cosmos organization and use it in store/v2 #10263

Closed

6 tasks

godoc

61bddc6

roysc mentioned this pull request Dec 8, 2021

State Sync for V2 Store (ADR 40) #10705

Closed

19 tasks

roysc added 3 commits December 9, 2021 15:01

clean up, nits

ac8abb4

PR revisions

438d384

use t.Run more, some cleanup

02a6fb1

alexanderbez approved these changes Dec 13, 2021

View reviewed changes

i-norden approved these changes Dec 14, 2021

View reviewed changes

robert-zaremba added A:automerge Automatically merge PR once all prerequisites pass. and removed A:automerge Automatically merge PR once all prerequisites pass. labels Dec 15, 2021

roysc and others added 4 commits December 16, 2021 00:04

Apply suggestions from code review

db7501e

Co-authored-by: Ian Norden <iansnorden@gmail.com>

revision

746fb29

rename store/v2/{root=>multi}

038f0eb

rename ReservePrefix => RegisterSubstore

312924f

roysc added the A:automerge Automatically merge PR once all prerequisites pass. label Dec 16, 2021

Merge remote-tracking branch 'cosmos/master' into create-rootstore

644cef8

roysc force-pushed the roysc/adr-040-create-rootstore branch from 198c1a5 to 644cef8 Compare December 16, 2021 12:58

mergify bot merged commit 109bc94 into cosmos:master Dec 16, 2021

roysc mentioned this pull request Dec 17, 2021

ADR-040 Implementation: Incorporate v2.MultiStore into BaseApp #10804

Closed

4 tasks

i-norden mentioned this pull request Jan 5, 2022

ADR-040 Implementation: Review and refactor MultiStore #10192

Closed

7 tasks

roysc deleted the roysc/adr-040-create-rootstore branch February 15, 2022 07:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ADR-040: Add `RootStore` implementation #10430

feat: ADR-040: Add `RootStore` implementation #10430

roysc commented Oct 25, 2021 •

edited

Loading

robert-zaremba left a comment

robert-zaremba left a comment •

edited

Loading

robert-zaremba Nov 10, 2021

robert-zaremba Nov 10, 2021

lgtm-com bot commented Nov 11, 2021

roysc commented Nov 17, 2021

codecov bot commented Nov 18, 2021 •

edited

Loading

i-norden left a comment •

edited

Loading

robert-zaremba left a comment

robert-zaremba Dec 1, 2021

roysc Dec 1, 2021

robert-zaremba left a comment

i-norden left a comment

i-norden Dec 14, 2021

i-norden Dec 14, 2021

robert-zaremba commented Dec 15, 2021

feat: ADR-040: Add RootStore implementation #10430

feat: ADR-040: Add RootStore implementation #10430

Conversation

roysc commented Oct 25, 2021 • edited Loading

Description

Author Checklist

Reviewers Checklist

robert-zaremba left a comment

Choose a reason for hiding this comment

robert-zaremba left a comment • edited Loading

Choose a reason for hiding this comment

robert-zaremba Nov 10, 2021

Choose a reason for hiding this comment

robert-zaremba Nov 10, 2021

Choose a reason for hiding this comment

lgtm-com bot commented Nov 11, 2021

roysc commented Nov 17, 2021

codecov bot commented Nov 18, 2021 • edited Loading

Codecov Report

i-norden left a comment • edited Loading

Choose a reason for hiding this comment

robert-zaremba left a comment

Choose a reason for hiding this comment

robert-zaremba Dec 1, 2021

Choose a reason for hiding this comment

roysc Dec 1, 2021

Choose a reason for hiding this comment

robert-zaremba left a comment

Choose a reason for hiding this comment

i-norden left a comment

Choose a reason for hiding this comment

i-norden Dec 14, 2021

Choose a reason for hiding this comment

i-norden Dec 14, 2021

Choose a reason for hiding this comment

robert-zaremba commented Dec 15, 2021

feat: ADR-040: Add `RootStore` implementation #10430

feat: ADR-040: Add `RootStore` implementation #10430

roysc commented Oct 25, 2021 •

edited

Loading

robert-zaremba left a comment •

edited

Loading

codecov bot commented Nov 18, 2021 •

edited

Loading

i-norden left a comment •

edited

Loading