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

go/consensus/tendermint: Use MKVS for storing application state #2674

Merged
merged 1 commit into from
Apr 10, 2020
Merged

go/consensus/tendermint: Use MKVS for storing application state #2674

merged 1 commit into from
Apr 10, 2020

Conversation

kostko
Copy link
Member

@kostko kostko commented Feb 14, 2020 •
edited

Based on #2659
Based on #2691
Based on #2715.
Based on #2775.
Fixes #1898
Fixes #2710
Fixes #2811

Also makes some things nicer.

TODO

  • Support state rollback.
  • Tests for state rollback.
  • Do some stress tests.
  • Make set consensus parameters operations safer.

@kostko kostko force-pushed the kostko/feature/storage-checkpoints-v2 branch 2 times, most recently from 6f5cb00 to 46f06ef Compare February 14, 2020 13:41
@kostko kostko force-pushed the kostko/feature/consensus-mkvs branch from 12e21e6 to 259a448 Compare February 14, 2020 14:01
@kostko kostko changed the base branch from kostko/feature/storage-checkpoints-v2 to master February 14, 2020 14:01
@kostko kostko force-pushed the kostko/feature/consensus-mkvs branch from 259a448 to feca3bc Compare February 14, 2020 14:11
@codecov
Copy link

codecov bot commented Feb 14, 2020 •
edited

Codecov Report

Merging #2674 into master will decrease coverage by 1.06%.
The diff coverage is 60.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2674      +/-   ##
==========================================
- Coverage   67.15%   66.09%   -1.07%     
==========================================
  Files         343      345       +2     
  Lines       32987    33269     +282     
==========================================
- Hits        22154    21989     -165     
- Misses       8143     8407     +264     
- Partials     2690     2873     +183
Impacted Files Coverage Δ
go/consensus/api/api.go 50.00% <ø> (ø)
go/consensus/tendermint/api/gas.go 92.00% <ø> (ø)
...nsensus/tendermint/apps/keymanager/transactions.go 0.00% <0.00%> (ø)
go/storage/mkvs/db/api/api.go 60.46% <ø> (ø)
go/storage/mkvs/db/badger/badger.go 68.81% <0.00%> (+2.22%) ⬆️
go/consensus/tendermint/apps/beacon/genesis.go 35.71% <25.00%> (-10.44%) ⬇️
go/upgrade/migrations/dummy.go 78.57% <28.57%> (-13.43%) ⬇️
go/consensus/tendermint/apps/staking/fees.go 38.39% <29.03%> (-11.11%) ⬇️
go/consensus/tendermint/apps/scheduler/genesis.go 41.11% <33.33%> (-4.87%) ⬇️
...nsensus/tendermint/apps/staking/signing_rewards.go 25.80% <33.33%> (-9.91%) ⬇️
... and 88 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87ddca8...151312a. Read the comment docs.

@kostko kostko force-pushed the kostko/feature/consensus-mkvs branch 7 times, most recently from dc00db2 to 9e83027 Compare February 18, 2020 14:08
@kostko kostko mentioned this pull request Feb 18, 2020
Merged
@kostko kostko force-pushed the kostko/feature/consensus-mkvs branch 3 times, most recently from e60560a to fa2cf66 Compare February 18, 2020 20:43
@kostko kostko changed the base branch from master to kostko/feature/mkvs-improvements February 18, 2020 20:44
@kostko kostko force-pushed the kostko/feature/mkvs-improvements branch from 812a510 to 91efcae Compare February 19, 2020 14:41
@kostko kostko changed the base branch from kostko/feature/mkvs-improvements to master February 20, 2020 13:25
@kostko kostko force-pushed the kostko/feature/consensus-mkvs branch 2 times, most recently from 22af83b to d330233 Compare February 20, 2020 13:54
@kostko kostko requested a review from pro-wh February 21, 2020 14:22
@kostko kostko force-pushed the kostko/feature/consensus-mkvs branch from d330233 to 42e32a2 Compare February 24, 2020 13:22
@kostko kostko mentioned this pull request Feb 24, 2020
Closed
Yawning
Yawning reviewed Feb 24, 2020
View reviewed changes
Copy link
Contributor

@Yawning Yawning left a comment

Choose a reason for hiding this comment

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

How setConesensusParameters isn't still a massive foot + gun waiting to come back to cause problems escapes me. This looks good, most of the changes look fairly boilerplate.

go/consensus/tendermint/apps/staking/state/state.go Outdated Show resolved Hide resolved
@kostko
Copy link
Member Author

kostko commented Feb 24, 2020

How setConesensusParameters isn't still a massive foot + gun waiting to come back to cause problems escapes me.

Yeah that's just because I rebased on top of #2715 earlier so it has that exact change :-) I'll refactor that state to be more similar to other mux apps and changed our consensus parameter mutation operations to enforce that they can only be used in the proper context (e.g., in InitChain/BeginBlock/EndBlock).

pro-wh
pro-wh reviewed Feb 25, 2020
View reviewed changes
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

I'm like 8/77 files in

go/consensus/tendermint/abci/context_test.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/context_test.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/state.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/state.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/mux.go Show resolved Hide resolved
go/consensus/tendermint/abci/prune.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/prune.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/errors.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/prune_test.go Show resolved Hide resolved
@kostko kostko force-pushed the kostko/feature/consensus-mkvs branch from 42e32a2 to ffaeccd Compare February 25, 2020 08:40
@kostko
Copy link
Member Author

kostko commented Feb 25, 2020 •
edited

The new way of updating consensus parameters is now in. This required a minor refactor, so some stuff has moved from consensus/tendermint/abci to consensus/tendermint/api.

@kostko kostko force-pushed the kostko/feature/consensus-mkvs branch 5 times, most recently from 2520189 to 2582564 Compare March 25, 2020 10:30
@kostko kostko mentioned this pull request Mar 25, 2020
Merged
@kostko kostko force-pushed the kostko/feature/consensus-mkvs branch 5 times, most recently from 0248ed8 to 5416eb3 Compare March 26, 2020 16:01
pro-wh
pro-wh approved these changes Mar 26, 2020
View reviewed changes
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

alright I think everything's resolved

@kostko kostko force-pushed the kostko/feature/consensus-mkvs branch 6 times, most recently from f9ebf42 to 7f27af0 Compare April 1, 2020 16:15
@kostko kostko mentioned this pull request Apr 2, 2020
Closed
@kostko kostko force-pushed the kostko/feature/consensus-mkvs branch 3 times, most recently from e9711a5 to 79a34ad Compare April 7, 2020 11:29
@kostko kostko mentioned this pull request Apr 10, 2020
Closed
@kostko kostko force-pushed the kostko/feature/consensus-mkvs branch 3 times, most recently from 9ebd4b0 to c366660 Compare April 10, 2020 10:20
@kostko kostko force-pushed the kostko/feature/consensus-mkvs branch from c366660 to 151312a Compare April 10, 2020 11:15
@kostko kostko merged commit 3f59f4a into master Apr 10, 2020
@kostko kostko deleted the kostko/feature/consensus-mkvs branch April 10, 2020 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yawning Yawning Yawning approved these changes

@pro-wh pro-wh pro-wh approved these changes

@peterjgilbert peterjgilbert Awaiting requested review from peterjgilbert peterjgilbert is a code owner

Assignees
No one assigned
Labels
c:breaking/consensus Category: breaking consensus changes
Projects
None yet
Milestone
No milestone
3 participants
@kostko @pro-wh @Yawning