From 2f8f12b4c0763c4f531511e14949ad99046f8240 Mon Sep 17 00:00:00 2001 From: itsfunny <18757883747@163.com> Date: Thu, 13 Apr 2023 12:29:45 +0800 Subject: [PATCH 1/3] lock when simuate and delivertx concurrently execute --- libs/cosmos-sdk/x/capability/keeper/keeper.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/libs/cosmos-sdk/x/capability/keeper/keeper.go b/libs/cosmos-sdk/x/capability/keeper/keeper.go index c846a39a4..479b059d8 100644 --- a/libs/cosmos-sdk/x/capability/keeper/keeper.go +++ b/libs/cosmos-sdk/x/capability/keeper/keeper.go @@ -3,6 +3,7 @@ package keeper import ( "fmt" "strings" + "sync" tmtypes "github.com/okx/okbchain/libs/tendermint/types" @@ -40,6 +41,7 @@ type ( storeKey sdk.StoreKey memKey sdk.StoreKey capMap map[uint64]*types.Capability + rwLock *sync.RWMutex scopedModules map[string]struct{} sealed bool } @@ -55,6 +57,7 @@ type ( storeKey sdk.StoreKey memKey sdk.StoreKey capMap map[uint64]*types.Capability + rwLock *sync.RWMutex module string } ) @@ -67,6 +70,7 @@ func NewKeeper(cdc *codec.CodecProxy, storeKey, memKey sdk.StoreKey) *Keeper { storeKey: storeKey, memKey: memKey, capMap: make(map[uint64]*types.Capability), + rwLock: &sync.RWMutex{}, scopedModules: make(map[string]struct{}), sealed: false, } @@ -94,6 +98,7 @@ func (k *Keeper) ScopeToModule(moduleName string) ScopedKeeper { storeKey: k.storeKey, memKey: k.memKey, capMap: k.capMap, + rwLock: k.rwLock, module: moduleName, } } @@ -223,7 +228,11 @@ func (sk ScopedKeeper) NewCapability(ctx sdk.Context, name string) (*types.Capab if _, ok := sk.GetCapability(ctx, name); ok { return nil, sdkerrors.Wrapf(types.ErrCapabilityTaken, fmt.Sprintf("module: %s, name: %s", sk.module, name)) } - + // NOTE, FwdCapabilityKey use the pointer address to build key + // which means, when simulate and deliver tx concurrently execute ,the capMap maybe override by simulate which will fail + // to create the channel + sk.rwLock.Lock() + defer sk.rwLock.Unlock() // create new capability with the current global index index := types.IndexFromKey(store.Get(types.KeyIndex)) cap := types.NewCapability(index) @@ -395,6 +404,8 @@ func (sk ScopedKeeper) GetCapability(ctx sdk.Context, name string) (*types.Capab //} // //return cap, true + sk.rwLock.RLock() + defer sk.rwLock.RUnlock() if strings.TrimSpace(name) == "" { return nil, false From cbcb312a81e5679ae987fbcbf2a65602d0290ed0 Mon Sep 17 00:00:00 2001 From: itsfunny <18757883747@163.com> Date: Fri, 12 May 2023 16:19:14 +0800 Subject: [PATCH 2/3] syncmap --- libs/cosmos-sdk/x/capability/keeper/keeper.go | 23 +++++------ .../x/capability/keeper/keeper_test.go | 2 +- libs/cosmos-sdk/x/capability/types/keys.go | 2 +- .../x/capability/types/keys_test.go | 2 +- .../core/04-channel/keeper/handshake_test.go | 38 ++++++++++--------- .../core/04-channel/keeper/packet_test.go | 2 +- 6 files changed, 33 insertions(+), 36 deletions(-) diff --git a/libs/cosmos-sdk/x/capability/keeper/keeper.go b/libs/cosmos-sdk/x/capability/keeper/keeper.go index 479b059d8..16fc70d1d 100644 --- a/libs/cosmos-sdk/x/capability/keeper/keeper.go +++ b/libs/cosmos-sdk/x/capability/keeper/keeper.go @@ -40,8 +40,7 @@ type ( cdc *codec.Codec storeKey sdk.StoreKey memKey sdk.StoreKey - capMap map[uint64]*types.Capability - rwLock *sync.RWMutex + capMap *sync.Map scopedModules map[string]struct{} sealed bool } @@ -56,8 +55,7 @@ type ( cdc *codec.Codec storeKey sdk.StoreKey memKey sdk.StoreKey - capMap map[uint64]*types.Capability - rwLock *sync.RWMutex + capMap *sync.Map module string } ) @@ -69,8 +67,7 @@ func NewKeeper(cdc *codec.CodecProxy, storeKey, memKey sdk.StoreKey) *Keeper { cdc: cdc.GetCdc(), storeKey: storeKey, memKey: memKey, - capMap: make(map[uint64]*types.Capability), - rwLock: &sync.RWMutex{}, + capMap: &sync.Map{}, scopedModules: make(map[string]struct{}), sealed: false, } @@ -205,7 +202,7 @@ func (k Keeper) InitializeCapability(ctx sdk.Context, index uint64, owners types memStore.Set(types.RevCapabilityKey(owner.Module, owner.Name), sdk.Uint64ToBigEndian(index)) // Set the mapping from index from index to in-memory capability in the go map - k.capMap[index] = cap + k.capMap.Store(index, cap) } } @@ -258,7 +255,7 @@ func (sk ScopedKeeper) NewCapability(ctx sdk.Context, name string) (*types.Capab memStore.Set(types.RevCapabilityKey(sk.module, name), sdk.Uint64ToBigEndian(index)) // Set the mapping from index from index to in-memory capability in the go map - sk.capMap[index] = cap + sk.capMap.Store(index, cap) logger(ctx).Info("created new capability", "module", sk.module, "name", name) @@ -349,7 +346,7 @@ func (sk ScopedKeeper) ReleaseCapability(ctx sdk.Context, cap *types.Capability) // remove capability owner set prefixStore.Delete(indexKey) // since no one owns capability, we can delete capability from map - delete(sk.capMap, cap.GetIndex()) + sk.capMap.Delete(cap.GetIndex()) } else { // update capability owner set prefixStore.Set(indexKey, sk.cdc.MustMarshalBinaryBare(capOwners)) @@ -404,8 +401,6 @@ func (sk ScopedKeeper) GetCapability(ctx sdk.Context, name string) (*types.Capab //} // //return cap, true - sk.rwLock.RLock() - defer sk.rwLock.RUnlock() if strings.TrimSpace(name) == "" { return nil, false @@ -427,12 +422,12 @@ func (sk ScopedKeeper) GetCapability(ctx sdk.Context, name string) (*types.Capab return nil, false } - cap := sk.capMap[index] - if cap == nil { + cap, ok := sk.capMap.Load(index) + if !ok { panic("capability found in memstore is missing from map") } - return cap, true + return cap.(*types.Capability), true } // GetCapabilityName allows a module to retrieve the name under which it stored a given diff --git a/libs/cosmos-sdk/x/capability/keeper/keeper_test.go b/libs/cosmos-sdk/x/capability/keeper/keeper_test.go index 059769f03..14f47eac0 100644 --- a/libs/cosmos-sdk/x/capability/keeper/keeper_test.go +++ b/libs/cosmos-sdk/x/capability/keeper/keeper_test.go @@ -111,7 +111,7 @@ func (suite *KeeperTestSuite) TestAuthenticateCapability() { suite.Require().NoError(err) suite.Require().NotNil(cap1) - forgedCap := types.NewCapability(cap1.Index) // index should be the same index as the first capability + forgedCap := types.NewCapability(cap1.Index + 1) // index should be the same index as the first capability suite.Require().False(sk1.AuthenticateCapability(suite.ctx, forgedCap, "transfer")) suite.Require().False(sk2.AuthenticateCapability(suite.ctx, forgedCap, "transfer")) diff --git a/libs/cosmos-sdk/x/capability/types/keys.go b/libs/cosmos-sdk/x/capability/types/keys.go index c05d1edfa..f084eb0f8 100644 --- a/libs/cosmos-sdk/x/capability/types/keys.go +++ b/libs/cosmos-sdk/x/capability/types/keys.go @@ -38,7 +38,7 @@ func RevCapabilityKey(module, name string) []byte { // FwdCapabilityKey returns a forward lookup key for a given module and capability // reference. func FwdCapabilityKey(module string, cap *Capability) []byte { - return []byte(fmt.Sprintf("%s/fwd/%p", module, cap)) + return []byte(fmt.Sprintf("%s/fwd/%d", module, cap.Index)) } // IndexToKey returns bytes to be used as a key for a given capability index. diff --git a/libs/cosmos-sdk/x/capability/types/keys_test.go b/libs/cosmos-sdk/x/capability/types/keys_test.go index 3a3643b5a..4f1d27b2d 100644 --- a/libs/cosmos-sdk/x/capability/types/keys_test.go +++ b/libs/cosmos-sdk/x/capability/types/keys_test.go @@ -16,7 +16,7 @@ func TestRevCapabilityKey(t *testing.T) { func TestFwdCapabilityKey(t *testing.T) { cap := types.NewCapability(23) - expected := []byte(fmt.Sprintf("bank/fwd/%p", cap)) + expected := []byte(fmt.Sprintf("bank/fwd/%d", cap.Index)) require.Equal(t, expected, types.FwdCapabilityKey("bank", cap)) } diff --git a/libs/ibc-go/modules/core/04-channel/keeper/handshake_test.go b/libs/ibc-go/modules/core/04-channel/keeper/handshake_test.go index 8dbbbccf8..356c1cb01 100644 --- a/libs/ibc-go/modules/core/04-channel/keeper/handshake_test.go +++ b/libs/ibc-go/modules/core/04-channel/keeper/handshake_test.go @@ -33,6 +33,24 @@ func (suite *KeeperTestSuite) TestChanOpenInit() { ) testCases := []testCase{ + {"connection does not support ORDERED channels", func() { + suite.coordinator.SetupConnections(path) + + // modify connA versions to only support UNORDERED channels + conn := path.EndpointA.GetConnection() + + version := connectiontypes.NewVersion("1", []string{"ORDER_UNORDERED"}) + conn.Versions = []*connectiontypes.Version{version} + + suite.chainA.App().GetIBCKeeper().ConnectionKeeper.SetConnection( + suite.chainA.GetContext(), + path.EndpointA.ConnectionID, conn, + ) + // NOTE: Opening UNORDERED channels is still expected to pass but ORDERED channels should fail + features = []string{"ORDER_UNORDERED"} + suite.chainA.CreatePortCapability(suite.chainA.GetSimApp().ScopedIBCMockKeeper, ibctesting.MockPort) + portCap = suite.chainA.GetPortCapability(ibctesting.MockPort) + }, true}, {"success", func() { suite.coordinator.SetupConnections(path) features = []string{"ORDER_ORDERED", "ORDER_UNORDERED"} @@ -41,6 +59,8 @@ func (suite *KeeperTestSuite) TestChanOpenInit() { }, true}, {"channel already exists", func() { suite.coordinator.Setup(path) + // we refactor the `FwdCapabilityKey`,so we have to change the index + portCap.Index = 100 }, false}, {"connection doesn't exist", func() { // any non-empty values @@ -69,24 +89,6 @@ func (suite *KeeperTestSuite) TestChanOpenInit() { suite.chainA.CreatePortCapability(suite.chainA.GetSimApp().ScopedIBCMockKeeper, ibctesting.MockPort) portCap = suite.chainA.GetPortCapability(ibctesting.MockPort) }, false}, - {"connection does not support ORDERED channels", func() { - suite.coordinator.SetupConnections(path) - - // modify connA versions to only support UNORDERED channels - conn := path.EndpointA.GetConnection() - - version := connectiontypes.NewVersion("1", []string{"ORDER_UNORDERED"}) - conn.Versions = []*connectiontypes.Version{version} - - suite.chainA.App().GetIBCKeeper().ConnectionKeeper.SetConnection( - suite.chainA.GetContext(), - path.EndpointA.ConnectionID, conn, - ) - // NOTE: Opening UNORDERED channels is still expected to pass but ORDERED channels should fail - features = []string{"ORDER_UNORDERED"} - suite.chainA.CreatePortCapability(suite.chainA.GetSimApp().ScopedIBCMockKeeper, ibctesting.MockPort) - portCap = suite.chainA.GetPortCapability(ibctesting.MockPort) - }, true}, } for _, tc := range testCases { diff --git a/libs/ibc-go/modules/core/04-channel/keeper/packet_test.go b/libs/ibc-go/modules/core/04-channel/keeper/packet_test.go index 14abdb18f..23556e173 100644 --- a/libs/ibc-go/modules/core/04-channel/keeper/packet_test.go +++ b/libs/ibc-go/modules/core/04-channel/keeper/packet_test.go @@ -199,7 +199,7 @@ func (suite *KeeperTestSuite) TestSendPacket() { {"channel capability not found", func() { suite.coordinator.Setup(path) packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp) - channelCap = capabilitytypes.NewCapability(5) + channelCap = capabilitytypes.NewCapability(10) }, false}, } From ddab591565e62ac675de834ebab4709635696bb0 Mon Sep 17 00:00:00 2001 From: itsfunny <18757883747@163.com> Date: Fri, 12 May 2023 16:26:48 +0800 Subject: [PATCH 3/3] compile --- libs/cosmos-sdk/x/capability/keeper/keeper.go | 8 +---- libs/cosmos-sdk/x/capability/types/keys.go | 4 +++ .../core/04-channel/keeper/handshake_test.go | 34 +++++++++---------- 3 files changed, 21 insertions(+), 25 deletions(-) diff --git a/libs/cosmos-sdk/x/capability/keeper/keeper.go b/libs/cosmos-sdk/x/capability/keeper/keeper.go index 16fc70d1d..588a41430 100644 --- a/libs/cosmos-sdk/x/capability/keeper/keeper.go +++ b/libs/cosmos-sdk/x/capability/keeper/keeper.go @@ -95,7 +95,6 @@ func (k *Keeper) ScopeToModule(moduleName string) ScopedKeeper { storeKey: k.storeKey, memKey: k.memKey, capMap: k.capMap, - rwLock: k.rwLock, module: moduleName, } } @@ -225,12 +224,7 @@ func (sk ScopedKeeper) NewCapability(ctx sdk.Context, name string) (*types.Capab if _, ok := sk.GetCapability(ctx, name); ok { return nil, sdkerrors.Wrapf(types.ErrCapabilityTaken, fmt.Sprintf("module: %s, name: %s", sk.module, name)) } - // NOTE, FwdCapabilityKey use the pointer address to build key - // which means, when simulate and deliver tx concurrently execute ,the capMap maybe override by simulate which will fail - // to create the channel - sk.rwLock.Lock() - defer sk.rwLock.Unlock() - // create new capability with the current global index + index := types.IndexFromKey(store.Get(types.KeyIndex)) cap := types.NewCapability(index) diff --git a/libs/cosmos-sdk/x/capability/types/keys.go b/libs/cosmos-sdk/x/capability/types/keys.go index f084eb0f8..3b50c40a2 100644 --- a/libs/cosmos-sdk/x/capability/types/keys.go +++ b/libs/cosmos-sdk/x/capability/types/keys.go @@ -37,6 +37,10 @@ func RevCapabilityKey(module, name string) []byte { // FwdCapabilityKey returns a forward lookup key for a given module and capability // reference. +// NOTE, FwdCapabilityKey use the pointer address to build key +// which means, when simulate and deliver tx concurrently execute ,the capMap maybe override by simulate which will fail +// to create the channel +// create new capability with the current global index func FwdCapabilityKey(module string, cap *Capability) []byte { return []byte(fmt.Sprintf("%s/fwd/%d", module, cap.Index)) } diff --git a/libs/ibc-go/modules/core/04-channel/keeper/handshake_test.go b/libs/ibc-go/modules/core/04-channel/keeper/handshake_test.go index 356c1cb01..4a5fb9fa7 100644 --- a/libs/ibc-go/modules/core/04-channel/keeper/handshake_test.go +++ b/libs/ibc-go/modules/core/04-channel/keeper/handshake_test.go @@ -153,6 +153,22 @@ func (suite *KeeperTestSuite) TestChanOpenTry() { ) testCases := []testCase{ + {"connection does not support ORDERED channels", func() { + suite.coordinator.SetupConnections(path) + path.SetChannelOrdered() + path.EndpointA.ChanOpenInit() + + // modify connA versions to only support UNORDERED channels + conn := path.EndpointA.GetConnection() + + version := connectiontypes.NewVersion("1", []string{"ORDER_UNORDERED"}) + conn.Versions = []*connectiontypes.Version{version} + + suite.chainA.GetSimApp().GetIBCKeeper().ConnectionKeeper.SetConnection( + suite.chainA.GetContext(), + path.EndpointA.ConnectionID, conn, + ) + }, false}, {"success", func() { suite.coordinator.SetupConnections(path) path.SetChannelOrdered() @@ -218,24 +234,6 @@ func (suite *KeeperTestSuite) TestChanOpenTry() { suite.chainB.CreatePortCapability(suite.chainB.GetSimApp().ScopedIBCMockKeeper, ibctesting.MockPort) portCap = suite.chainB.GetPortCapability(ibctesting.MockPort) }, false}, - {"connection does not support ORDERED channels", func() { - suite.coordinator.SetupConnections(path) - path.SetChannelOrdered() - path.EndpointA.ChanOpenInit() - - // modify connA versions to only support UNORDERED channels - conn := path.EndpointA.GetConnection() - - version := connectiontypes.NewVersion("1", []string{"ORDER_UNORDERED"}) - conn.Versions = []*connectiontypes.Version{version} - - suite.chainA.GetSimApp().GetIBCKeeper().ConnectionKeeper.SetConnection( - suite.chainA.GetContext(), - path.EndpointA.ConnectionID, conn, - ) - suite.chainA.CreatePortCapability(suite.chainA.GetSimApp().ScopedIBCMockKeeper, ibctesting.MockPort) - portCap = suite.chainA.GetPortCapability(ibctesting.MockPort) - }, false}, } for _, tc := range testCases {