Skip to content

Commit

Permalink
fix: state-sync - app version stored in params (#241)
Browse files Browse the repository at this point in the history
Closes: #XXX

## What is the purpose of the change

Backporting and expanding on: #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 #11182
- fixed some issues
- addressed some style comments from code review in #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
  • Loading branch information
p0mvn authored May 23, 2022
1 parent a2fcb7a commit a68d28e
Show file tree
Hide file tree
Showing 19 changed files with 276 additions and 105 deletions.
12 changes: 11 additions & 1 deletion baseapp/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ 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 {
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.
Expand Down Expand Up @@ -107,10 +111,15 @@ 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)
if err != nil {
app.logger.Error("failed to get app version", err)
}

return abci.ResponseInfo{
Data: app.name,
Version: app.version,
AppVersion: app.appVersion,
AppVersion: appVersion,
LastBlockHeight: lastCommitID.Version,
LastBlockAppHash: lastCommitID.Hash,
}
Expand Down Expand Up @@ -741,6 +750,7 @@ func handleQueryApp(app *BaseApp, path []string, req abci.RequestQuery) abci.Res
}

case "version":

return abci.ResponseQuery{
Codespace: sdkerrors.RootCodespace,
Height: req.Height,
Expand Down
3 changes: 2 additions & 1 deletion baseapp/abci_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
pruningtypes "github.com/cosmos/cosmos-sdk/pruning/types"
"github.com/cosmos/cosmos-sdk/snapshots"
snapshottypes "github.com/cosmos/cosmos-sdk/snapshots/types"
"github.com/cosmos/cosmos-sdk/testutil/mock"
snaphotstestutil "github.com/cosmos/cosmos-sdk/testutil/snapshots"
sdk "github.com/cosmos/cosmos-sdk/types"
)
Expand Down Expand Up @@ -110,7 +111,7 @@ func TestGetBlockRentionHeight(t *testing.T) {
for name, tc := range testCases {
tc := tc

tc.bapp.SetParamStore(&paramStore{db: dbm.NewMemDB()})
tc.bapp.SetParamStore(&mock.ParamStore{Db: dbm.NewMemDB()})
tc.bapp.InitChain(abci.RequestInitChain{
ConsensusParams: &abci.ConsensusParams{
Evidence: &tmprototypes.EvidenceParams{
Expand Down
37 changes: 27 additions & 10 deletions baseapp/baseapp.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package baseapp

import (
"errors"
"fmt"
"reflect"
"strings"
Expand All @@ -19,6 +20,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx"
upgrade "github.com/cosmos/cosmos-sdk/x/upgrade/exported"
)

const (
Expand Down Expand Up @@ -114,13 +116,9 @@ type BaseApp struct { // nolint: maligned
// ResponseCommit.RetainHeight.
minRetainBlocks uint64

// application's version string
// version represents the application software semantic version
version string

// application's protocol version that increments on every upgrade
// if BaseApp is passed to the upgrade keeper's NewKeeper method.
appVersion uint64

// recovery handler for app.runTx method
runTxRecoveryMiddleware recoveryMiddleware

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

var _ upgrade.ProtocolVersionManager = (*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
// configuration choices.
Expand Down Expand Up @@ -172,11 +172,6 @@ func (app *BaseApp) Name() string {
return app.name
}

// AppVersion returns the application's protocol version.
func (app *BaseApp) AppVersion() uint64 {
return app.appVersion
}

// Version returns the application's version string.
func (app *BaseApp) Version() string {
return app.version
Expand Down Expand Up @@ -314,6 +309,18 @@ 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) {
return err
}

if err := app.SetAppVersion(app.checkState.ctx, appVersion); err != nil {
return err
}
app.Seal()

rms, ok := app.cms.(*rootmulti.Store)
Expand Down Expand Up @@ -429,6 +436,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
}

return cp
}

Expand All @@ -452,6 +466,9 @@ 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.
}

// getMaximumBlockGas gets the maximum gas from the consensus params. It panics
Expand Down
Loading

0 comments on commit a68d28e

Please sign in to comment.