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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 39 commits into from
Oct 19, 2021
Merged

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

merged 39 commits into from
Oct 19, 2021

Conversation

roysc
Copy link
Contributor

@roysc roysc commented Aug 10, 2021 •
edited
Loading

Description

Resolves: #10117

Implements a CommitKVStore which separates the concerns of state storage and state commitment according to ADR-040.

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

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules - n/a
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • 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 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)

@i-norden i-norden mentioned this pull request Aug 10, 2021
Closed
@roysc roysc mentioned this pull request Aug 12, 2021
Merged
9 tasks
@roysc roysc force-pushed the roysc/adr-040-store branch 2 times, most recently from 14daa15 to 2c6c90d Compare August 26, 2021 17:18
This was referenced Sep 8, 2021
Closed
Closed
@roysc roysc force-pushed the roysc/adr-040-store branch 5 times, most recently from 00bc622 to 7ed1be9 Compare September 22, 2021 13:34
@roysc
Copy link
Contributor Author

roysc commented Sep 22, 2021

I'm not sure what to make of all these build failures - CI wants to update go.mod. Could it be related to the replace ... => ./db directive?

@roysc roysc marked this pull request as ready for review September 22, 2021 14:16
alexanderbez
alexanderbez reviewed Sep 23, 2021
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Could we perhaps review this PR together in a call?

store/types/store.go
return "StoreTypeSMT"

case StoreTypeDecoupled:
return "StoreTypeDecoupled"
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this type?

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 represents the new decoupled.Store type

odeke-em
odeke-em requested changes Sep 28, 2021
View reviewed changes
Copy link
Collaborator

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

Thank you for this change @roysc! I've added some comments from the first pass, please take a look.

db/rocksdb/db.go Outdated Show resolved Hide resolved
db/rocksdb/db.go Outdated Show resolved Hide resolved
db/rocksdb/db.go Outdated Show resolved Hide resolved
db/rocksdb/iterator.go Outdated Show resolved Hide resolved
db/rocksdb/iterator.go Outdated Show resolved Hide resolved
db/rocksdb/iterator.go Outdated Show resolved Hide resolved
store/v2/decoupled/store.go Outdated Show resolved Hide resolved
store/v2/decoupled/store.go Outdated Show resolved Hide resolved
@roysc
Copy link
Contributor Author

roysc commented Sep 28, 2021

Thanks for the review @odeke-em - I'll add the RocksDB revisions to that PR

@roysc roysc force-pushed the roysc/adr-040-store branch 4 times, most recently from c733df3 to 514c119 Compare September 28, 2021 09:25
roysc
roysc commented Sep 29, 2021
View reviewed changes
store/v2/decoupled/store.go Outdated Show resolved Hide resolved
roysc
roysc commented Sep 29, 2021
View reviewed changes
store/v2/decoupled/store.go Outdated Show resolved Hide resolved
roysc
roysc commented Sep 29, 2021
View reviewed changes
store/v2/types.go Outdated Show resolved Hide resolved
roysc
roysc commented Sep 29, 2021
View reviewed changes
store/v2/decoupled/store.go Outdated
@@ -0,0 +1,493 @@
package decoupled
Copy link
Contributor Author

@roysc roysc Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

TODO - rename this package/store ("decoupled" being somewhat a misnomer)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been drawing a blank on a good alternative name. Perhaps "flat" store, to reflect that reads don't need a tree traversal? Open to suggestions

Copy link
Collaborator

Choose a reason for hiding this comment

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

flat def sounds good to me.

This was referenced Oct 5, 2021
Merged
Merged
tac0turtle
tac0turtle approved these changes Oct 14, 2021
View reviewed changes
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

utACK

left some comments.

@roysc
Copy link
Contributor Author

roysc commented Oct 15, 2021

I've expanded the test coverage for the decoupled and smt stores and tweaked the errors some.

@tac0turtle tac0turtle added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 15, 2021
@roysc
Copy link
Contributor Author

roysc commented Oct 18, 2021

@odeke-em I've addressed the changes from your review - if you can approve, this is ready to merge

@odeke-em
Copy link
Collaborator

@odeke-em I've addressed the changes from your review - if you can approve, this is ready to merge

Thanks for the ping @roysc, I am taking a look right now.

odeke-em
odeke-em approved these changes Oct 19, 2021
View reviewed changes
Copy link
Collaborator

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

Thank you @roysc for this work! LGTM but with some suggestions, please take a look, and congrats! I have some future changes I'll send to further increase performance but I won't distract this critical change from landing for now :-) I shall also then be able to send benchmarks after this lands.

db/dbtest/util.go Outdated Show resolved Hide resolved
db/dbtest/util.go Outdated Show resolved Hide resolved
db/dbtest/util.go Outdated Show resolved Hide resolved
db/go.mod Outdated Show resolved Hide resolved
db/prefix/prefix.go Outdated Show resolved Hide resolved
store/v2/decoupled/store.go Outdated Show resolved Hide resolved
store/v2/decoupled/store.go Outdated Show resolved Hide resolved
store/v2/decoupled/store.go Outdated Show resolved Hide resolved
store/v2/decoupled/store.go Outdated Show resolved Hide resolved
store/v2/smt/proof.go Outdated Show resolved Hide resolved
@tac0turtle
Copy link
Member

@roysc could you update the branch to master, the bot will take care of the merge

@mergify mergify bot merged commit 85eed1f into cosmos:master Oct 19, 2021
@roysc roysc deleted the roysc/adr-040-store branch October 19, 2021 13:12
@odeke-em
Copy link
Collaborator

@roysc some of this code doesn't compile and failed per https://dashboard.github.orijtech.com/benchmark/c29ef717d40744458619a51d565e7471

@roysc
Copy link
Contributor Author

roysc commented Oct 19, 2021 •
edited
Loading

Oh, that's bad. I'll make a patch right now. I forgot the nested module's tests are not included in CI.

@roysc
Copy link
Contributor Author

roysc commented Oct 19, 2021

@odeke-em PR at #10403, do I need to make an issue as well?

@odeke-em
Copy link
Collaborator

odeke-em commented Oct 19, 2021 •
edited
Loading

@odeke-em PR at #10403, do I need to make an issue as well?

Thanks for the prompt fix, looks good now https://dashboard.github.orijtech.com/benchmark/6d259263f5784f348c9b5153eb553e1f. No, you don't need an issue for it.

amaury1093
amaury1093 reviewed Oct 20, 2021
View reviewed changes
go.mod
@@ -128,3 +131,5 @@ replace google.golang.org/grpc => google.golang.org/grpc v1.33.2
replace github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1

replace github.com/99designs/keyring => github.com/cosmos/keyring v1.1.7-0.20210622111912-ef00f8ac3d76

replace github.com/cosmos/cosmos-sdk/db => ./db
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a dumb question: What is the purpose of this line?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nvm, it's because db is its own module.

@robert-zaremba robert-zaremba mentioned this pull request Nov 3, 2021
Closed
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amaury1093 amaury1093 amaury1093 left review comments

@odeke-em odeke-em odeke-em approved these changes

@alexanderbez alexanderbez alexanderbez left review comments

@ValarDragon ValarDragon ValarDragon left review comments

@tac0turtle tac0turtle tac0turtle approved these changes

@robert-zaremba robert-zaremba robert-zaremba approved these changes

@aaronc aaronc Awaiting requested review from aaronc

Assignees
No one assigned
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:Store
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ADR-040 Implementation: Hook state store and SMT into the SDK
8 participants
@roysc @odeke-em @tac0turtle @robert-zaremba @amaury1093 @alexanderbez @ValarDragon @tzdybal