Skip to content
This repository was archived by the owner on Nov 16, 2023. It is now read-only.

Commit a68d28e

Browse files
authored
fix: state-sync - app version stored in params (#241)
Closes: #XXX ## What is the purpose of the change Backporting and expanding on: cosmos#11182 This is an attempt to fix app version bug related to state-sync and tendermit. More context is in the linked-above PR. Upon backporting all relevant commits from the linked-above PR, it was discovered that some integration and simulation tests were failing. With the original design, it was difficult to find the source of the problem due to storing the protocol version in 2 database locations. Therefore, the original work was significantly redesigned to only keep the protocol version in param store. Additionally, the protocol version was assumed to have already been set. Since we are creating a new entry in the database, it is now possible that the protocol version is non-existent at the start. As a result, mechanisms for detecting a missing protocol version were added: - `baseapp.init(...)` * This is called on every restart. Here, we check if the protocol version is already in db. If not, set it to 0 - `baseapp.InitChain(...)` * This is called once at genesis. Here, we always set protocol version to 0 Then, on every upgrade the version gets incremented by 1 ## Brief Changelog - cherry-picked all relevant commits from cosmos#11182 - fixed some issues - addressed some style comments from code review in cosmos#11182 - redesigned to only store protocol version in params store and removed logic for storing it in the upgrade keeper - added logic for detecting missing protocol version and, if missing, setting it to 0 - unit and integration tested - fixed all tests ## Testing and Verifying This change is a trivial rework / code cleanup without any test coverage. This change is already covered by existing tests, such as *(please describe tests)*. TODO: This change needs to be manually tested ## Documentation and Release Note - Does this pull request introduce a new feature or user-facing behavior changes? yes - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? no - How is the feature or change documented? not applicable
1 parent a2fcb7a commit a68d28e

File tree

19 files changed

+276
-105
lines changed

19 files changed

+276
-105
lines changed

baseapp/abci.go

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,10 @@ func (app *BaseApp) InitChain(req abci.RequestInitChain) (res abci.ResponseInitC
4545
app.setDeliverState(initHeader)
4646
app.setCheckState(initHeader)
4747

48+
if err := app.SetAppVersion(app.deliverState.ctx, 0); err != nil {
49+
panic(err)
50+
}
51+
4852
// Store the consensus params in the BaseApp's paramstore. Note, this must be
4953
// done after the deliver state and context have been set as it's persisted
5054
// to state.
@@ -107,10 +111,15 @@ func (app *BaseApp) InitChain(req abci.RequestInitChain) (res abci.ResponseInitC
107111
func (app *BaseApp) Info(req abci.RequestInfo) abci.ResponseInfo {
108112
lastCommitID := app.cms.LastCommitID()
109113

114+
appVersion, err := app.GetAppVersion(app.checkState.ctx)
115+
if err != nil {
116+
app.logger.Error("failed to get app version", err)
117+
}
118+
110119
return abci.ResponseInfo{
111120
Data: app.name,
112121
Version: app.version,
113-
AppVersion: app.appVersion,
122+
AppVersion: appVersion,
114123
LastBlockHeight: lastCommitID.Version,
115124
LastBlockAppHash: lastCommitID.Hash,
116125
}
@@ -741,6 +750,7 @@ func handleQueryApp(app *BaseApp, path []string, req abci.RequestQuery) abci.Res
741750
}
742751

743752
case "version":
753+
744754
return abci.ResponseQuery{
745755
Codespace: sdkerrors.RootCodespace,
746756
Height: req.Height,

baseapp/abci_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
pruningtypes "github.com/cosmos/cosmos-sdk/pruning/types"
1313
"github.com/cosmos/cosmos-sdk/snapshots"
1414
snapshottypes "github.com/cosmos/cosmos-sdk/snapshots/types"
15+
"github.com/cosmos/cosmos-sdk/testutil/mock"
1516
snaphotstestutil "github.com/cosmos/cosmos-sdk/testutil/snapshots"
1617
sdk "github.com/cosmos/cosmos-sdk/types"
1718
)
@@ -110,7 +111,7 @@ func TestGetBlockRentionHeight(t *testing.T) {
110111
for name, tc := range testCases {
111112
tc := tc
112113

113-
tc.bapp.SetParamStore(&paramStore{db: dbm.NewMemDB()})
114+
tc.bapp.SetParamStore(&mock.ParamStore{Db: dbm.NewMemDB()})
114115
tc.bapp.InitChain(abci.RequestInitChain{
115116
ConsensusParams: &abci.ConsensusParams{
116117
Evidence: &tmprototypes.EvidenceParams{

baseapp/baseapp.go

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package baseapp
22

33
import (
4+
"errors"
45
"fmt"
56
"reflect"
67
"strings"
@@ -19,6 +20,7 @@ import (
1920
sdk "github.com/cosmos/cosmos-sdk/types"
2021
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
2122
"github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx"
23+
upgrade "github.com/cosmos/cosmos-sdk/x/upgrade/exported"
2224
)
2325

2426
const (
@@ -114,13 +116,9 @@ type BaseApp struct { // nolint: maligned
114116
// ResponseCommit.RetainHeight.
115117
minRetainBlocks uint64
116118

117-
// application's version string
119+
// version represents the application software semantic version
118120
version string
119121

120-
// application's protocol version that increments on every upgrade
121-
// if BaseApp is passed to the upgrade keeper's NewKeeper method.
122-
appVersion uint64
123-
124122
// recovery handler for app.runTx method
125123
runTxRecoveryMiddleware recoveryMiddleware
126124

@@ -132,6 +130,8 @@ type BaseApp struct { // nolint: maligned
132130
indexEvents map[string]struct{}
133131
}
134132

133+
var _ upgrade.ProtocolVersionManager = (*BaseApp)(nil)
134+
135135
// NewBaseApp returns a reference to an initialized BaseApp. It accepts a
136136
// variadic number of option functions, which act on the BaseApp to set
137137
// configuration choices.
@@ -172,11 +172,6 @@ func (app *BaseApp) Name() string {
172172
return app.name
173173
}
174174

175-
// AppVersion returns the application's protocol version.
176-
func (app *BaseApp) AppVersion() uint64 {
177-
return app.appVersion
178-
}
179-
180175
// Version returns the application's version string.
181176
func (app *BaseApp) Version() string {
182177
return app.version
@@ -314,6 +309,18 @@ func (app *BaseApp) init() error {
314309

315310
// needed for the export command which inits from store but never calls initchain
316311
app.setCheckState(tmproto.Header{})
312+
313+
// If there is no app version set in the store, we should set it to 0.
314+
// Panic on any other error.
315+
// If errMsgNoProtocolVersionSet, we assume that appVersion is assigned to be 0.
316+
appVersion, err := app.GetAppVersion(app.checkState.ctx)
317+
if err != nil && !errors.Is(err, errMsgNoProtocolVersionSet) {
318+
return err
319+
}
320+
321+
if err := app.SetAppVersion(app.checkState.ctx, appVersion); err != nil {
322+
return err
323+
}
317324
app.Seal()
318325

319326
rms, ok := app.cms.(*rootmulti.Store)
@@ -429,6 +436,13 @@ func (app *BaseApp) GetConsensusParams(ctx sdk.Context) *abci.ConsensusParams {
429436
cp.Validator = &vp
430437
}
431438

439+
if app.paramStore.Has(ctx, ParamStoreKeyVersionParams) {
440+
var vp tmproto.VersionParams
441+
442+
app.paramStore.Get(ctx, ParamStoreKeyVersionParams, &vp)
443+
cp.Version = &vp
444+
}
445+
432446
return cp
433447
}
434448

@@ -452,6 +466,9 @@ func (app *BaseApp) StoreConsensusParams(ctx sdk.Context, cp *abci.ConsensusPara
452466
app.paramStore.Set(ctx, ParamStoreKeyBlockParams, cp.Block)
453467
app.paramStore.Set(ctx, ParamStoreKeyEvidenceParams, cp.Evidence)
454468
app.paramStore.Set(ctx, ParamStoreKeyValidatorParams, cp.Validator)
469+
app.paramStore.Set(ctx, ParamStoreKeyVersionParams, cp.Version)
470+
// We're explicitly not storing the Tendermint app_version in the param store. It's
471+
// stored instead in the x/upgrade store, with its own bump logic.
455472
}
456473

457474
// getMaximumBlockGas gets the maximum gas from the consensus params. It panics

0 commit comments

Comments
 (0)