-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix!: Capability Issue on Restart, Backport to v0.43 (#9836)
<!-- The default pull request template is for types feat, fix, or refactor. For other templates, add one of the following parameters to the url: - template=docs.md - template=other.md --> ## Description Closes: #9800 The following is a breaking fix to #9800. In the non-breaking fix, the initialization logic was moved out of `InitializeAndSeal` but the API was preserved. I've now removed `InitializeAndSeal` and replaced it with just the `Seal` function, which may be called much more cleanly in `app.go` --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [x] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [x] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) (cherry picked from commit 7f316ad) # Conflicts: # CHANGELOG.md
- Loading branch information
1 parent
6137324
commit cba0ca4
Showing
10 changed files
with
270 additions
and
58 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| package capability | ||
|
|
||
| import ( | ||
| "time" | ||
|
|
||
| "github.com/cosmos/cosmos-sdk/telemetry" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| "github.com/cosmos/cosmos-sdk/x/capability/keeper" | ||
| "github.com/cosmos/cosmos-sdk/x/capability/types" | ||
| ) | ||
|
|
||
| // BeginBlocker will call InitMemStore to initialize the memory stores in the case | ||
| // that this is the first time the node is executing a block since restarting (wiping memory). | ||
| // In this case, the BeginBlocker method will reinitialize the memory stores locally, so that subsequent | ||
| // capability transactions will pass. | ||
| // Otherwise BeginBlocker performs a no-op. | ||
| func BeginBlocker(ctx sdk.Context, k keeper.Keeper) { | ||
| defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker) | ||
|
|
||
| k.InitMemStore(ctx) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| package capability_test | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/suite" | ||
| abci "github.com/tendermint/tendermint/abci/types" | ||
| tmproto "github.com/tendermint/tendermint/proto/tendermint/types" | ||
|
|
||
| "github.com/cosmos/cosmos-sdk/codec" | ||
| "github.com/cosmos/cosmos-sdk/simapp" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| "github.com/cosmos/cosmos-sdk/types/module" | ||
| banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" | ||
| "github.com/cosmos/cosmos-sdk/x/capability" | ||
| "github.com/cosmos/cosmos-sdk/x/capability/keeper" | ||
| "github.com/cosmos/cosmos-sdk/x/capability/types" | ||
| ) | ||
|
|
||
| type CapabilityTestSuite struct { | ||
| suite.Suite | ||
|
|
||
| cdc codec.Codec | ||
| ctx sdk.Context | ||
| app *simapp.SimApp | ||
| keeper *keeper.Keeper | ||
| module module.AppModule | ||
| } | ||
|
|
||
| func (suite *CapabilityTestSuite) SetupTest() { | ||
| checkTx := false | ||
| app := simapp.Setup(checkTx) | ||
| cdc := app.AppCodec() | ||
|
|
||
| // create new keeper so we can define custom scoping before init and seal | ||
| keeper := keeper.NewKeeper(cdc, app.GetKey(types.StoreKey), app.GetMemKey(types.MemStoreKey)) | ||
|
|
||
| suite.app = app | ||
| suite.ctx = app.BaseApp.NewContext(checkTx, tmproto.Header{Height: 1}) | ||
| suite.keeper = keeper | ||
| suite.cdc = cdc | ||
| suite.module = capability.NewAppModule(cdc, *keeper) | ||
| } | ||
|
|
||
| // The following test case mocks a specific bug discovered in https://github.com/cosmos/cosmos-sdk/issues/9800 | ||
| // and ensures that the current code successfully fixes the issue. | ||
| func (suite *CapabilityTestSuite) TestInitializeMemStore() { | ||
| sk1 := suite.keeper.ScopeToModule(banktypes.ModuleName) | ||
|
|
||
| cap1, err := sk1.NewCapability(suite.ctx, "transfer") | ||
| suite.Require().NoError(err) | ||
| suite.Require().NotNil(cap1) | ||
|
|
||
| // mock statesync by creating new keeper that shares persistent state but loses in-memory map | ||
| newKeeper := keeper.NewKeeper(suite.cdc, suite.app.GetKey(types.StoreKey), suite.app.GetMemKey("testingkey")) | ||
| newSk1 := newKeeper.ScopeToModule(banktypes.ModuleName) | ||
|
|
||
| // Mock App startup | ||
| ctx := suite.app.BaseApp.NewUncachedContext(false, tmproto.Header{}) | ||
| newKeeper.Seal() | ||
| suite.Require().False(newKeeper.IsInitialized(ctx), "memstore initialized flag set before BeginBlock") | ||
|
|
||
| // Mock app beginblock and ensure that no gas has been consumed and memstore is initialized | ||
| ctx = suite.app.BaseApp.NewContext(false, tmproto.Header{}).WithBlockGasMeter(sdk.NewGasMeter(50)) | ||
| prevGas := ctx.BlockGasMeter().GasConsumed() | ||
| restartedModule := capability.NewAppModule(suite.cdc, *newKeeper) | ||
| restartedModule.BeginBlock(ctx, abci.RequestBeginBlock{}) | ||
| suite.Require().True(newKeeper.IsInitialized(ctx), "memstore initialized flag not set") | ||
| gasUsed := ctx.BlockGasMeter().GasConsumed() | ||
|
|
||
| suite.Require().Equal(prevGas, gasUsed, "beginblocker consumed gas during execution") | ||
|
|
||
| // Mock the first transaction getting capability and subsequently failing | ||
| // by using a cached context and discarding all cached writes. | ||
| cacheCtx, _ := ctx.CacheContext() | ||
| _, ok := newSk1.GetCapability(cacheCtx, "transfer") | ||
| suite.Require().True(ok) | ||
|
|
||
| // Ensure that the second transaction can still receive capability even if first tx fails. | ||
| ctx = suite.app.BaseApp.NewContext(false, tmproto.Header{}) | ||
|
|
||
| cap1, ok = newSk1.GetCapability(ctx, "transfer") | ||
| suite.Require().True(ok) | ||
|
|
||
| // Ensure the capabilities don't get reinitialized on next BeginBlock | ||
| // by testing to see if capability returns same pointer | ||
| // also check that initialized flag is still set | ||
| restartedModule.BeginBlock(ctx, abci.RequestBeginBlock{}) | ||
| recap, ok := newSk1.GetCapability(ctx, "transfer") | ||
| suite.Require().True(ok) | ||
| suite.Require().Equal(cap1, recap, "capabilities got reinitialized after second BeginBlock") | ||
| suite.Require().True(newKeeper.IsInitialized(ctx), "memstore initialized flag not set") | ||
| } | ||
|
|
||
| func TestCapabilityTestSuite(t *testing.T) { | ||
| suite.Run(t, new(CapabilityTestSuite)) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| package capability_test | ||
|
|
||
| import ( | ||
| "github.com/tendermint/tendermint/libs/log" | ||
| tmproto "github.com/tendermint/tendermint/proto/tendermint/types" | ||
| dbm "github.com/tendermint/tm-db" | ||
|
|
||
| "github.com/cosmos/cosmos-sdk/simapp" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" | ||
| "github.com/cosmos/cosmos-sdk/x/capability" | ||
| "github.com/cosmos/cosmos-sdk/x/capability/keeper" | ||
| "github.com/cosmos/cosmos-sdk/x/capability/types" | ||
| stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" | ||
| ) | ||
|
|
||
| func (suite *CapabilityTestSuite) TestGenesis() { | ||
| sk1 := suite.keeper.ScopeToModule(banktypes.ModuleName) | ||
| sk2 := suite.keeper.ScopeToModule(stakingtypes.ModuleName) | ||
|
|
||
| cap1, err := sk1.NewCapability(suite.ctx, "transfer") | ||
| suite.Require().NoError(err) | ||
| suite.Require().NotNil(cap1) | ||
|
|
||
| err = sk2.ClaimCapability(suite.ctx, cap1, "transfer") | ||
| suite.Require().NoError(err) | ||
|
|
||
| cap2, err := sk2.NewCapability(suite.ctx, "ica") | ||
| suite.Require().NoError(err) | ||
| suite.Require().NotNil(cap2) | ||
|
|
||
| genState := capability.ExportGenesis(suite.ctx, *suite.keeper) | ||
|
|
||
| // create new app that does not share persistent or in-memory state | ||
| // and initialize app from exported genesis state above. | ||
| db := dbm.NewMemDB() | ||
| encCdc := simapp.MakeTestEncodingConfig() | ||
| newApp := simapp.NewSimApp(log.NewNopLogger(), db, nil, true, map[int64]bool{}, simapp.DefaultNodeHome, 5, encCdc, simapp.EmptyAppOptions{}) | ||
|
|
||
| newKeeper := keeper.NewKeeper(suite.cdc, newApp.GetKey(types.StoreKey), newApp.GetMemKey(types.MemStoreKey)) | ||
| newSk1 := newKeeper.ScopeToModule(banktypes.ModuleName) | ||
| newSk2 := newKeeper.ScopeToModule(stakingtypes.ModuleName) | ||
| deliverCtx, _ := newApp.BaseApp.NewUncachedContext(false, tmproto.Header{}).WithBlockGasMeter(sdk.NewInfiniteGasMeter()).CacheContext() | ||
|
|
||
| capability.InitGenesis(deliverCtx, *newKeeper, *genState) | ||
|
|
||
| // check that all previous capabilities exist in new app after InitGenesis | ||
| sk1Cap1, ok := newSk1.GetCapability(deliverCtx, "transfer") | ||
| suite.Require().True(ok, "could not get first capability after genesis on first ScopedKeeper") | ||
| suite.Require().Equal(*cap1, *sk1Cap1, "capability values not equal on first ScopedKeeper") | ||
|
|
||
| sk2Cap1, ok := newSk2.GetCapability(deliverCtx, "transfer") | ||
| suite.Require().True(ok, "could not get first capability after genesis on first ScopedKeeper") | ||
| suite.Require().Equal(*cap1, *sk2Cap1, "capability values not equal on first ScopedKeeper") | ||
|
|
||
| sk2Cap2, ok := newSk2.GetCapability(deliverCtx, "ica") | ||
| suite.Require().True(ok, "could not get second capability after genesis on second ScopedKeeper") | ||
| suite.Require().Equal(*cap2, *sk2Cap2, "capability values not equal on second ScopedKeeper") | ||
| } |
Oops, something went wrong.