Fix Restart application issue #5579

AdityaSripal · 2020-01-28T01:27:42Z

Description

Fixes the restart issue by making sure that CommitID contains the last hash that was persisted to disk, rather than the hash of the last block that was executed

Improves Pruning flexibility by splitting the old KeepEvery parameter into => KeepEvery and SnapshotEvery parameters.

The KeepEvery parameter defines how often the state gets flushed to disk. The SnapshotEvery parameter defines the snapshot blocks that will remain in disk permanently.
On each flush, the SDK removes the last flushed state unless it is a snapshot version.

closes: #5570

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

… disk

store/rootmulti/store.go

store/types/pruning.go

store/iavl/store.go

store/rootmulti/store.go

store/iavl/store.go

AdityaSripal · 2020-01-29T18:22:50Z

On further thinking, I don't think the sim tests are necessary here. Don't believe they test anything that baseapp doesn't.

So ready for review!!

AdityaSripal · 2020-01-29T18:48:10Z

Also needs CHANGELOG, but i can add that in once people agree on what needs to be added

New SnapshotEvery parameter, and difference between old KeepEvery parameter
Version method on CommitStore
CommitID stores latest persisted hash/version

anything else?

store/iavl/store.go

store/rootmulti/store.go

store/types/pruning.go

Co-Authored-By: Alexander Bezobchuk <[email protected]>

ethanfrey · 2020-01-31T09:50:43Z

Hmm so i tried implementing the approach Ethan recommended. Run into the issue that the Query function works by grabbing commitInfo of the version from disk. If the latest version's commitInfo is not flushed to disk, we cannot query latest information, and can only query from flushed versions:

Very interesting, so Query is reading from disk directly, not just via iavl. There was another outstanding issue about queries and db locks... maybe this is related?
tendermint/tendermint#3962 and the original issue from sdk

alexanderbez · 2020-01-31T14:50:44Z

Hmm so i tried implementing the approach Ethan recommended. Run into the issue that the Query function works by grabbing commitInfo of the version from disk. If the latest version's commitInfo is not flushed to disk, we cannot query latest information, and can only query from flushed versions:

https://github.com/cosmos/cosmos-sdk/blob/master/store/rootmulti/store.go#L444

If we were able to query (by height), why can we also not get the commit info by that same height?

AdityaSripal · 2020-02-05T01:54:18Z

Ready for review again. Manual tests performed:

Ran gaia with syncable option, sent a tx, queried successfully, restarted successfully, queried correct state again successfully
Ran local-testnet with different pruning options for each container. (1 everything, 1 nothing, 2 syncable) and ran for 150 blocks. Note: no txs

@alexanderbez @ethanfrey

alexanderbez

Changes look good. There are a few small outstanding items to be addressed.

store/iavl/store_test.go

store/rootmulti/store.go

alexanderbez · 2020-02-05T15:26:01Z

store/iavl/store.go

@@ -94,18 +138,41 @@ func (st *Store) Commit() types.CommitID {
 		panic(err)
 	}

-	return types.CommitID{
+	st.version = version
+	flushed := st.pruning.FlushVersion(version)


@AdityaSripal do you have a response to this? Or is this even valid?

store/rootmulti/store_test.go

store/types/pruning.go

…os-sdk into aditya/pruning-fixes

alexanderbez

ACK

AdityaSripal

ACK, Great work with documentation @alexanderbez! thanks for helping

codecov · 2020-02-06T20:25:58Z

Codecov Report

Merging #5579 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5579      +/-   ##
==========================================
- Coverage   44.53%   44.51%   -0.03%     
==========================================
  Files         324      324              
  Lines       24672    24700      +28     
==========================================
+ Hits        10988    10995       +7     
- Misses      12627    12645      +18     
- Partials     1057     1060       +3

ethanfrey · 2020-02-07T16:28:45Z

I just saw this was merged to master. Which has some major breaking changes from v0.38.0. I was expecting this to go into releases/v0.38.0 for a v0.38.1 release. Such a v0.38.1 bugfix is blocking at least 3 chains.

I will do a review on the diff now

ethanfrey

Looks good and seems quite correct to me.

Added a number of relatively minor comments that you may want to address in a smaller follow-up PR.

ethanfrey · 2020-02-07T16:32:16Z

CHANGELOG.md

+determines which committed heights are flushed to disk and `SnapshotEvery` determines which of these
+heights are kept after pruning. The `IsValid` method should be called whenever using these options. Methods
+`SnapshotVersion` and `FlushVersion` accept a version arugment and determine if the version should be
+flushed to disk or kept as a snapshot. Note, `KeepRecent` is automatically inferred from the options


+1 for automatically inferring, and less confusion in configs

ethanfrey · 2020-02-07T16:34:29Z

store/iavl/store.go

+	// If KeepEvery = 1, keepRecent should be 0 since there is no need to keep
+	// latest version in a in-memory cache.
+	//
+	// If KeepEvery > 1, keepRecent should be 1 so that state changes in between


Huh, that's what 1 does?
I would have though it would have to equal KeepEvery or something.
But I guess it doesn't if we only want "query last block"

ethanfrey · 2020-02-07T16:35:52Z

store/iavl/store.go

+
+		// Previous flushed version should only be pruned if the previous version is
+		// not a snapshot version OR if snapshotting is disabled (SnapshotEvery == 0).
+		if previous != 0 && !st.pruning.SnapshotVersion(previous) {


Maybe previous > 0 is "safer".
Though I cannot think of a scenario when this could be negative, still better safe

ethanfrey · 2020-02-07T16:38:05Z

store/iavl/store.go

+		// not a snapshot version OR if snapshotting is disabled (SnapshotEvery == 0).
+		if previous != 0 && !st.pruning.SnapshotVersion(previous) {
+			err := st.tree.DeleteVersion(previous)
+			if errCause := errors.Cause(err); errCause != nil && errCause != iavl.ErrVersionDoesNotExist {


How about

if err != nil && !iavl.ErrVersionDoesNotExist.Is(err)

https://github.com/cosmos/cosmos-sdk/blob/master/types/errors/errors.go#L175

ethanfrey · 2020-02-07T16:39:57Z

store/rootmulti/store.go

-	setLatestVersion(batch, version)
-	batch.Write()
+	// write CommitInfo to disk only if this version was flushed to disk
+	if rs.pruningOpts.FlushVersion(version) {


Much nicer. We update in memory and response every time, and just use this check to keep CommitInfo in sync with the underlying iavl substores

ethanfrey · 2020-02-07T16:41:57Z

store/rootmulti/store.go

+	// Otherwise, we query for the commit info from disk.
+	var commitInfo commitInfo
+
+	if res.Height == rs.lastCommitInfo.Version {


Okay, so we can always query the last one (guaranteed to be in memory cache via auto-set KeepRecent) and an other one that was flushed to disk. Seems good.

User providing height = 0 leading to "current height" is taken care of elsewhere (in baseapp), right?

User providing height = 0 leading to "current height" is taken care of elsewhere (in baseapp), right?

Yes!

ethanfrey · 2020-02-07T16:44:03Z

store/types/pruning.go

+	// PruneEverything defines a pruning strategy where all committed states will
+	// be deleted, persisting only the current state.
+	PruneEverything = PruningOptions{
+		KeepEvery:     1,


Interesting with this option, you will have significantly more IO than PruneSyncable, yet much less disk usage.

What about this just keeping one flushed to disk (as now), but only flushing every 10 blocks or so? (just increment KeepEvery). This would be helpful for nodes that need throughput

I would prefer to keep sound and most "general" strategies as we have now, except allow the CLI/start command to accept manual values for these instead of providing a bunch of other strategies. ie. gaiad start --pruning-keep-every ... --pruning-snapshot-every ...

Actually probably best to set these in app.toml instead of (or in additon to) the cli.

But I agree with your point, leaving these 3 safe named posibilities, and then allowing the user to take the risk and shoot themself in the foot if they want to do advanced configuration

fix commitID along with adding new SnapshotEvery parameter to cleanup…

a540b0e

… disk

AdityaSripal requested review from alessio, alexanderbez, fedekunze, jackzampolin and rigelrozanski as code owners January 28, 2020 01:27

AdityaSripal self-assigned this Jan 28, 2020

AdityaSripal added C:Store T:Bug labels Jan 28, 2020

AdityaSripal added 3 commits January 27, 2020 17:30

fix commitID along with adding new SnapshotEvery parameter to cleanup…

253ad5c

… disk

remove file from unrelated debugging worl

f1e5ba9

fix cleanup bug

923f1e5

jackzampolin reviewed Jan 28, 2020

View reviewed changes

store/rootmulti/store.go Outdated Show resolved Hide resolved

golangcibot reviewed Jan 28, 2020

View reviewed changes

store/rootmulti/store.go Outdated Show resolved Hide resolved

alexanderbez added the WIP label Jan 28, 2020

fedekunze reviewed Jan 28, 2020

View reviewed changes

store/types/pruning.go Outdated Show resolved Hide resolved

store/types/pruning.go Show resolved Hide resolved

store/iavl/store.go Show resolved Hide resolved

store/iavl/store.go Outdated Show resolved Hide resolved

alessio reviewed Jan 28, 2020

View reviewed changes

store/rootmulti/store.go Show resolved Hide resolved

fedekunze reviewed Jan 28, 2020

View reviewed changes

store/iavl/store.go Outdated Show resolved Hide resolved

AdityaSripal added 4 commits January 28, 2020 12:18

add iavl tests

a35591b

add rootmulti tests

0ae90d8

add baseapp tests along with Version method on CommitStore

d612e56

cleanup from review

63bd983

AdityaSripal changed the title ~~WIP: Fix Restart application issue~~ Fix Restart application issue Jan 29, 2020

AdityaSripal added R4R and removed WIP labels Jan 29, 2020

Merge branch 'master' into aditya/pruning-fixes

9e4b434

alexanderbez suggested changes Jan 29, 2020

View reviewed changes

Update store/types/pruning.go

fbd1a52

Co-Authored-By: Alexander Bezobchuk <[email protected]>

AdityaSripal added 3 commits January 31, 2020 11:41

start addressing review comments

c9c0fd1

further cleanup

f041162

enforce keepRecent is either 0 or 1

d52da2c

Merge branch 'master' into aditya/pruning-fixes

f52fa5e

alexanderbez suggested changes Feb 5, 2020

View reviewed changes

AdityaSripal and others added 6 commits February 5, 2020 16:24

address comments and remove keepRecent from user-facing pruning options

5fab6cd

Merge branch 'aditya/pruning-fixes' of https://github.com/cosmos/cosm…

c811093

…os-sdk into aditya/pruning-fixes

address last comments on review

4fa7fe7

Linting and cleanup

e8676ab

More linting

d8186a6

More linting

b210d0e

alexanderbez approved these changes Feb 6, 2020

View reviewed changes

alexanderbez and others added 2 commits February 6, 2020 13:05

Edit changelog

5b00d1a

Merge branch 'master' into aditya/pruning-fixes

5102a90

AdityaSripal commented Feb 6, 2020

View reviewed changes

alexanderbez mentioned this pull request Feb 6, 2020

0.38.1 Point Release #5574

Closed

8 tasks

Merge branch 'master' into aditya/pruning-fixes

6e67a1b

jackzampolin approved these changes Feb 6, 2020

View reviewed changes

alexanderbez merged commit dba80ca into master Feb 6, 2020

alexanderbez deleted the aditya/pruning-fixes branch February 6, 2020 20:58

ethanfrey reviewed Feb 7, 2020

View reviewed changes

ethanfrey pushed a commit to CosmWasm/cosmos-sdk that referenced this pull request Feb 8, 2020

Merge PR cosmos#5579: Fix Restart application issue

55867bc

alexanderbez pushed a commit that referenced this pull request Feb 10, 2020

Merge PR #5579: Fix Restart application issue

4115525

assafmo mentioned this pull request Feb 17, 2020

Upgrade cosmos-sdk v0.38.0 -> v0.38.1 scrtlabs/SecretNetwork#9

Closed

anilcse mentioned this pull request Mar 4, 2020

State load issue when applying upgrades #5746

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Restart application issue #5579

Fix Restart application issue #5579

AdityaSripal commented Jan 28, 2020 •

edited by alexanderbez

Loading

AdityaSripal commented Jan 29, 2020

AdityaSripal commented Jan 29, 2020

ethanfrey commented Jan 31, 2020

alexanderbez commented Jan 31, 2020

AdityaSripal commented Feb 5, 2020 •

edited

Loading

alexanderbez left a comment

alexanderbez Feb 5, 2020

alexanderbez left a comment

AdityaSripal left a comment

codecov bot commented Feb 6, 2020 •

edited

Loading

ethanfrey commented Feb 7, 2020 •

edited

Loading

ethanfrey left a comment

ethanfrey Feb 7, 2020

ethanfrey Feb 7, 2020

ethanfrey Feb 7, 2020

ethanfrey Feb 7, 2020

alexanderbez Feb 7, 2020

ethanfrey Feb 7, 2020

ethanfrey Feb 7, 2020

alexanderbez Feb 7, 2020

ethanfrey Feb 7, 2020

alexanderbez Feb 10, 2020

ethanfrey Feb 10, 2020

Fix Restart application issue #5579

Fix Restart application issue #5579

Conversation

AdityaSripal commented Jan 28, 2020 • edited by alexanderbez Loading

Description

AdityaSripal commented Jan 29, 2020

AdityaSripal commented Jan 29, 2020

ethanfrey commented Jan 31, 2020

alexanderbez commented Jan 31, 2020

AdityaSripal commented Feb 5, 2020 • edited Loading

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 6, 2020 • edited Loading

Codecov Report

ethanfrey commented Feb 7, 2020 • edited Loading

ethanfrey 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal commented Jan 28, 2020 •

edited by alexanderbez

Loading

AdityaSripal commented Feb 5, 2020 •

edited

Loading

codecov bot commented Feb 6, 2020 •

edited

Loading

ethanfrey commented Feb 7, 2020 •

edited

Loading