From 66854d0e7c3f21e0429cce910d7b03c5c9eaf17f Mon Sep 17 00:00:00 2001 From: Philip Su Date: Thu, 11 Jan 2024 12:18:52 -0800 Subject: [PATCH 1/7] Add migration handler for disabling seqno (#394) ## Describe your changes and provide context Add migration handler to add new disable seqno parameter to chain. Note that i verified that we already use default values so we can just set the params to default instead of maintaining authParamsV1 and V2 ## Testing performed to validate your change - unit tests - upgraded local chain, ensure that bank sends work --- x/auth/keeper/migrations.go | 6 ++++++ x/auth/keeper/v2_to_v3_test.go | 28 ++++++++++++++++++++++++++++ x/auth/module.go | 6 +++++- 3 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 x/auth/keeper/v2_to_v3_test.go diff --git a/x/auth/keeper/migrations.go b/x/auth/keeper/migrations.go index d3ad7a2f8..4e16d9288 100644 --- a/x/auth/keeper/migrations.go +++ b/x/auth/keeper/migrations.go @@ -41,3 +41,9 @@ func (m Migrator) Migrate1to2(ctx sdk.Context) error { return iterErr } + +func (m Migrator) Migrate2to3(ctx sdk.Context) error { + defaultParams := types.DefaultParams() + m.keeper.SetParams(ctx, defaultParams) + return nil +} diff --git a/x/auth/keeper/v2_to_v3_test.go b/x/auth/keeper/v2_to_v3_test.go new file mode 100644 index 000000000..92b3e631b --- /dev/null +++ b/x/auth/keeper/v2_to_v3_test.go @@ -0,0 +1,28 @@ +package keeper_test + +import ( + "github.com/cosmos/cosmos-sdk/x/auth/keeper" + "github.com/cosmos/cosmos-sdk/x/auth/types" + "github.com/stretchr/testify/require" + "testing" +) + +func TestMigrate2to3(t *testing.T) { + app, ctx := createTestApp(true) + + prevParams := types.Params{ + MaxMemoCharacters: types.DefaultMaxMemoCharacters, + TxSigLimit: types.DefaultTxSigLimit, + TxSizeCostPerByte: types.DefaultTxSizeCostPerByte, + SigVerifyCostED25519: types.DefaultSigVerifyCostED25519, + SigVerifyCostSecp256k1: types.DefaultSigVerifyCostSecp256k1, + } + + app.AccountKeeper.SetParams(ctx, prevParams) + // migrate to default params + m := keeper.NewMigrator(app.AccountKeeper, app.GRPCQueryRouter()) + err := m.Migrate2to3(ctx) + require.NoError(t, err) + params := app.AccountKeeper.GetParams(ctx) + require.Equal(t, params.DisableSeqnoCheck, false) +} diff --git a/x/auth/module.go b/x/auth/module.go index ef13f74a5..ef2d1f2e2 100644 --- a/x/auth/module.go +++ b/x/auth/module.go @@ -134,6 +134,10 @@ func (am AppModule) RegisterServices(cfg module.Configurator) { if err != nil { panic(err) } + err = cfg.RegisterMigration(types.ModuleName, 2, m.Migrate2to3) + if err != nil { + panic(err) + } } // InitGenesis performs genesis initialization for the auth module. It returns @@ -153,7 +157,7 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw } // ConsensusVersion implements AppModule/ConsensusVersion. -func (AppModule) ConsensusVersion() uint64 { return 2 } +func (AppModule) ConsensusVersion() uint64 { return 3 } // AppModuleSimulation functions From e12efbc30e5c024986d3b846e66d441319d9faf0 Mon Sep 17 00:00:00 2001 From: Yiming Zang <50607998+yzang2019@users.noreply.github.com> Date: Wed, 17 Jan 2024 12:33:23 -0800 Subject: [PATCH 2/7] Revert removing events for cachekv (#396) ## Describe your changes and provide context This PR reverts the change in https://github.com/sei-protocol/sei-cosmos/pull/353 and https://github.com/sei-protocol/sei-cosmos/pull/391 until we have OCC fully enabled. ## Testing performed to validate your change Unit test coverage --- store/cachekv/mergeiterator.go | 23 +++++++++++++++-------- store/cachekv/mergeiterator_test.go | 28 ++++++++++++++++++++++------ store/cachekv/store.go | 7 ++++++- 3 files changed, 43 insertions(+), 15 deletions(-) diff --git a/store/cachekv/mergeiterator.go b/store/cachekv/mergeiterator.go index a32dfb346..f13c4025c 100644 --- a/store/cachekv/mergeiterator.go +++ b/store/cachekv/mergeiterator.go @@ -16,10 +16,11 @@ import ( // // TODO: Optimize by memoizing. type cacheMergeIterator struct { - parent types.Iterator - cache types.Iterator - ascending bool - storeKey sdktypes.StoreKey + parent types.Iterator + cache types.Iterator + ascending bool + storeKey sdktypes.StoreKey + eventManager *sdktypes.EventManager } var _ types.Iterator = (*cacheMergeIterator)(nil) @@ -28,12 +29,14 @@ func NewCacheMergeIterator( parent, cache types.Iterator, ascending bool, storeKey sdktypes.StoreKey, + eventManager *sdktypes.EventManager, ) *cacheMergeIterator { iter := &cacheMergeIterator{ - parent: parent, - cache: cache, - ascending: ascending, - storeKey: storeKey, + parent: parent, + cache: cache, + ascending: ascending, + storeKey: storeKey, + eventManager: eventManager, } return iter @@ -135,12 +138,14 @@ func (iter *cacheMergeIterator) Value() []byte { // If parent is invalid, get the cache value. if !iter.parent.Valid() { value := iter.cache.Value() + iter.eventManager.EmitResourceAccessReadEvent("iterator", iter.storeKey, iter.cache.Key(), value) return value } // If cache is invalid, get the parent value. if !iter.cache.Valid() { value := iter.parent.Value() + iter.eventManager.EmitResourceAccessReadEvent("iterator", iter.storeKey, iter.parent.Key(), value) return value } @@ -151,9 +156,11 @@ func (iter *cacheMergeIterator) Value() []byte { switch cmp { case -1: // parent < cache value := iter.parent.Value() + iter.eventManager.EmitResourceAccessReadEvent("iterator", iter.storeKey, keyP, value) return value case 0, 1: // parent >= cache value := iter.cache.Value() + iter.eventManager.EmitResourceAccessReadEvent("iterator", iter.storeKey, keyC, value) return value default: panic("invalid comparison result") diff --git a/store/cachekv/mergeiterator_test.go b/store/cachekv/mergeiterator_test.go index b2648a865..00f065151 100644 --- a/store/cachekv/mergeiterator_test.go +++ b/store/cachekv/mergeiterator_test.go @@ -6,6 +6,7 @@ import ( "github.com/cosmos/cosmos-sdk/store/cachekv" "github.com/cosmos/cosmos-sdk/store/dbadapter" "github.com/cosmos/cosmos-sdk/store/types" + sdktypes "github.com/cosmos/cosmos-sdk/types" "github.com/stretchr/testify/require" dbm "github.com/tendermint/tm-db" ) @@ -13,6 +14,7 @@ import ( func TestMangerIterator(t *testing.T) { // initiate mock kvstore mem := dbadapter.Store{DB: dbm.NewMemDB()} + eventManager := sdktypes.NewEventManager() kvstore := cachekv.NewStore(mem, types.NewKVStoreKey("CacheKvTest"), types.DefaultCacheSizeLimit) value := randSlice(defaultValueSizeBz) startKey := randSlice(32) @@ -27,13 +29,27 @@ func TestMangerIterator(t *testing.T) { cache := kvstore.Iterator(nil, nil) for ; cache.Valid(); cache.Next() { } - iter := cachekv.NewCacheMergeIterator(parent, cache, true, types.NewKVStoreKey("CacheKvTest")) + iter := cachekv.NewCacheMergeIterator(parent, cache, true, types.NewKVStoreKey("CacheKvTest"), eventManager) - // get the next value and it should not be nil - nextValue := iter.Value() - require.NotNil(t, nextValue) + // get the next value + iter.Value() + + // assert the resource access is still emitted correctly when the cache store is unavailable + require.Equal(t, "access_type", string(eventManager.Events()[0].Attributes[0].Key)) + require.Equal(t, "read", string(eventManager.Events()[0].Attributes[0].Value)) + require.Equal(t, "store_key", string(eventManager.Events()[0].Attributes[1].Key)) + require.Equal(t, "CacheKvTest", string(eventManager.Events()[0].Attributes[1].Value)) + + // assert event emission when cache is available + cache = kvstore.Iterator(keys[1], keys[2]) + iter = cachekv.NewCacheMergeIterator(parent, cache, true, types.NewKVStoreKey("CacheKvTest"), eventManager) // get the next value - nextValue = iter.Value() - require.NotNil(t, nextValue) + iter.Value() + + // assert the resource access is still emitted correctly when the cache store is available + require.Equal(t, "access_type", string(eventManager.Events()[0].Attributes[0].Key)) + require.Equal(t, "read", string(eventManager.Events()[0].Attributes[0].Value)) + require.Equal(t, "store_key", string(eventManager.Events()[0].Attributes[1].Key)) + require.Equal(t, "CacheKvTest", string(eventManager.Events()[0].Attributes[1].Value)) } diff --git a/store/cachekv/store.go b/store/cachekv/store.go index 06d491d04..f9dee6cbb 100644 --- a/store/cachekv/store.go +++ b/store/cachekv/store.go @@ -56,6 +56,8 @@ func (store *Store) GetEvents() []abci.Event { // Implements Store func (store *Store) ResetEvents() { + store.mtx.Lock() + defer store.mtx.Unlock() store.eventManager = sdktypes.NewEventManager() } @@ -75,6 +77,7 @@ func (store *Store) getFromCache(key []byte) []byte { // Get implements types.KVStore. func (store *Store) Get(key []byte) (value []byte) { types.AssertValidKey(key) + store.eventManager.EmitResourceAccessReadEvent("get", store.storeKey, key, value) return store.getFromCache(key) } @@ -83,11 +86,13 @@ func (store *Store) Set(key []byte, value []byte) { types.AssertValidKey(key) types.AssertValidValue(value) store.setCacheValue(key, value, false, true) + store.eventManager.EmitResourceAccessWriteEvent("set", store.storeKey, key, value) } // Has implements types.KVStore. func (store *Store) Has(key []byte) bool { value := store.Get(key) + store.eventManager.EmitResourceAccessReadEvent("has", store.storeKey, key, value) return value != nil } @@ -189,7 +194,7 @@ func (store *Store) iterator(start, end []byte, ascending bool) types.Iterator { }() store.dirtyItems(start, end) cache = newMemIterator(start, end, store.sortedCache, store.deleted, ascending, store.eventManager, store.storeKey) - return NewCacheMergeIterator(parent, cache, ascending, store.storeKey) + return NewCacheMergeIterator(parent, cache, ascending, store.storeKey, store.eventManager) } func findStartIndex(strL []string, startQ string) int { From f01454cedb92613dc3529371c041de9088498677 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Wed, 24 Jan 2024 13:24:10 -0500 Subject: [PATCH 3/7] fix(baseapp): Ensure Panic Recovery in Prepare & Process Handlers (#401) ## Changelog * Ensure we have panic recovery handlers in `PrepareProposal` and `ProcessProposal`. --- baseapp/abci.go | 61 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 48 insertions(+), 13 deletions(-) diff --git a/baseapp/abci.go b/baseapp/abci.go index f8201b040..b70874ace 100644 --- a/baseapp/abci.go +++ b/baseapp/abci.go @@ -25,6 +25,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/types/legacytm" + "github.com/cosmos/cosmos-sdk/utils" ) // InitChain implements the ABCI interface. It runs the initialization logic @@ -912,7 +913,7 @@ func splitPath(requestPath string) (path []string) { } // ABCI++ -func (app *BaseApp) PrepareProposal(ctx context.Context, req *abci.RequestPrepareProposal) (*abci.ResponsePrepareProposal, error) { +func (app *BaseApp) PrepareProposal(ctx context.Context, req *abci.RequestPrepareProposal) (resp *abci.ResponsePrepareProposal, err error) { defer telemetry.MeasureSince(time.Now(), "abci", "prepare_proposal") header := tmproto.Header{ @@ -946,21 +947,40 @@ func (app *BaseApp) PrepareProposal(ctx context.Context, req *abci.RequestPrepar app.preparePrepareProposalState() + defer func() { + if err := recover(); err != nil { + app.logger.Error( + "panic recovered in PrepareProposal", + "height", req.Height, + "time", req.Time, + "panic", err, + ) + + resp = &abci.ResponsePrepareProposal{ + TxRecords: utils.Map(req.Txs, func(tx []byte) *abci.TxRecord { + return &abci.TxRecord{Action: abci.TxRecord_UNMODIFIED, Tx: tx} + }), + } + } + }() + if app.prepareProposalHandler != nil { - res, err := app.prepareProposalHandler(app.prepareProposalState.ctx, req) + resp, err = app.prepareProposalHandler(app.prepareProposalState.ctx, req) if err != nil { return nil, err } + if cp := app.GetConsensusParams(app.prepareProposalState.ctx); cp != nil { - res.ConsensusParamUpdates = cp + resp.ConsensusParamUpdates = cp } - return res, nil - } else { - return nil, errors.New("no prepare proposal handler") + + return resp, nil } + + return nil, errors.New("no prepare proposal handler") } -func (app *BaseApp) ProcessProposal(ctx context.Context, req *abci.RequestProcessProposal) (*abci.ResponseProcessProposal, error) { +func (app *BaseApp) ProcessProposal(ctx context.Context, req *abci.RequestProcessProposal) (resp *abci.ResponseProcessProposal, err error) { defer telemetry.MeasureSince(time.Now(), "abci", "process_proposal") header := tmproto.Header{ @@ -1001,21 +1021,36 @@ func (app *BaseApp) ProcessProposal(ctx context.Context, req *abci.RequestProces } // NOTE: header hash is not set in NewContext, so we manually set it here - app.prepareProcessProposalState(gasMeter, req.Hash) + defer func() { + if err := recover(); err != nil { + app.logger.Error( + "panic recovered in ProcessProposal", + "height", req.Height, + "time", req.Time, + "hash", fmt.Sprintf("%X", req.Hash), + "panic", err, + ) + + resp = &abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_REJECT} + } + }() + if app.processProposalHandler != nil { - res, err := app.processProposalHandler(app.processProposalState.ctx, req) + resp, err = app.processProposalHandler(app.processProposalState.ctx, req) if err != nil { return nil, err } + if cp := app.GetConsensusParams(app.processProposalState.ctx); cp != nil { - res.ConsensusParamUpdates = cp + resp.ConsensusParamUpdates = cp } - return res, nil - } else { - return nil, errors.New("no process proposal handler") + + return resp, nil } + + return nil, errors.New("no process proposal handler") } func (app *BaseApp) FinalizeBlock(ctx context.Context, req *abci.RequestFinalizeBlock) (*abci.ResponseFinalizeBlock, error) { From 5a1afb8b981d5cd79861ed9cb2d73d7f7783a436 Mon Sep 17 00:00:00 2001 From: Uday Patil Date: Thu, 25 Jan 2024 11:28:41 -0600 Subject: [PATCH 4/7] No longer disable dynamic dep generation during ACL dependency generation (#404) ## Describe your changes and provide context This removes the behavior of disabling a dynamic dependency generator if it returns something invalid during message dependency generation. This needs to be removed so that the state writes are consistent with synchronous execution, such that message dependency generation is a read-only path. This change won't cause correctness issues, but in the case that there is an incorrect dynamic dependency generator, it will result in wasted compute since it will try it every time instead of disabling it after the first time it was incorrect. This seems like an acceptable compromise in order to have consistency with synchronous execution. ## Testing performed to validate your change Updated unit tests, and tested this behavior on a loadtest cluster --- x/accesscontrol/keeper/keeper.go | 5 ----- x/accesscontrol/keeper/keeper_test.go | 7 ++++--- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/x/accesscontrol/keeper/keeper.go b/x/accesscontrol/keeper/keeper.go index 1ca93f602..2a2719f7e 100644 --- a/x/accesscontrol/keeper/keeper.go +++ b/x/accesscontrol/keeper/keeper.go @@ -578,11 +578,6 @@ func (k Keeper) GetMessageDependencies(ctx sdk.Context, msg sdk.Msg) []acltypes. ctx.Logger().Error(errorMessage) } } - if dependencyMapping.DynamicEnabled { - // there was an issue with dynamic generation, so lets disable it - // this will not error, the validation check was done in previous calls already - _ = k.SetDependencyMappingDynamicFlag(ctx, messageKey, false) - } return dependencyMapping.AccessOps } diff --git a/x/accesscontrol/keeper/keeper_test.go b/x/accesscontrol/keeper/keeper_test.go index f08cd1ade..52e714f5c 100644 --- a/x/accesscontrol/keeper/keeper_test.go +++ b/x/accesscontrol/keeper/keeper_test.go @@ -112,7 +112,8 @@ func TestInvalidGetMessageDependencies(t *testing.T) { delete(app.AccessControlKeeper.MessageDependencyGeneratorMapper, undelegateKey) accessOps := app.AccessControlKeeper.GetMessageDependencies(ctx, &stakingUndelegate) require.Equal(t, types.SynchronousMessageDependencyMapping("").AccessOps, accessOps) - require.False(t, app.AccessControlKeeper.GetResourceDependencyMapping(ctx, undelegateKey).DynamicEnabled) + // no longer gets disabled such that there arent writes in the dependency generation path + require.True(t, app.AccessControlKeeper.GetResourceDependencyMapping(ctx, undelegateKey).DynamicEnabled) } func TestWasmDependencyMapping(t *testing.T) { @@ -2464,14 +2465,14 @@ func (suite *KeeperTestSuite) TestMessageDependencies() { req.Equal(delegateStaticMapping.AccessOps, accessOps) // verify dynamic got disabled dependencyMapping = app.AccessControlKeeper.GetResourceDependencyMapping(ctx, delegateKey) - req.Equal(false, dependencyMapping.DynamicEnabled) + req.Equal(true, dependencyMapping.DynamicEnabled) // lets also try with undelegate, but this time there is no dynamic generator, so we disable it as well accessOps = app.AccessControlKeeper.GetMessageDependencies(ctx, &stakingUndelegate) req.Equal(undelegateStaticMapping.AccessOps, accessOps) // verify dynamic got disabled dependencyMapping = app.AccessControlKeeper.GetResourceDependencyMapping(ctx, undelegateKey) - req.Equal(false, dependencyMapping.DynamicEnabled) + req.Equal(true, dependencyMapping.DynamicEnabled) } func (suite *KeeperTestSuite) TestImportContractReferences() { From 219175b7698a8f7ace2827fc48e603d1a887335d Mon Sep 17 00:00:00 2001 From: Yiming Zang <50607998+yzang2019@users.noreply.github.com> Date: Tue, 30 Jan 2024 09:01:39 +0800 Subject: [PATCH 5/7] [SeiDB] Fix concurrent map access (#411) ## Describe your changes and provide context This should fix the concurrent map access for storeV2 root multistore over (rs.ckvStores). The problem is that it is currently not protected by a read lock so when other goroutine modify the map, it will throw panic ## Testing performed to validate your change --- storev2/rootmulti/store.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/storev2/rootmulti/store.go b/storev2/rootmulti/store.go index 89076e7d1..be7f4f2af 100644 --- a/storev2/rootmulti/store.go +++ b/storev2/rootmulti/store.go @@ -237,6 +237,8 @@ func (rs *Store) CacheMultiStoreWithVersion(version int64) (types.CacheMultiStor if version <= 0 || (rs.lastCommitInfo != nil && version == rs.lastCommitInfo.Version) { return rs.CacheMultiStore(), nil } + rs.mtx.RLock() + defer rs.mtx.RUnlock() stores := make(map[types.StoreKey]types.CacheWrapper) // add the transient/mem stores registered in current app. for k, store := range rs.ckvStores { From a88d442ac81ad7e13851b51e79153b605c762d34 Mon Sep 17 00:00:00 2001 From: Yiming Zang <50607998+yzang2019@users.noreply.github.com> Date: Fri, 2 Feb 2024 06:02:26 +0800 Subject: [PATCH 6/7] [SeiDB] Fix various issues from ottersec audit (#418) ## Describe your changes and provide context Fix a bunch of minor issues from audit: - LoadVersionAndUpgrade should not ignore version - Simplify CacheWrap to avoid wrapping twice - SS store query is not setting height correctly in the query response - Add panic logic for bumpversion if it is false in commit - Use the correct commit info in query ## Testing performed to validate your change --- storev2/rootmulti/store.go | 24 +++++++++++++++++------- storev2/state/store.go | 2 +- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/storev2/rootmulti/store.go b/storev2/rootmulti/store.go index be7f4f2af..6b168e48f 100644 --- a/storev2/rootmulti/store.go +++ b/storev2/rootmulti/store.go @@ -91,7 +91,7 @@ func NewStore( // Commit implements interface Committer, called by ABCI Commit func (rs *Store) Commit(bumpVersion bool) types.CommitID { if !bumpVersion { - return rs.lastCommitInfo.CommitID() + panic("Commit should always bump version in root multistore") } if err := rs.flush(); err != nil { panic(err) @@ -206,8 +206,8 @@ func (rs *Store) GetStoreType() types.StoreType { } // Implements interface CacheWrapper -func (rs *Store) CacheWrap(storeKey types.StoreKey) types.CacheWrap { - return rs.CacheMultiStore().CacheWrap(storeKey) +func (rs *Store) CacheWrap(_ types.StoreKey) types.CacheWrap { + return rs.CacheMultiStore().(types.CacheWrap) } // Implements interface CacheWrapper @@ -356,6 +356,12 @@ func (rs *Store) LoadVersionAndUpgrade(version int64, upgrades *types.StoreUpgra if err := rs.scStore.Initialize(initialStores); err != nil { return err } + if version > 0 { + _, err := rs.scStore.LoadVersion(version, false) + if err != nil { + return nil + } + } var treeUpgrades []*proto.TreeNameUpgrade for _, key := range storesKeys { @@ -488,6 +494,7 @@ func (rs *Store) Query(req abci.RequestQuery) abci.ResponseQuery { return sdkerrors.QueryResult(err) } var store types.Queryable + var commitInfo *types.CommitInfo if !req.Prove && version < rs.lastCommitInfo.Version && rs.ssStore != nil { // Serve abci query from ss store if no proofs needed @@ -500,9 +507,13 @@ func (rs *Store) Query(req abci.RequestQuery) abci.ResponseQuery { return sdkerrors.QueryResult(err) } store = types.Queryable(commitment.NewStore(scStore.GetTreeByName(storeName), rs.logger)) + commitInfo = convertCommitInfo(scStore.LastCommitInfo()) + commitInfo = amendCommitInfo(commitInfo, rs.storesParams) } else { // Serve directly from latest sc store store = types.Queryable(commitment.NewStore(rs.scStore.GetTreeByName(storeName), rs.logger)) + commitInfo = convertCommitInfo(rs.scStore.LastCommitInfo()) + commitInfo = amendCommitInfo(commitInfo, rs.storesParams) } // trim the path and execute the query @@ -511,14 +522,13 @@ func (rs *Store) Query(req abci.RequestQuery) abci.ResponseQuery { if !req.Prove || !rootmulti.RequireProof(subPath) { return res + } else if commitInfo != nil { + // Restore origin path and append proof op. + res.ProofOps.Ops = append(res.ProofOps.Ops, commitInfo.ProofOp(storeName)) } if res.ProofOps == nil || len(res.ProofOps.Ops) == 0 { return sdkerrors.QueryResult(errors.Wrap(sdkerrors.ErrInvalidRequest, "proof is unexpectedly empty; ensure height has not been pruned")) } - commitInfo := convertCommitInfo(rs.scStore.LastCommitInfo()) - commitInfo = amendCommitInfo(commitInfo, rs.storesParams) - // Restore origin path and append proof op. - res.ProofOps.Ops = append(res.ProofOps.Ops, commitInfo.ProofOp(storeName)) return res } diff --git a/storev2/state/store.go b/storev2/state/store.go index 72e4b0ce9..76c129bf5 100644 --- a/storev2/state/store.go +++ b/storev2/state/store.go @@ -97,6 +97,7 @@ func (st *Store) Query(req abci.RequestQuery) (res abci.ResponseQuery) { if req.Height > 0 && req.Height > st.version { return sdkerrors.QueryResult(errors.Wrap(sdkerrors.ErrInvalidHeight, "invalid height")) } + res.Height = st.version switch req.Path { case "/key": // get by key res.Key = req.Data // data holds the key bytes @@ -105,7 +106,6 @@ func (st *Store) Query(req abci.RequestQuery) (res abci.ResponseQuery) { pairs := kv.Pairs{ Pairs: make([]kv.Pair, 0), } - subspace := req.Data res.Key = subspace iterator := types.KVStorePrefixIterator(st, subspace) From b005e1f1fba942c53043bd29d7893e9458e861ed Mon Sep 17 00:00:00 2001 From: Steven Landers Date: Tue, 6 Feb 2024 13:12:09 -0500 Subject: [PATCH 7/7] Revert "Revert removing events for cachekv (#396)" This reverts commit e12efbc30e5c024986d3b846e66d441319d9faf0. --- store/cachekv/mergeiterator.go | 23 ++++++++--------------- store/cachekv/mergeiterator_test.go | 28 ++++++---------------------- store/cachekv/store.go | 7 +------ 3 files changed, 15 insertions(+), 43 deletions(-) diff --git a/store/cachekv/mergeiterator.go b/store/cachekv/mergeiterator.go index f13c4025c..a32dfb346 100644 --- a/store/cachekv/mergeiterator.go +++ b/store/cachekv/mergeiterator.go @@ -16,11 +16,10 @@ import ( // // TODO: Optimize by memoizing. type cacheMergeIterator struct { - parent types.Iterator - cache types.Iterator - ascending bool - storeKey sdktypes.StoreKey - eventManager *sdktypes.EventManager + parent types.Iterator + cache types.Iterator + ascending bool + storeKey sdktypes.StoreKey } var _ types.Iterator = (*cacheMergeIterator)(nil) @@ -29,14 +28,12 @@ func NewCacheMergeIterator( parent, cache types.Iterator, ascending bool, storeKey sdktypes.StoreKey, - eventManager *sdktypes.EventManager, ) *cacheMergeIterator { iter := &cacheMergeIterator{ - parent: parent, - cache: cache, - ascending: ascending, - storeKey: storeKey, - eventManager: eventManager, + parent: parent, + cache: cache, + ascending: ascending, + storeKey: storeKey, } return iter @@ -138,14 +135,12 @@ func (iter *cacheMergeIterator) Value() []byte { // If parent is invalid, get the cache value. if !iter.parent.Valid() { value := iter.cache.Value() - iter.eventManager.EmitResourceAccessReadEvent("iterator", iter.storeKey, iter.cache.Key(), value) return value } // If cache is invalid, get the parent value. if !iter.cache.Valid() { value := iter.parent.Value() - iter.eventManager.EmitResourceAccessReadEvent("iterator", iter.storeKey, iter.parent.Key(), value) return value } @@ -156,11 +151,9 @@ func (iter *cacheMergeIterator) Value() []byte { switch cmp { case -1: // parent < cache value := iter.parent.Value() - iter.eventManager.EmitResourceAccessReadEvent("iterator", iter.storeKey, keyP, value) return value case 0, 1: // parent >= cache value := iter.cache.Value() - iter.eventManager.EmitResourceAccessReadEvent("iterator", iter.storeKey, keyC, value) return value default: panic("invalid comparison result") diff --git a/store/cachekv/mergeiterator_test.go b/store/cachekv/mergeiterator_test.go index 00f065151..b2648a865 100644 --- a/store/cachekv/mergeiterator_test.go +++ b/store/cachekv/mergeiterator_test.go @@ -6,7 +6,6 @@ import ( "github.com/cosmos/cosmos-sdk/store/cachekv" "github.com/cosmos/cosmos-sdk/store/dbadapter" "github.com/cosmos/cosmos-sdk/store/types" - sdktypes "github.com/cosmos/cosmos-sdk/types" "github.com/stretchr/testify/require" dbm "github.com/tendermint/tm-db" ) @@ -14,7 +13,6 @@ import ( func TestMangerIterator(t *testing.T) { // initiate mock kvstore mem := dbadapter.Store{DB: dbm.NewMemDB()} - eventManager := sdktypes.NewEventManager() kvstore := cachekv.NewStore(mem, types.NewKVStoreKey("CacheKvTest"), types.DefaultCacheSizeLimit) value := randSlice(defaultValueSizeBz) startKey := randSlice(32) @@ -29,27 +27,13 @@ func TestMangerIterator(t *testing.T) { cache := kvstore.Iterator(nil, nil) for ; cache.Valid(); cache.Next() { } - iter := cachekv.NewCacheMergeIterator(parent, cache, true, types.NewKVStoreKey("CacheKvTest"), eventManager) + iter := cachekv.NewCacheMergeIterator(parent, cache, true, types.NewKVStoreKey("CacheKvTest")) - // get the next value - iter.Value() - - // assert the resource access is still emitted correctly when the cache store is unavailable - require.Equal(t, "access_type", string(eventManager.Events()[0].Attributes[0].Key)) - require.Equal(t, "read", string(eventManager.Events()[0].Attributes[0].Value)) - require.Equal(t, "store_key", string(eventManager.Events()[0].Attributes[1].Key)) - require.Equal(t, "CacheKvTest", string(eventManager.Events()[0].Attributes[1].Value)) - - // assert event emission when cache is available - cache = kvstore.Iterator(keys[1], keys[2]) - iter = cachekv.NewCacheMergeIterator(parent, cache, true, types.NewKVStoreKey("CacheKvTest"), eventManager) + // get the next value and it should not be nil + nextValue := iter.Value() + require.NotNil(t, nextValue) // get the next value - iter.Value() - - // assert the resource access is still emitted correctly when the cache store is available - require.Equal(t, "access_type", string(eventManager.Events()[0].Attributes[0].Key)) - require.Equal(t, "read", string(eventManager.Events()[0].Attributes[0].Value)) - require.Equal(t, "store_key", string(eventManager.Events()[0].Attributes[1].Key)) - require.Equal(t, "CacheKvTest", string(eventManager.Events()[0].Attributes[1].Value)) + nextValue = iter.Value() + require.NotNil(t, nextValue) } diff --git a/store/cachekv/store.go b/store/cachekv/store.go index d3c71d373..a33adf17c 100644 --- a/store/cachekv/store.go +++ b/store/cachekv/store.go @@ -56,8 +56,6 @@ func (store *Store) GetEvents() []abci.Event { // Implements Store func (store *Store) ResetEvents() { - store.mtx.Lock() - defer store.mtx.Unlock() store.eventManager = sdktypes.NewEventManager() } @@ -77,7 +75,6 @@ func (store *Store) getFromCache(key []byte) []byte { // Get implements types.KVStore. func (store *Store) Get(key []byte) (value []byte) { types.AssertValidKey(key) - store.eventManager.EmitResourceAccessReadEvent("get", store.storeKey, key, value) return store.getFromCache(key) } @@ -86,13 +83,11 @@ func (store *Store) Set(key []byte, value []byte) { types.AssertValidKey(key) types.AssertValidValue(value) store.setCacheValue(key, value, false, true) - store.eventManager.EmitResourceAccessWriteEvent("set", store.storeKey, key, value) } // Has implements types.KVStore. func (store *Store) Has(key []byte) bool { value := store.Get(key) - store.eventManager.EmitResourceAccessReadEvent("has", store.storeKey, key, value) return value != nil } @@ -194,7 +189,7 @@ func (store *Store) iterator(start, end []byte, ascending bool) types.Iterator { }() store.dirtyItems(start, end) cache = newMemIterator(start, end, store.sortedCache, store.deleted, ascending, store.eventManager, store.storeKey) - return NewCacheMergeIterator(parent, cache, ascending, store.storeKey, store.eventManager) + return NewCacheMergeIterator(parent, cache, ascending, store.storeKey) } func (store *Store) VersionExists(version int64) bool {