feat: ADR-040: Implement RocksDB backend #9851

roysc · 2021-08-04T16:36:49Z

Description

Partially resolves: vulcanize#14

Implements a RocksDB-based backend for the DB interface introduced by #9573 and specified by ADR-040.

Historical versioning is implemented with Checkpoints.
Uses OptimisticTransactionDB to allow concurrent transactions with write conflict detection. This depends on some additional CGo bindings - see Add go.mod file, update for 6.16 tecbot/gorocksdb#216, Add more C bindings for OptimisticTransactionDB 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...

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)

roysc · 2021-08-04T16:37:59Z

Depends on #9573, so this will be a draft until that is merged.

roysc · 2021-08-31T10:40:06Z

Rebased and ready for review

alexanderbez

Nice work :)

db/go.mod

go.mod

db/rocksdb/db.go

db/rocksdb/iterator.go

tac0turtle · 2021-09-18T09:40:24Z

db/go.mod

 )
+
+replace github.com/tecbot/gorocksdb => github.com/roysc/gorocksdb v1.1.0 // v0.0.0-20210804143633-c0bf0b3635e5 // FIXME


would be good to add more context to the FIXME for the next person

Sorry, I had hoped there might be movement on their upstream repo before this was to be merged. That seems unlikely now.

Would it be better to have this point to a cosmos/gorocksdb fork before merging?

if you dont want to maintain it, this would be the best solution. @alexanderbez what you think?

I mean I honestly don't see us maintaining it -- they would diverge quickly I imagine. I'm actually concerned that it's not merged into the main upstream repo. Is there a reason for that?

It seems to just be neglected, there's been no activity for almost two years. The PR adds some some simple bindings. I don't really mind using my fork, just not sure what the preference is on your end.

TBH, if a db backend has been neglected for that long, I'm very very apprehensive about supporting it. We can totally not support RocksDB.

Just to be clear, this is an unofficial Go wrapper I'm talking about. The Rocks maintainers were pretty efficient about merging my PR to add the needed C bindings.

I think its fine, our default db currently, leveldb, is not maintained as well. I would prefer we create a fork in cosmos and add a downstream pr opening bot if anything changes upstream.

The Rocks maintainers were pretty efficient about merging my PR to add the needed C bindings.

Then we should go with this. I do not feel comfortable maintaining support for physical backends that are not actively maintained, even if they're just wrappers.

RocksDB doesn't provide Go bindings. The tecbot repo is the main third-party solution: https://github.com/facebook/rocksdb/blob/main/LANGUAGE-BINDINGS.md.

robert-zaremba

part 1 of the review

db/dbtest/testcases.go

robert-zaremba · 2021-09-22T13:01:43Z

db/go.mod

-)
+// FIXME: gorocksdb bindings for OptimisticTransactionDB are not merged upstream, so we use a fork
+// See https://github.com/tecbot/gorocksdb/pull/216
+replace github.com/tecbot/gorocksdb => github.com/roysc/gorocksdb v1.1.0


let's fork it into cosmos/gorocsdb. We did the same with keyring. @okwme , could you fork github.com/tecbot/gorocksdb in to cosmos?

Will be solved here: #10263

db/rocksdb/batch.go

db/rocksdb/db.go

roysc · 2021-09-23T06:33:38Z

Updated per reviews and merged master

robert-zaremba · 2021-09-29T15:39:02Z

@roysc - could you merge master in your branch? Then we will be able to merge this.

* use optimistic transactions * checkpoint-based versioning

multiple past readers squash! db txn testcases

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

codecov · 2021-10-05T15:14:18Z

Codecov Report

Merging #9851 (64d657f) into master (9094794) will decrease coverage by 0.41%.
The diff coverage is n/a.

❗ Current head 64d657f differs from pull request most recent head 97f0c90. Consider uploading reports for the commit 97f0c90 to get more accurate results

@@            Coverage Diff             @@
##           master    #9851      +/-   ##
==========================================
- Coverage   64.15%   63.73%   -0.42%     
==========================================
  Files         572      572              
  Lines       53983    53966      -17     
==========================================
- Hits        34632    34397     -235     
- Misses      17375    17612     +237     
+ Partials     1976     1957      -19

Impacted Files	Coverage Δ
x/bank/types/events.go	`0.00% <0.00%> (-75.00%)`	⬇️
x/distribution/abci.go	`0.00% <0.00%> (-68.75%)`	⬇️
x/staking/types/genesis.go	`26.08% <0.00%> (-30.44%)`	⬇️
x/evidence/abci.go	`0.00% <0.00%> (-30.00%)`	⬇️
x/staking/keeper/pool.go	`61.53% <0.00%> (-25.65%)`	⬇️
x/staking/keeper/hooks.go	`36.00% <0.00%> (-20.00%)`	⬇️
x/distribution/keeper/invariants.go	`55.65% <0.00%> (-20.00%)`	⬇️
x/mint/keeper/keeper.go	`60.00% <0.00%> (-17.78%)`	⬇️
x/auth/genesis.go	`30.43% <0.00%> (-17.40%)`	⬇️
x/staking/keeper/alias_functions.go	`16.12% <0.00%> (-16.13%)`	⬇️
... and 49 more

roysc · 2021-10-05T15:22:40Z

This has been updated with a DBConnection.Revert method and implementation, to allow the DB to roll back committed data that is part of the unsaved working version - this will be needed for the KV store to commit atomically to its decoupled buckets. Please review the top commits (2) when you get a chance @alexanderbez @robert-zaremba.

Revert implementations for Badger and Memdb can be added to #9892 or another PR.

robert-zaremba mentioned this pull request Aug 6, 2021

ADR-040 Implementation #9816

Closed

38 tasks

roysc marked this pull request as ready for review August 10, 2021 13:43

roysc requested review from aaronc and alexanderbez as code owners August 10, 2021 13:43

roysc force-pushed the roysc/adr-040-db-rocks branch 2 times, most recently from 7c37ce1 to 77f5053 Compare August 23, 2021 05:06

roysc force-pushed the roysc/adr-040-db-rocks branch from e5985c5 to 3b1df92 Compare August 31, 2021 10:39

i-norden mentioned this pull request Sep 7, 2021

ADR-040 Implementation: New DB interface #10094

Closed

8 tasks

roysc force-pushed the roysc/adr-040-db-rocks branch from 82cfe31 to 35ade40 Compare September 15, 2021 04:33

alexanderbez requested a review from robert-zaremba September 15, 2021 15:42

alexanderbez reviewed Sep 15, 2021

View reviewed changes

alexanderbez added the C:Store label Sep 15, 2021

tac0turtle reviewed Sep 18, 2021

View reviewed changes

alexanderbez approved these changes Sep 20, 2021

View reviewed changes

robert-zaremba reviewed Sep 22, 2021

View reviewed changes

roysc mentioned this pull request Sep 28, 2021

feat: ADR-040: Implement KV Store with decoupled storage and SMT #9892

Merged

19 tasks

roysc force-pushed the roysc/adr-040-db-rocks branch 2 times, most recently from fefdcb4 to 5911cfe Compare September 28, 2021 09:24

robert-zaremba mentioned this pull request Sep 29, 2021

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

Closed

6 tasks

robert-zaremba approved these changes Sep 29, 2021

View reviewed changes

robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Sep 29, 2021

roysc added 5 commits October 5, 2021 00:22

Implement RocksDB backend

b20b8d2

* use optimistic transactions * checkpoint-based versioning

go.mod updates

af810ec

add rocksdb to db/readme

6f9ea89

changelog entry

a574115

PR review

1cc0474

roysc and others added 13 commits October 5, 2021 00:22

db txn testcase

a67f0dc

multiple past readers squash! db txn testcases

checkpoint caching

639b2f2

add context for go.mod fixme

3f5c32e

Apply suggestions from code review

937ace9

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

refactor kv validation

284fdb7

rm unneeded reference

6c21684

Discard() returns error

5040ec5

invalidate closed txns + testcase

f163ad0

PR revisions

d69a13e

Discard() is idempotent - also fixes badger, memdb

f59c551

go mod tidy

0f73708

add DBConnection.Revert + testcase

25b166d

rocksdb: implement DBConnection.Revert

9cbf319

roysc force-pushed the roysc/adr-040-db-rocks branch from 64d657f to 9cbf319 Compare October 5, 2021 15:13

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

97f0c90

mergify bot merged commit 33c8314 into cosmos:master Oct 5, 2021

roysc mentioned this pull request Oct 7, 2021

db/memdb: failing tests and non-compiling code from "cannot use (*MemDB)(nil) (value of type *MemDB) as db.DBConnection value" #10319

Closed

4 tasks

robert-zaremba mentioned this pull request Oct 13, 2021

ADR-40 Implementation #10360

Closed

20 tasks

roysc deleted the roysc/adr-040-db-rocks branch January 15, 2022 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ADR-040: Implement RocksDB backend #9851

feat: ADR-040: Implement RocksDB backend #9851

roysc commented Aug 4, 2021 •

edited

Loading

roysc commented Aug 4, 2021

roysc commented Aug 31, 2021

alexanderbez left a comment

tac0turtle Sep 18, 2021

roysc Sep 18, 2021

tac0turtle Sep 21, 2021

alexanderbez Sep 21, 2021

roysc Sep 21, 2021

alexanderbez Sep 21, 2021

roysc Sep 22, 2021

tac0turtle Sep 22, 2021

alexanderbez Sep 22, 2021 •

edited

Loading

roysc Sep 22, 2021 •

edited

Loading

robert-zaremba left a comment

robert-zaremba Sep 22, 2021

robert-zaremba Sep 29, 2021

roysc commented Sep 23, 2021

robert-zaremba commented Sep 29, 2021

codecov bot commented Oct 5, 2021

roysc commented Oct 5, 2021

		)

		replace github.com/tecbot/gorocksdb => github.com/roysc/gorocksdb v1.1.0 // v0.0.0-20210804143633-c0bf0b3635e5 // FIXME

feat: ADR-040: Implement RocksDB backend #9851

feat: ADR-040: Implement RocksDB backend #9851

Conversation

roysc commented Aug 4, 2021 • edited Loading

Description

Author Checklist

Reviewers Checklist

roysc commented Aug 4, 2021

roysc commented Aug 31, 2021

alexanderbez 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

alexanderbez Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

roysc Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

robert-zaremba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roysc commented Sep 23, 2021

robert-zaremba commented Sep 29, 2021

codecov bot commented Oct 5, 2021

Codecov Report

roysc commented Oct 5, 2021

roysc commented Aug 4, 2021 •

edited

Loading

alexanderbez Sep 22, 2021 •

edited

Loading

roysc Sep 22, 2021 •

edited

Loading