From 528b9aaee8facc3188d131af9c1e17f069371436 Mon Sep 17 00:00:00 2001 From: ItsFunny <39111451+ItsFunny@users.noreply.github.com> Date: Thu, 27 Apr 2023 12:12:11 +0800 Subject: [PATCH] add lock to IBC Map (#3101) * add lock * pointer => index * add note * fix ut * ut rerun * rm debug --------- Co-authored-by: ylsGit Co-authored-by: KamiD <44460798+KamiD@users.noreply.github.com> --- libs/cosmos-sdk/x/capability/keeper/keeper.go | 22 ++++++------ .../x/capability/keeper/keeper_test.go | 2 +- libs/cosmos-sdk/x/capability/types/keys.go | 6 +++- .../x/capability/types/keys_test.go | 2 +- .../core/04-channel/keeper/handshake_test.go | 36 +++++++++---------- .../core/04-channel/keeper/packet_test.go | 2 +- 6 files changed, 36 insertions(+), 34 deletions(-) diff --git a/libs/cosmos-sdk/x/capability/keeper/keeper.go b/libs/cosmos-sdk/x/capability/keeper/keeper.go index cbb7cb1f25..c40b015f85 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/okex/exchain/libs/tendermint/types" @@ -39,7 +40,7 @@ type ( cdc *codec.Codec storeKey sdk.StoreKey memKey sdk.StoreKey - capMap map[uint64]*types.Capability + capMap *sync.Map scopedModules map[string]struct{} sealed bool } @@ -54,7 +55,7 @@ type ( cdc *codec.Codec storeKey sdk.StoreKey memKey sdk.StoreKey - capMap map[uint64]*types.Capability + capMap *sync.Map module string } ) @@ -66,7 +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), + capMap: &sync.Map{}, scopedModules: make(map[string]struct{}), sealed: false, } @@ -200,7 +201,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) } } @@ -223,8 +224,6 @@ 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)) } - - // create new capability with the current global index index := types.IndexFromKey(store.Get(types.KeyIndex)) cap := types.NewCapability(index) @@ -249,7 +248,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) @@ -340,7 +339,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)) @@ -395,7 +394,6 @@ func (sk ScopedKeeper) GetCapability(ctx sdk.Context, name string) (*types.Capab //} // //return cap, true - if strings.TrimSpace(name) == "" { return nil, false } @@ -416,12 +414,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 6b09262823..e6ef5b06cb 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 549eef3eb5..c32e82b9f0 100644 --- a/libs/cosmos-sdk/x/capability/types/keys.go +++ b/libs/cosmos-sdk/x/capability/types/keys.go @@ -2,6 +2,7 @@ package types import ( "fmt" + sdk "github.com/okex/exchain/libs/cosmos-sdk/types" ) @@ -37,8 +38,11 @@ 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 concurrently execute ,the capMap maybe override by simulate which will fail +// to create the channel 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 2c0f08185c..74c4f01d92 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 6b7187a44d..40328b22de 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 @@ -41,6 +41,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 @@ -151,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() @@ -216,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 { 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 05932c407f..ddd456e764 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}, }