refactor: address snapshot/pruning refactor comments upstream #184

p0mvn · 2022-04-08T20:03:58Z

Description

Addressing functional and security comments from review upstream:
cosmos#11496

All relevant commits were backported from the PR above

Testing

Tested this on Osmosis v7.x: #206

had to backport to v7.x branches because the main is unstable
all commits in chore: backport snapshot / pruning refactor #206 are squashed into 1
pruning = everything and snapshot interval = 100
- On prune-everything the interval and keep-recent are both 10

Following the Logs

If we specify a small snapshot-interval (such as 100) and snapshots overlap, the new requests to snapshot will be skipped if there is one that is already in progress. Therefore, I split the snapshot height tests into 2 cases:

snapshot performed
snapshot skipped

Snapshot Heights - Snapshot Performed

Verified that it works as expected by exposing some debug logs:

When we would normally prune height 4080000 (at 4080010), the snapshot height does not show up anymore because it is kept until after a snapshot is done:

9:49PM INF pruning heights heights=[4080001,4080002,4080003,4080004,4080005,4080006,4080007,4080008,4080009]

note that we have only 9 heights when the interval is 10 (4080000 is a snapshot height)

Eventually we see the following log message signifying that a snapshot height was pruned away:

HandleHeightSnapshot height=4080000
10:19PM INF completed state snapshot format=1 height=4080000
10:20PM INF pruning heights heights=[4080280,4080281,4080282,4080283,4080284,4080000,4080285,4080286,4080287,4080288,4080289]

Snapshot Heights - Skipped

Snapshot heights that overlap with an already in progress snapshot are skipped and pruned almost immediately:

9:58PM INF creating state snapshot height=4080100
9:58PM INF HandleHeightSnapshot height=4080100
9:58PM ERR failed to create state snapshot err="a snapshot operation is in progress: conflict" height=4080100
9:58PM INF indexed block height=4080100 module=txindex
10:00PM INF pruning heights heights=[4080101,4080100,4080102,4080103,4080104,4080105,4080106,4080107,4080108,4080109]

note that we have 10 heights when the interval is 10, snapshot is skipped
also note that 4080100 is out of order because it was added to the list in a separate goroutine after we checked for an in progress snapshot.

Regular Heights

For heights that are not multiples of snapshot-interval are removed right away according to the pruning strategy:

9:50PM INF pruning heights heights=[4080010,4080011,4080012,4080013,4080014,4080015,4080016,4080017,4080018,4080019]

note that we have 10 heights when the interval is 10 (none of these heights are snapshot heights)

Querying

I attempted to query epochs at a specific height. As soon as the queried height went beyond keep-recent, I would get an error. Ths indicates that pruning functions correctly.

Flow of events:

osmosisd query epochs current-epoch day --height 4080135
current_epoch: "306"

Observe 4080145 reached in the block explorer (4080135 + 10)
Observe 10:03PM INF pruning heights heights=[4080130,4080131,4080132,4080133,4080134,4080135,4080136,4080137,4080138,4080139] in the logs

osmosisd query epochs current-epoch day --height 4080135
Error: rpc error: code = InvalidArgument desc = not available identifier: invalid request

pruning/export_test.go

pruning/manager.go

alexanderbez · 2022-04-09T14:50:17Z

snapshots/manager.go

@@ -314,6 +314,11 @@ func (m *Manager) shouldTakeSnapshot(height int64) bool {
 func (m *Manager) snapshot(height int64) {
 	m.logger.Info("creating state snapshot", "height", height)

+	if height < 0 {


should this be <= 0 (I asked based on the error message)?

Yes, you are right. I will get that changed

pruning/export_test.go

alexanderbez · 2022-04-11T08:38:43Z

pruning/manager.go

@@ -59,13 +62,14 @@ func (m *Manager) GetPruningHeights() []int64 {

 // ResetPruningHeights resets the heights to be pruned.
 func (m *Manager) ResetPruningHeights() {
-	m.pruneHeights = make([]int64, 0)
+	// reuse previously allocated memory.


Suggested change

// reuse previously allocated memory.

// reuse previously allocated memory

alexanderbez · 2022-04-11T08:49:03Z

pruning/manager.go

+	}
+}
+
+// TODO: convert to a generic version with Go 1.18.


This this really necessary? How would this change?

If we must do this, let's reference (and create) an issue. I'd like to avoid "naked" TODOs.

No, you are right. It's not really necessary unless we start reusing this functionality with other types. I removed the comment

alexanderbez · 2022-04-11T08:50:40Z

x/genutil/config/priv_validator_key.json

@@ -0,0 +1,11 @@
+{


err, what's this file?

ugh, I've been switching frequently between this repo and the SDK repo, likely merged by accident

will ve removed

alexanderbez · 2022-04-11T08:50:45Z

x/genutil/data/priv_validator_state.json

@@ -0,0 +1,5 @@
+{


what's this file too?

Will be removed

p0mvn · 2022-04-11T14:49:54Z

@alexanderbez thanks for the review. Will get back to your comments. I might have to update this PR with more changes from the comments in the upstream so marking this as a draft for now

p0mvn · 2022-04-14T02:57:33Z

All comments are now addressed. I backported all changes from the upstream up until: #187

I will do everything including and after it in a separate PR to limit the scope here

alexanderbez

Reviewing both of these PRs in the SDK and our fork is not tenable IMO. Can we wait till the SDK one is merged or vice versa? I left a bunch of comments in the core repo.

alexanderbez · 2022-04-14T12:18:07Z

pruning/manager.go

 	snapshotInterval     uint64
 	pruneHeights         []int64
 	pruneSnapshotHeights *list.List
 	mx                   sync.Mutex
 }

 const (
-	pruneHeightsKey         = "s/pruneheights"
-	pruneSnapshotHeightsKey = "s/pruneSnheights"
+	errNegativeHeightsFmt = "failed to get pruned heights: %d"


This is a string. It should not start with err

p0mvn · 2022-04-14T13:05:07Z

Reviewing both of these PRs in the SDK and our fork is not tenable IMO. Can we wait till the SDK one is merged or vice versa? I left a bunch of comments in the core repo.

Sounds good. I agree. Let's wait until the core is accepted

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

…nager

p0mvn · 2022-04-21T23:57:57Z

Hi @alexanderbez This is a backport of all commits in cosmos#11496
It's caught up to all the changes. All comments in this PR should also be addressed

Can we merge this?

…209)

p0mvn changed the title ~~refactor: address snapshot/pruning refactor upstream comments~~ refactor: address snapshot/pruning refactor comments upstream Apr 8, 2022

p0mvn marked this pull request as ready for review April 8, 2022 21:20

p0mvn requested a review from alexanderbez as a code owner April 8, 2022 21:20

p0mvn requested review from alexanderbez and ValarDragon and removed request for alexanderbez April 8, 2022 21:23

alexanderbez reviewed Apr 9, 2022

View reviewed changes

github-actions bot added the C:x/genutil label Apr 9, 2022

alexanderbez reviewed Apr 11, 2022

View reviewed changes

alexanderbez mentioned this pull request Apr 11, 2022

feat: Retroactive prune very old blocks from store cosmos/cosmos-sdk#11064

Closed

19 tasks

p0mvn marked this pull request as draft April 11, 2022 14:48

p0mvn force-pushed the roman/refactor-fixes branch from 270bdd1 to 5b2697d Compare April 13, 2022 17:32

github-actions bot added C:x/upgrade and removed C:x/genutil labels Apr 13, 2022

p0mvn force-pushed the roman/refactor-fixes branch from e684bda to 95e8071 Compare April 14, 2022 00:48

p0mvn marked this pull request as ready for review April 14, 2022 02:55

alexanderbez reviewed Apr 14, 2022

View reviewed changes

p0mvn marked this pull request as draft April 20, 2022 15:44

p0mvn force-pushed the roman/refactor-fixes branch 2 times, most recently from 4fcdd02 to cf03c8e Compare April 20, 2022 20:35

p0mvn and others added 7 commits April 20, 2022 17:11

address snapshot/pruning refactor upstream comments

4bc6cbf

add unit test for NewPruningOptionsFromString

2cea83c

add TestLoadPruningHeights_PruneNothing

9ee81eb

remove db from LoadPruningHeights

c0b8584

address previousHeight == 0 guard

c61154d

Update pruning/manager.go

d9fc8bb

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

fix snapshot height must be positive check

ae14bf6

p0mvn and others added 16 commits April 20, 2022 17:11

sdk import in util_test.go

8a854fe

Apply suggestions from code review in pruning/README.md

7ef157a

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

Apply more style suggestions from code review

c707b5d

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

style fix in simapp/simd/cmd/root.go

b7f7e8e

remove unused import in baseapp

4ab0fdc

update snapshots/README.md

0002cc7

Update SnapshotIntervalOff comment in snapshots/types/options.go

d8ca2fe

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

Update pruneSnapshotHeightsKey pruning/manager.go

2d42509

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

update SetSnapshotInterval comment

a929861

remove snapshot store type

096cee2

remove pruning and snapshot wrappers from store types

790e4f0

fix HandleHeight comment in pruning manager

d261d42

snapshot happens in a separate goroutine

a17cbec

fix cherry-pick conflict

d7224ad

avoid returning custom pruning options from NewPruningOptions

29aa3b0

fix options_test.go

6d2ac52

p0mvn force-pushed the roman/refactor-fixes branch from 5c12960 to 6d2ac52 Compare April 20, 2022 21:11

p0mvn marked this pull request as ready for review April 20, 2022 22:37

This was referenced Apr 20, 2022

refactor!: abstractions for snapshot and pruning; snapshot intervals eventually pruned; unit tests cosmos/cosmos-sdk#11496

Merged

fix: data race related to last commit info; flush pruning heights as soon as changed #154

Closed

p0mvn added 2 commits April 21, 2022 17:50

set snapshot interval on multistore in baseapp instead of snapshot ma…

5b73cf4

…nager

fix snapshot manager unit tests

7e1219c

alexanderbez merged commit cf91c82 into osmosis-main Apr 25, 2022

alexanderbez deleted the roman/refactor-fixes branch April 25, 2022 15:18

p0mvn added a commit that referenced this pull request Apr 26, 2022

refactor: address snapshot/pruning refactor comments upstream (#184)

c766173

p0mvn mentioned this pull request Apr 26, 2022

chore: backport snapshot / pruning refactor (#184) #209

Merged

p0mvn added a commit that referenced this pull request Apr 26, 2022

refactor: address snapshot/pruning refactor comments upstream (#184) (#…

f1a3f95

…209)

czarcas7ic mentioned this pull request Apr 27, 2022

chore: upgrade sdk to v0.45.0x-osmo-v7.8.1 osmosis-labs/osmosis#1357

Merged

p0mvn mentioned this pull request May 1, 2022

Investigate test snapshot with pruning failing #179

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: address snapshot/pruning refactor comments upstream #184

refactor: address snapshot/pruning refactor comments upstream #184

p0mvn commented Apr 8, 2022 •

edited

Loading

alexanderbez Apr 9, 2022

p0mvn Apr 9, 2022

alexanderbez Apr 11, 2022

alexanderbez Apr 11, 2022

p0mvn Apr 14, 2022

alexanderbez Apr 11, 2022

p0mvn Apr 13, 2022

alexanderbez Apr 11, 2022

p0mvn Apr 14, 2022

p0mvn commented Apr 11, 2022

p0mvn commented Apr 14, 2022

alexanderbez left a comment

alexanderbez Apr 14, 2022

p0mvn commented Apr 14, 2022

p0mvn commented Apr 21, 2022

	// reuse previously allocated memory.
	// reuse previously allocated memory

refactor: address snapshot/pruning refactor comments upstream #184

refactor: address snapshot/pruning refactor comments upstream #184

Conversation

p0mvn commented Apr 8, 2022 • edited Loading

Description

Testing

Following the Logs

Snapshot Heights - Snapshot Performed

Snapshot Heights - Skipped

Regular Heights

Querying

Flow of events:

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

p0mvn commented Apr 11, 2022

p0mvn commented Apr 14, 2022

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn commented Apr 14, 2022

p0mvn commented Apr 21, 2022

p0mvn commented Apr 8, 2022 •

edited

Loading