feat: ADR 040: New DB interface #9573

roysc · 2021-06-23T17:25:05Z

New DB interface to replace `tm-db`

This introduces a new interface to wrap the backend KV store DB while supporting versioning and ACID transactions.
Part of ADR-040 changes.

This has been revised to consist of only the interface types. All implementations and utilities will be in follow-up PRs.

Partially resolves: vulcanize#2

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)

tac0turtle

could this be rebased onto master please. Many changes arent relevant

roysc · 2021-08-13T16:22:04Z

I've added a DeleteVersions method to the connection - this will of course be needed for pruning.

tychoish · 2021-08-16T17:36:38Z

db/types.go

+// if err := itr.Error(); err != nil {
+//   ...
+// }
+type Iterator interface {


I've written/used iterators with the pattern:

var iter Iterator for iter.Next() { } if err := iter.Err(); err != nil { return err } if err := iter.Close(); err != nil { return err }

Which collapses Valid/Next,

Seems fine, the only odd thing about this is it requires the iterator to start in an invalid state

👍 - had similar opinion about it above.

Thinking a bit more on this, it may not be ideal as it violates the single-responsibility principle. The interface is smaller but you can't check the iterator's state without mutating it.

That said, I have pushed the change to the interface.

tychoish · 2021-08-16T17:38:26Z

db/types.go

+	// Empty keys are not valid.
+	// CONTRACT: No writes may happen within a domain while an iterator exists over it.
+	// CONTRACT: start, end readonly []byte
+	ReverseIterator(start, end []byte) (Iterator, error)


my inclination is that you'd specify iteration direction as an option to the method that produes the iterator and not require duplicate methods.

Yeah, that's a trivial change. Any other opinions on this?

I don't have a strong preference, but tend to have the opposite inclination 🤷‍♂️

keeping the interface minimal is a desired design. However it shouldn't on a cost of overloading the functions.

I also don't have a strong opinion here, but we use different methods in the SDK currently. Being as we're only adding one parameter, I don't see why not.

roysc · 2021-08-17T13:13:32Z

How does this look @aaronc and @alexanderbez? I think the design changes @robert-zaremba and @tychoish brought up seem fine, but I don't have a strong opinion either way.

aaronc

Pre-approving - seems like most of the remaining changes are minor, so if you can work those out with @robert-zaremba we can move forward

codecov · 2021-08-18T11:30:15Z

Codecov Report

Merging #9573 (5533111) into master (b92308e) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 5533111 differs from pull request most recent head d0f8927. Consider uploading reports for the commit d0f8927 to get more accurate results

@@           Coverage Diff           @@
##           master    #9573   +/-   ##
=======================================
  Coverage   63.80%   63.80%           
=======================================
  Files         566      566           
  Lines       53362    53362           
=======================================
+ Hits        34046    34047    +1     
+ Misses      17397    17396    -1     
  Partials     1919     1919

Impacted Files	Coverage Δ
crypto/keys/internal/ecdsa/privkey.go	`84.21% <0.00%> (+1.75%)`	⬆️

db/types.go

alexanderbez · 2021-08-18T13:22:18Z

db/types.go

+	// Empty keys are not valid.
+	// CONTRACT: No writes may happen within a domain while an iterator exists over it.
+	// CONTRACT: start, end readonly []byte
+	ReverseIterator(start, end []byte) (Iterator, error)


I also don't have a strong opinion here, but we use different methods in the SDK currently. Being as we're only adding one parameter, I don't see why not.

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

robert-zaremba

thanks for merging Next and Valid.

tac0turtle · 2021-08-19T13:53:32Z

@roysc once you update to master the bot should be able to merge the PR automatically

orijbot · 2021-08-19T17:43:18Z

Visit https://dashboard.github.orijtech.com?pr=9573&remote=false&repo=cosmos%2Fcosmos-sdk to see benchmark details.

## Description Implements an in-memory backend for the DB interface introduced by #9573 and specified by [ADR-040](https://github.com/cosmos/cosmos-sdk/blob/eb7d939f86c6cd7b4218492364cdda3f649f06b5/docs/architecture/adr-040-storage-and-smt-state-commitments.md). This expands on the [btree](https://pkg.go.dev/github.com/google/btree)-based [`MemDB`](https://github.com/tendermint/tm-db/tree/master/memdb) from `tm-db` by using copy-on-write clones to implement versioning. Resolves: vulcanize#2 Will move out of draft once #9573 is merged and rebased on. ### 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... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) n/a - [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [x] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [x] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)

## Description Partially resolves: vulcanize#14 Implements a [BadgerDB](https://pkg.go.dev/github.com/dgraph-io/badger/v3)-based backend for the DB interface introduced by #9573 and specified by [ADR-040](https://github.com/cosmos/cosmos-sdk/blob/eb7d939f86c6cd7b4218492364cdda3f649f06b5/docs/architecture/adr-040-storage-and-smt-state-commitments.md). This uses Badger's "managed" mode for version management, and supports optimistically concurrent transactions (required one [patch](dgraph-io/badger#1716) upstream to fix handling of write conflicts). --- ### 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... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - n/a - [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [x] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [x] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)

## Description Partially resolves: vulcanize#14 Implements a [RocksDB](https://github.com/facebook/rocksdb)-based backend for the DB interface introduced by #9573 and specified by [ADR-040](https://github.com/cosmos/cosmos-sdk/blob/eb7d939f86c6cd7b4218492364cdda3f649f06b5/docs/architecture/adr-040-storage-and-smt-state-commitments.md). * Historical versioning is implemented with [Checkpoints](https://github.com/facebook/rocksdb/wiki/Checkpoints). * Uses `OptimisticTransactionDB` to allow concurrent transactions with write conflict detection. This depends on some additional CGo bindings - see tecbot/gorocksdb#216, facebook/rocksdb#8526. We'll need to replace the `gorocksdb` module until these are upstream. --- ### 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... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - n/a - [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [x] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [x] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)

tac0turtle reviewed Jun 23, 2021

View reviewed changes

roysc force-pushed the roysc/adr-040-db branch from 924f265 to 692241d Compare June 24, 2021 05:09

github-actions bot removed C:Simulations C:x/slashing Type: Build C:Rosetta Issues and PR related to Rosetta C:x/auth C:x/feegrant C:Keys Keybase, KMS and HSMs C:x/distribution distribution module related C:x/genutil genutil module issues labels Jun 24, 2021

roysc added 2 commits August 14, 2021 00:21

DBConnection.DeleteVersion

3d9dc49

Revise DB interface

80f9e6a

roysc force-pushed the roysc/adr-040-db branch from e663fb5 to 80f9e6a Compare August 13, 2021 16:21

db/go.mod nit

8b5d7cb

tychoish reviewed Aug 16, 2021

View reviewed changes

roysc mentioned this pull request Aug 17, 2021

feat: ADR 040: Implement in-memory DB backend #9952

Merged

19 tasks

aaronc approved these changes Aug 17, 2021

View reviewed changes

Collapse iterator methods Next/Valid to Next() bool

69e5a9a

roysc force-pushed the roysc/adr-040-db branch from 96640ef to 69e5a9a Compare August 18, 2021 11:29

alexanderbez reviewed Aug 18, 2021

View reviewed changes

db/types.go Outdated Show resolved Hide resolved

alexanderbez approved these changes Aug 18, 2021

View reviewed changes

roysc and others added 2 commits August 18, 2021 22:37

Update db/types.go

fd9b9aa

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Add todo notes

dbdb1c9

robert-zaremba approved these changes Aug 18, 2021

View reviewed changes

aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 18, 2021

roysc force-pushed the roysc/adr-040-db branch from eef18d9 to dbdb1c9 Compare August 19, 2021 12:50

go mod tidy

5533111

Merge branch 'master' into roysc/adr-040-db

d0f8927

mergify bot merged commit e3aec18 into cosmos:master Aug 19, 2021

roysc deleted the roysc/adr-040-db branch August 20, 2021 18:20

i-norden mentioned this pull request Sep 7, 2021

ADR-040 Implementation: New DB interface #10094

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ADR 040: New DB interface #9573

feat: ADR 040: New DB interface #9573

roysc commented Jun 23, 2021 •

edited

Loading

tac0turtle left a comment

roysc commented Aug 13, 2021

tychoish Aug 16, 2021

roysc Aug 17, 2021

robert-zaremba Aug 17, 2021

roysc Aug 18, 2021

tychoish Aug 16, 2021

roysc Aug 17, 2021

aaronc Aug 17, 2021

robert-zaremba Aug 17, 2021

alexanderbez Aug 18, 2021

roysc commented Aug 17, 2021

aaronc left a comment

codecov bot commented Aug 18, 2021 •

edited

Loading

alexanderbez Aug 18, 2021

robert-zaremba left a comment

tac0turtle commented Aug 19, 2021 •

edited

Loading

orijbot commented Aug 19, 2021

feat: ADR 040: New DB interface #9573

feat: ADR 040: New DB interface #9573

Conversation

roysc commented Jun 23, 2021 • edited Loading

New DB interface to replace tm-db

Author Checklist

Reviewers Checklist

tac0turtle left a comment

Choose a reason for hiding this comment

roysc commented Aug 13, 2021

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 Aug 17, 2021

aaronc left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 18, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

robert-zaremba left a comment

Choose a reason for hiding this comment

tac0turtle commented Aug 19, 2021 • edited Loading

orijbot commented Aug 19, 2021

roysc commented Jun 23, 2021 •

edited

Loading

New DB interface to replace `tm-db`

codecov bot commented Aug 18, 2021 •

edited

Loading

tac0turtle commented Aug 19, 2021 •

edited

Loading