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

refactor!: app version stored in multi store and shared in state sync #265

Merged
merged 28 commits into from
Jun 17, 2022
Merged

refactor!: app version stored in multi store and shared in state sync #265

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
fd75a32
refactor: app version stored in multi store and shared in state sync
p0mvn Jun 15, 2022
5e77bf7
app version is only stored in baseapp
p0mvn Jun 15, 2022
9e2b4ec
increase snapshot format version to with app version shared
p0mvn Jun 15, 2022
85d6390
remove old comment
p0mvn Jun 15, 2022
a94db01
Update store/rootmulti/store.go
p0mvn Jun 15, 2022
9803be9
Update store/rootmulti/store.go
p0mvn Jun 15, 2022
b01ce9b
Update baseapp/baseapp.go
p0mvn Jun 15, 2022
f878d0d
add TestGetAppVersion
p0mvn Jun 15, 2022
9b7a1aa
add TestSetAppVersion
p0mvn Jun 15, 2022
3fbae6e
test app version snapshot and restore in different store
p0mvn Jun 15, 2022
73b5957
set app version to 0 in testing init chain simulation
p0mvn Jun 15, 2022
950db19
test app version is zero on init chain
p0mvn Jun 15, 2022
52052eb
revert changes to abci info
p0mvn Jun 15, 2022
5ac95aa
consolidate app version manager in single interface
p0mvn Jun 15, 2022
84eb25f
Apply suggestions from code review
p0mvn Jun 15, 2022
6f2d090
remove unused error
p0mvn Jun 15, 2022
16d062d
update godoc for CurrentFormat
p0mvn Jun 16, 2022
d150e7a
Update baseapp/baseapp_test.go
p0mvn Jun 16, 2022
135367c
Update baseapp/abci.go
p0mvn Jun 16, 2022
336d5c2
revert constant
p0mvn Jun 16, 2022
5da1906
AppVersionError
p0mvn Jun 16, 2022
a2fd575
reintroduce initialAppVersion
p0mvn Jun 16, 2022
2f48af2
rename to newBaseAppWithDB
p0mvn Jun 16, 2022
9e1bb68
typo
p0mvn Jun 16, 2022
e3b16a9
add on to snapshots README
p0mvn Jun 16, 2022
d5f7551
Merge branch 'osmosis-main' into roman/app-version-main
p0mvn Jun 16, 2022
e912d23
Merge branch 'osmosis-main' into roman/app-version-main
p0mvn Jun 17, 2022
b676090
comment explaining the reason for storing version in multi store
p0mvn Jun 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions baseapp/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,16 @@ import (
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)

const (
initialAppVersion = 0
)

var (
errAppVersionIsNotInitial = func(actualVersion uint64) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason this is an aliased function and not just a normal function?

Copy link
Member Author

@p0mvn p0mvn Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@p0mvn p0mvn Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@peterbourgon 's suggestion here seems like a better alternative than this aliased function

Copy link
Collaborator

Choose a reason for hiding this comment

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

@peterbourgon 's suggestion here seems like a better alternative than this aliased function

What he suggested is the idiomatic approach 👍

return fmt.Errorf("app version is not initial, was %d, expected %d", actualVersion, initialAppVersion)
}
)
p0mvn marked this conversation as resolved.
Show resolved Hide resolved

// InitChain implements the ABCI interface. It runs the initialization logic
// directly on the CommitMultiStore.
func (app *BaseApp) InitChain(req abci.RequestInitChain) (res abci.ResponseInitChain) {
Expand All @@ -46,14 +56,20 @@ func (app *BaseApp) InitChain(req abci.RequestInitChain) (res abci.ResponseInitC
app.setDeliverState(initHeader)
app.setCheckState(initHeader)

if err := app.SetAppVersion(app.deliverState.ctx, 0); err != nil {
if err := app.SetAppVersion(initialAppVersion); err != nil {
panic(err)
}

// Store the consensus params in the BaseApp's paramstore. Note, this must be
// done after the deliver state and context have been set as it's persisted
// to state.
if req.ConsensusParams != nil {
// When init chain is called, the app version should either be absent and determined by the application
p0mvn marked this conversation as resolved.
Show resolved Hide resolved
// or set to 0. Panic if it's not.
if req.ConsensusParams.Version != nil && req.ConsensusParams.Version.AppVersion != initialAppVersion {
panic(errAppVersionIsNotInitial(req.ConsensusParams.Version.AppVersion))
}

app.StoreConsensusParams(app.deliverState.ctx, req.ConsensusParams)
}

Expand Down Expand Up @@ -112,7 +128,7 @@ func (app *BaseApp) InitChain(req abci.RequestInitChain) (res abci.ResponseInitC
func (app *BaseApp) Info(req abci.RequestInfo) abci.ResponseInfo {
lastCommitID := app.cms.LastCommitID()

appVersion, err := app.GetAppVersion(app.checkState.ctx)
appVersion, err := app.GetAppVersion()
if err != nil {
app.logger.Error("failed to get app version", err)
}
Expand Down
29 changes: 12 additions & 17 deletions baseapp/baseapp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package baseapp

import (
"errors"
"fmt"
"reflect"
"strings"
Expand Down Expand Up @@ -130,7 +129,7 @@ type BaseApp struct { // nolint: maligned
indexEvents map[string]struct{}
}

var _ upgrade.ProtocolVersionManager = (*BaseApp)(nil)
var _ upgrade.AppVersionManager = (*BaseApp)(nil)

// NewBaseApp returns a reference to an initialized BaseApp. It accepts a
// variadic number of option functions, which act on the BaseApp to set
Expand Down Expand Up @@ -310,15 +309,12 @@ func (app *BaseApp) init() error {
// needed for the export command which inits from store but never calls initchain
app.setCheckState(tmproto.Header{})

// If there is no app version set in the store, we should set it to 0.
// Panic on any other error.
// If errMsgNoProtocolVersionSet, we assume that appVersion is assigned to be 0.
appVersion, err := app.GetAppVersion(app.checkState.ctx)
if err != nil && !errors.Is(err, errMsgNoProtocolVersionSet) {
appVersion, err := app.GetAppVersion()
if err != nil {
return err
}

if err := app.SetAppVersion(app.checkState.ctx, appVersion); err != nil {
if err := app.SetAppVersion(appVersion); err != nil {
return err
}
app.Seal()
Expand Down Expand Up @@ -436,13 +432,13 @@ func (app *BaseApp) GetConsensusParams(ctx sdk.Context) *abci.ConsensusParams {
cp.Validator = &vp
}

if app.paramStore.Has(ctx, ParamStoreKeyVersionParams) {
var vp tmproto.VersionParams

app.paramStore.Get(ctx, ParamStoreKeyVersionParams, &vp)
cp.Version = &vp
appVersion, err := app.cms.GetAppVersion()
if err != nil {
panic(err)
}
cp.Version = &tmproto.VersionParams{
AppVersion: appVersion,
}

return cp
}

Expand All @@ -466,9 +462,8 @@ func (app *BaseApp) StoreConsensusParams(ctx sdk.Context, cp *abci.ConsensusPara
app.paramStore.Set(ctx, ParamStoreKeyBlockParams, cp.Block)
app.paramStore.Set(ctx, ParamStoreKeyEvidenceParams, cp.Evidence)
app.paramStore.Set(ctx, ParamStoreKeyValidatorParams, cp.Validator)
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
Copy link
Collaborator

@alexanderbez alexanderbez Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I think this is fine then as long as we have clear documentation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So I would add to the existing comment that the entire reason we do this is to allow state-sync to function properly.

// persisted in the multi-store which is used as the single source of truth.
}

// getMaximumBlockGas gets the maximum gas from the consensus params. It panics
Expand Down
108 changes: 67 additions & 41 deletions baseapp/baseapp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,12 @@ func defaultLogger() log.Logger {
}

func newBaseApp(name string, options ...func(*BaseApp)) *BaseApp {
logger := defaultLogger()
db := dbm.NewMemDB()
return newBaseAppWithDb(name, db, options...)
}

func newBaseAppWithDb(name string, db dbm.DB, options ...func(*BaseApp)) *BaseApp {
p0mvn marked this conversation as resolved.
Show resolved Hide resolved
logger := defaultLogger()
codec := codec.NewLegacyAmino()
registerTestCodec(codec)
return NewBaseApp(name, logger, db, testTxDecoder(codec), options...)
Expand Down Expand Up @@ -482,7 +486,7 @@ func TestInfo(t *testing.T) {
assert.Equal(t, int64(0), res.LastBlockHeight)
require.Equal(t, []uint8(nil), res.LastBlockAppHash)

appVersion, err := app.GetAppVersion(app.deliverState.ctx)
appVersion, err := app.GetAppVersion()
require.NoError(t, err)

assert.Equal(t, appVersion, res.AppVersion)
Expand Down Expand Up @@ -619,7 +623,9 @@ func TestInitChainer(t *testing.T) {
require.Equal(t, value, res.Value)
}

func TestInitChain_ProtocolVersionSetToZero(t *testing.T) {
func TestInitChain_AppVersionSetToZero(t *testing.T) {
const expectedAppVersion = uint64(0)
p0mvn marked this conversation as resolved.
Show resolved Hide resolved

name := t.Name()
db := dbm.NewMemDB()
logger := defaultLogger()
Expand All @@ -632,9 +638,35 @@ func TestInitChain_ProtocolVersionSetToZero(t *testing.T) {
},
)

protocolVersion, err := app.GetAppVersion(app.deliverState.ctx)
protocolVersion, err := app.GetAppVersion()
require.NoError(t, err)
require.Equal(t, uint64(0), protocolVersion)
require.Equal(t, expectedAppVersion, protocolVersion)

consensusParams := app.GetConsensusParams(app.checkState.ctx)

require.Equal(t, expectedAppVersion, consensusParams.Version.AppVersion)
}

func TestInitChain_NonZeroAppVersionInRequestPanic(t *testing.T) {
name := t.Name()
db := dbm.NewMemDB()
logger := defaultLogger()
app := NewBaseApp(name, logger, db, nil)
app.SetParamStore(&mock.ParamStore{Db: dbm.NewMemDB()})

sut := func() {
app.InitChain(
abci.RequestInitChain{
InitialHeight: 3,
ConsensusParams: &abci.ConsensusParams{
Version: &tmproto.VersionParams{
AppVersion: 10,
},
},
},
)
}
require.Panics(t, sut)
}

func TestInitChain_WithInitialHeight(t *testing.T) {
Expand Down Expand Up @@ -1823,8 +1855,8 @@ func TestListSnapshots(t *testing.T) {
defer teardown()

expected := abci.ResponseListSnapshots{Snapshots: []*abci.Snapshot{
{Height: 4, Format: 1, Chunks: 2},
{Height: 2, Format: 1, Chunks: 1},
{Height: 4, Format: 2, Chunks: 2},
{Height: 2, Format: 2, Chunks: 1},
}}

resp := app.ListSnapshots(abci.RequestListSnapshots{})
Expand Down Expand Up @@ -1867,7 +1899,7 @@ func TestSnapshotWithPruning(t *testing.T) {
pruningOpts: pruningtypes.NewPruningOptions(pruningtypes.PruningNothing),
},
expectedSnapshots: []*abci.Snapshot{
{Height: 20, Format: 1, Chunks: 5},
{Height: 20, Format: 2, Chunks: 5},
},
},
"prune everything with snapshot": {
Expand All @@ -1879,7 +1911,7 @@ func TestSnapshotWithPruning(t *testing.T) {
pruningOpts: pruningtypes.NewPruningOptions(pruningtypes.PruningEverything),
},
expectedSnapshots: []*abci.Snapshot{
{Height: 20, Format: 1, Chunks: 5},
{Height: 20, Format: 2, Chunks: 5},
},
},
"default pruning with snapshot": {
Expand All @@ -1891,7 +1923,7 @@ func TestSnapshotWithPruning(t *testing.T) {
pruningOpts: pruningtypes.NewPruningOptions(pruningtypes.PruningDefault),
},
expectedSnapshots: []*abci.Snapshot{
{Height: 20, Format: 1, Chunks: 5},
{Height: 20, Format: 2, Chunks: 5},
},
},
"custom": {
Expand All @@ -1903,8 +1935,8 @@ func TestSnapshotWithPruning(t *testing.T) {
pruningOpts: pruningtypes.NewCustomPruningOptions(12, 12),
},
expectedSnapshots: []*abci.Snapshot{
{Height: 25, Format: 1, Chunks: 6},
{Height: 20, Format: 1, Chunks: 5},
{Height: 25, Format: 2, Chunks: 6},
{Height: 20, Format: 2, Chunks: 5},
},
},
"no snapshots": {
Expand All @@ -1925,9 +1957,9 @@ func TestSnapshotWithPruning(t *testing.T) {
pruningOpts: pruningtypes.NewPruningOptions(pruningtypes.PruningNothing),
},
expectedSnapshots: []*abci.Snapshot{
{Height: 9, Format: 1, Chunks: 2},
{Height: 6, Format: 1, Chunks: 2},
{Height: 3, Format: 1, Chunks: 1},
{Height: 9, Format: 2, Chunks: 2},
{Height: 6, Format: 2, Chunks: 2},
{Height: 3, Format: 2, Chunks: 1},
},
},
}
Expand Down Expand Up @@ -2004,13 +2036,13 @@ func TestLoadSnapshotChunk(t *testing.T) {
chunk uint32
expectEmpty bool
}{
"Existing snapshot": {2, 1, 1, false},
"Missing height": {100, 1, 1, true},
"Missing format": {2, 2, 1, true},
"Missing chunk": {2, 1, 9, true},
"Zero height": {0, 1, 1, true},
"Existing snapshot": {2, 2, 1, false},
"Missing height": {100, 2, 1, true},
"Missing format": {2, 1, 1, true},
"Missing chunk": {2, 2, 9, true},
"Zero height": {0, 2, 1, true},
"Zero format": {2, 0, 1, true},
"Zero chunk": {2, 1, 0, false},
"Zero chunk": {2, 2, 0, false},
}
for name, tc := range testcases {
tc := tc
Expand Down Expand Up @@ -2056,13 +2088,13 @@ func TestOfferSnapshot_Errors(t *testing.T) {
Height: 1, Format: 9, Chunks: 3, Hash: hash, Metadata: metadata,
}, abci.ResponseOfferSnapshot_REJECT_FORMAT},
"incorrect chunk count": {&abci.Snapshot{
Height: 1, Format: 1, Chunks: 2, Hash: hash, Metadata: metadata,
Height: 1, Format: 2, Chunks: 2, Hash: hash, Metadata: metadata,
}, abci.ResponseOfferSnapshot_REJECT},
"no chunks": {&abci.Snapshot{
Height: 1, Format: 1, Chunks: 0, Hash: hash, Metadata: metadata,
Height: 1, Format: 2, Chunks: 0, Hash: hash, Metadata: metadata,
}, abci.ResponseOfferSnapshot_REJECT},
"invalid metadata serialization": {&abci.Snapshot{
Height: 1, Format: 1, Chunks: 0, Hash: hash, Metadata: []byte{3, 1, 4},
Height: 1, Format: 2, Chunks: 0, Hash: hash, Metadata: []byte{3, 1, 4},
}, abci.ResponseOfferSnapshot_REJECT},
}
for name, tc := range testcases {
Expand Down Expand Up @@ -2442,12 +2474,12 @@ func TestBaseApp_Init_PruningAndSnapshot(t *testing.T) {
}
}

func TestBaseApp_Init_ProtocolVersion(t *testing.T) {
const versionNotSet = -1
func TestBaseApp_Init_AppVersion(t *testing.T) {
const versionNotSet = 0

testcases := []struct {
name string
protocolVersion int64
protocolVersion uint64
}{
{
name: "no app version was set - set to 0",
Expand All @@ -2461,29 +2493,23 @@ func TestBaseApp_Init_ProtocolVersion(t *testing.T) {

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
app := newBaseApp(t.Name())
db := dbm.NewMemDB()
app := newBaseAppWithDb(t.Name(), db)

app.SetParamStore(&mock.ParamStore{db})

var expectedProtocolVersion uint64
if tc.protocolVersion != versionNotSet {
// Set version on another app with the same param store (db),
// pretending that the app version was set on the app in advance.
oldApp := newBaseApp(t.Name())
oldApp.SetParamStore(&mock.ParamStore{db})
require.NoError(t, oldApp.init())

expectedProtocolVersion = uint64(tc.protocolVersion)
require.NoError(t, oldApp.SetAppVersion(oldApp.checkState.ctx, expectedProtocolVersion))
err := app.cms.SetAppVersion(tc.protocolVersion)
require.NoError(t, err)
}

// recreate app
app = newBaseAppWithDb(t.Name(), db)

require.NoError(t, app.init())

actualProtocolVersion, err := app.GetAppVersion(app.checkState.ctx)
actualProtocolVersion, err := app.GetAppVersion()
require.NoError(t, err)

require.Equal(t, expectedProtocolVersion, actualProtocolVersion)
require.Equal(t, tc.protocolVersion, actualProtocolVersion)
})
}
}
Loading