refactor!: app version stored in multi store and shared in state sync #265
Conversation
p0mvn
changed the title
p0mvn
changed the title
snapshots/types/format.go
Outdated
| @@ -3,4 +3,7 @@ package types | |||
| // CurrentFormat is the currently used format for snapshots. Snapshots using the same format | |||
| // must be identical across all nodes for a given height, so this must be bumped when the binary | |||
| // snapshot output changes. | |||
| const CurrentFormat uint32 = 1 | |||
| // Format 1 was the original format. | |||
| // Format 2 serailizes app version in addition to the original | |||
There was a problem hiding this comment.
Asking to learn: does format indicate the format of the snapshot itself(with format2 indicating the current updated version with the app version?
There was a problem hiding this comment.
Yes, that's correct. Every time we change how snapshots are serialized, we should increase this CurrentFormat.
This is done to ensure that nodes are capable of interpreting snapshot chunks received from other nodes. If node A supports format 1 and receives a chunk from node b that supports format 2, node A can immediately reject it. Otherwise, it wouldn't know how to deserialize it.
I changed the godoc a bit to make it clearer
I thought we agreed that we aren't maintaining changelog for our sdk fork. How about I document this change in README and update changelog in Osmosis once we update its SDK dependency? |
|
forgot that we agreed on not maintaining changelog for our sdk fork, but yes I think this change is notable enough to be documented somewhere along with the code changes! |
baseapp/abci.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| errAppVersionIsNotInitial = func(actualVersion uint64) error { |
There was a problem hiding this comment.
Is there a reason this is an aliased function and not just a normal function?
There was a problem hiding this comment.
I'm using the convention of storing errors and their messages as vars or constants respectively at the top of the file. It allows identifying these errors more easily and increases the likelihood of reusing them where needed.
The need for making this a function arises at times when we need to format parameters at runtime. That's why I create such aliased functions for these "dynamic" errors.
I'm not sure what the best practice that would allow the consolidation of all errors in one place while removing the need for such aliased functions with runtime parameters.
If you have a suggestion, please let me know
In terms of making this a regular function, I think it would make this error less visible and less likely to be reused.
There was a problem hiding this comment.
@peterbourgon 's suggestion here seems like a better alternative than this aliased function
There was a problem hiding this comment.
@peterbourgon 's suggestion here seems like a better alternative than this aliased function
What he suggested is the idiomatic approach 👍
baseapp/baseapp.go
Outdated
| app.paramStore.Set(ctx, ParamStoreKeyVersionParams, cp.Version) | ||
| // We're explicitly not storing the Tendermint app_version in the param store. It's | ||
| // stored instead in the x/upgrade store, with its own bump logic. | ||
| // We do not store version params here because they are |
There was a problem hiding this comment.
It's a bit odd to me that we treat cp.Version as a special case here.
What if we instead store it here as we did before but also store it in the multi-store directly? Or does that not seem right either?
There was a problem hiding this comment.
Yeah, I've thought about this as well. It's totally possible to store it in multiple places. However, the long-term problem that I foresee with doing that is that these will go out of sync and get misused. There should really be only one data source of such critical parameters.
It seems to me that the original problem with app version breaking state-sync is exactly for this reason - storing it in multiple places and, as a result, not updating correctly.
There was a problem hiding this comment.
Ok, I think this is fine then as long as we have clear documentation.
There was a problem hiding this comment.
So I would add to the existing comment that the entire reason we do this is to allow state-sync to function properly.
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Peter Bourgon <[email protected]>
baseapp/baseapp.go
Outdated
| app.paramStore.Set(ctx, ParamStoreKeyVersionParams, cp.Version) | ||
| // We're explicitly not storing the Tendermint app_version in the param store. It's | ||
| // stored instead in the x/upgrade store, with its own bump logic. | ||
| // We do not store version params here because they are |
There was a problem hiding this comment.
So I would add to the existing comment that the entire reason we do this is to allow state-sync to function properly.
Closes: #osmosis-labs/osmosis#1805
What is the purpose of the change
State sync is still broken in Osmosis v10. Upon manual testing, it was found that the param store is not being shared by state-sync snapshots. App version was persisted there in #241 and #244 .
When a raw node is trying to state-sync, it might be in a state where it does not have any information about the app version at all since it is trying to state-sync from an empty state. Therefore, we need to change the snapshot format to include the app version.
Upon applying the snapshot to the app, the state-synching node should have the same app version as the original node, at the time when the applied snapshot was taken. Then, Tendermint's app version validation should pass.
This change introduces the new snapshot format with the app version included. Additionally, the app version is now stored in multi-store instead of params to be able to conveniently access it for snapshot serialization/deserialization.
This change was successfully tested on mainnet with 2 full nodes where 1 one was trying to state-sync from the other while having it as persistent peer and peer exchange off.
To make it work in tests, an additional hack was made to hardcode the current app version to the new location. To release this change to public, we will have to do a major release (v11).
Brief Changelog
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? no