From 764d30f8ec51ca2f0873332cb2ec0d4a4cb8ca8a Mon Sep 17 00:00:00 2001 From: Facundo Date: Thu, 25 Jul 2024 16:25:05 +0200 Subject: [PATCH 1/9] fix: concurrent map writes when calling GetSigners --- simapp/app.go | 5 +++++ x/tx/signing/context.go | 8 +++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/simapp/app.go b/simapp/app.go index df3e524922d6..753422ba21d7 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -603,6 +603,11 @@ func NewSimApp( } } + // validate SigningContext to pre-fill internal signers funcs map + if err = app.interfaceRegistry.SigningContext().Validate(); err != nil { + panic(err) + } + return app } diff --git a/x/tx/signing/context.go b/x/tx/signing/context.go index 7aca6fd2b1cd..386e39399e58 100644 --- a/x/tx/signing/context.go +++ b/x/tx/signing/context.go @@ -3,6 +3,7 @@ package signing import ( "errors" "fmt" + "sync" cosmos_proto "github.com/cosmos/cosmos-proto" gogoproto "github.com/cosmos/gogoproto/proto" @@ -29,9 +30,11 @@ type Context struct { typeResolver protoregistry.MessageTypeResolver addressCodec address.Codec validatorAddressCodec address.Codec - getSignersFuncs map[protoreflect.FullName]GetSignersFunc customGetSignerFuncs map[protoreflect.FullName]GetSignersFunc maxRecursionDepth int + + mtx sync.Mutex + getSignersFuncs map[protoreflect.FullName]GetSignersFunc } // Options are options for creating Context which will be used for signing operations. @@ -110,6 +113,7 @@ func NewContext(options Options) (*Context, error) { typeResolver: protoTypes, addressCodec: options.AddressCodec, validatorAddressCodec: options.ValidatorAddressCodec, + mtx: sync.Mutex{}, getSignersFuncs: map[protoreflect.FullName]GetSignersFunc{}, customGetSignerFuncs: customGetSignerFuncs, maxRecursionDepth: options.MaxRecursionDepth, @@ -336,6 +340,8 @@ func (c *Context) getGetSignersFn(messageDescriptor protoreflect.MessageDescript } f, ok = c.getSignersFuncs[messageDescriptor.FullName()] if !ok { + c.mtx.Lock() + defer c.mtx.Unlock() var err error f, err = c.makeGetSignersFunc(messageDescriptor) if err != nil { From 33348e777a0d87091e75dea73869042971495af2 Mon Sep 17 00:00:00 2001 From: Facundo Date: Thu, 25 Jul 2024 16:34:24 +0200 Subject: [PATCH 2/9] cl++ --- x/tx/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/x/tx/CHANGELOG.md b/x/tx/CHANGELOG.md index af095740513d..d83545c56088 100644 --- a/x/tx/CHANGELOG.md +++ b/x/tx/CHANGELOG.md @@ -33,6 +33,10 @@ Since v0.13.0, x/tx follows Cosmos SDK semver: https://github.com/cosmos/cosmos- ## [Unreleased] +### Improvements + +* [#21073](https://github.com/cosmos/cosmos-sdk/pull/21073) Add write-only mutex in Context to protect `getSignersFuncs` map from concurrent writes. + ## [v0.13.3](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v0.13.3) - 2024-04-22 ### Improvements From 168407a96eefacfba13c8e65c9d3c2dbf998a49e Mon Sep 17 00:00:00 2001 From: Facundo Date: Mon, 29 Jul 2024 10:16:10 +0200 Subject: [PATCH 3/9] another try --- simapp/app.go | 5 ----- x/tx/signing/context.go | 19 ++++++------------- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/simapp/app.go b/simapp/app.go index 753422ba21d7..df3e524922d6 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -603,11 +603,6 @@ func NewSimApp( } } - // validate SigningContext to pre-fill internal signers funcs map - if err = app.interfaceRegistry.SigningContext().Validate(); err != nil { - panic(err) - } - return app } diff --git a/x/tx/signing/context.go b/x/tx/signing/context.go index 386e39399e58..747b110787ec 100644 --- a/x/tx/signing/context.go +++ b/x/tx/signing/context.go @@ -3,7 +3,6 @@ package signing import ( "errors" "fmt" - "sync" cosmos_proto "github.com/cosmos/cosmos-proto" gogoproto "github.com/cosmos/gogoproto/proto" @@ -30,11 +29,9 @@ type Context struct { typeResolver protoregistry.MessageTypeResolver addressCodec address.Codec validatorAddressCodec address.Codec + getSignersFuncs map[protoreflect.FullName]GetSignersFunc customGetSignerFuncs map[protoreflect.FullName]GetSignersFunc maxRecursionDepth int - - mtx sync.Mutex - getSignersFuncs map[protoreflect.FullName]GetSignersFunc } // Options are options for creating Context which will be used for signing operations. @@ -113,12 +110,15 @@ func NewContext(options Options) (*Context, error) { typeResolver: protoTypes, addressCodec: options.AddressCodec, validatorAddressCodec: options.ValidatorAddressCodec, - mtx: sync.Mutex{}, getSignersFuncs: map[protoreflect.FullName]GetSignersFunc{}, customGetSignerFuncs: customGetSignerFuncs, maxRecursionDepth: options.MaxRecursionDepth, } + if err := c.Validate(); err != nil { + return nil, err + } + return c, nil } @@ -340,14 +340,7 @@ func (c *Context) getGetSignersFn(messageDescriptor protoreflect.MessageDescript } f, ok = c.getSignersFuncs[messageDescriptor.FullName()] if !ok { - c.mtx.Lock() - defer c.mtx.Unlock() - var err error - f, err = c.makeGetSignersFunc(messageDescriptor) - if err != nil { - return nil, err - } - c.getSignersFuncs[messageDescriptor.FullName()] = f + return nil, fmt.Errorf("no GetSignersFunc found for message %s; have you called Validate()?", messageDescriptor.FullName()) } return f, nil From 8f8322d1393fe70d3b7844b9df4f4e9e4f73c326 Mon Sep 17 00:00:00 2001 From: Facundo Date: Mon, 29 Jul 2024 10:23:48 +0200 Subject: [PATCH 4/9] progress --- x/tx/signing/context.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/x/tx/signing/context.go b/x/tx/signing/context.go index 747b110787ec..d743a08a2891 100644 --- a/x/tx/signing/context.go +++ b/x/tx/signing/context.go @@ -165,7 +165,7 @@ func (c *Context) Validate() error { errs = append(errs, fmt.Errorf("a custom signer function as been defined for message %s which already has a signer field defined with (cosmos.msg.v1.signer)", md.FullName())) continue } - _, err := c.getGetSignersFn(md) + _, err := c.getGetSignersFn(md, true) if err != nil { errs = append(errs, err) } @@ -333,14 +333,23 @@ func (c *Context) getAddressCodec(field protoreflect.FieldDescriptor) address.Co return addrCdc } -func (c *Context) getGetSignersFn(messageDescriptor protoreflect.MessageDescriptor) (GetSignersFunc, error) { +func (c *Context) getGetSignersFn(messageDescriptor protoreflect.MessageDescriptor, add bool) (GetSignersFunc, error) { f, ok := c.customGetSignerFuncs[messageDescriptor.FullName()] if ok { return f, nil } f, ok = c.getSignersFuncs[messageDescriptor.FullName()] if !ok { - return nil, fmt.Errorf("no GetSignersFunc found for message %s; have you called Validate()?", messageDescriptor.FullName()) + if !add { + return nil, fmt.Errorf("no GetSignersFunc found for message %s; have you called Validate()?", messageDescriptor.FullName()) + } + + var err error + f, err = c.makeGetSignersFunc(messageDescriptor) + if err != nil { + return nil, err + } + c.getSignersFuncs[messageDescriptor.FullName()] = f } return f, nil @@ -348,7 +357,7 @@ func (c *Context) getGetSignersFn(messageDescriptor protoreflect.MessageDescript // GetSigners returns the signers for a given message. func (c *Context) GetSigners(msg proto.Message) ([][]byte, error) { - f, err := c.getGetSignersFn(msg.ProtoReflect().Descriptor()) + f, err := c.getGetSignersFn(msg.ProtoReflect().Descriptor(), false) if err != nil { return nil, err } From 4f3109807905414a6d4b049d9087646afb48f5ce Mon Sep 17 00:00:00 2001 From: Facundo Date: Mon, 29 Jul 2024 15:58:10 +0200 Subject: [PATCH 5/9] test --- x/tx/signing/context.go | 10 +++------- x/tx/signing/context_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/x/tx/signing/context.go b/x/tx/signing/context.go index d743a08a2891..b13a51b0b0b1 100644 --- a/x/tx/signing/context.go +++ b/x/tx/signing/context.go @@ -165,7 +165,7 @@ func (c *Context) Validate() error { errs = append(errs, fmt.Errorf("a custom signer function as been defined for message %s which already has a signer field defined with (cosmos.msg.v1.signer)", md.FullName())) continue } - _, err := c.getGetSignersFn(md, true) + _, err := c.getGetSignersFn(md) if err != nil { errs = append(errs, err) } @@ -333,17 +333,13 @@ func (c *Context) getAddressCodec(field protoreflect.FieldDescriptor) address.Co return addrCdc } -func (c *Context) getGetSignersFn(messageDescriptor protoreflect.MessageDescriptor, add bool) (GetSignersFunc, error) { +func (c *Context) getGetSignersFn(messageDescriptor protoreflect.MessageDescriptor) (GetSignersFunc, error) { f, ok := c.customGetSignerFuncs[messageDescriptor.FullName()] if ok { return f, nil } f, ok = c.getSignersFuncs[messageDescriptor.FullName()] if !ok { - if !add { - return nil, fmt.Errorf("no GetSignersFunc found for message %s; have you called Validate()?", messageDescriptor.FullName()) - } - var err error f, err = c.makeGetSignersFunc(messageDescriptor) if err != nil { @@ -357,7 +353,7 @@ func (c *Context) getGetSignersFn(messageDescriptor protoreflect.MessageDescript // GetSigners returns the signers for a given message. func (c *Context) GetSigners(msg proto.Message) ([][]byte, error) { - f, err := c.getGetSignersFn(msg.ProtoReflect().Descriptor(), false) + f, err := c.getGetSignersFn(msg.ProtoReflect().Descriptor()) if err != nil { return nil, err } diff --git a/x/tx/signing/context_test.go b/x/tx/signing/context_test.go index d8c2b370333a..f64c0978c2ae 100644 --- a/x/tx/signing/context_test.go +++ b/x/tx/signing/context_test.go @@ -51,6 +51,33 @@ var deeplyNestedRepeatedSigner = &testpb.DeeplyNestedRepeatedSigner{ }, } +func TestGetGetSignersFnConcurrent(t *testing.T) { + ctx, err := NewContext(Options{ + AddressCodec: dummyAddressCodec{}, + ValidatorAddressCodec: dummyValidatorAddressCodec{}, + }) + require.NoError(t, err) + + msg := &bankv1beta1.MsgSend{ + FromAddress: hex.EncodeToString([]byte("foo")), + } + + desc := msg.ProtoReflect().Descriptor() + + // err = ctx.Validate() + // require.NoError(t, err) + + for i := 0; i < 100; i++ { + go func() { + _, _ = ctx.getGetSignersFn(desc) + }() + } + + // signers, err := fn(msg) + // require.NoError(t, err) + // require.Equal(t, [][]byte{[]byte("foo")}, signers) +} + func TestGetSigners(t *testing.T) { ctx, err := NewContext(Options{ AddressCodec: dummyAddressCodec{}, From 848b15f0eb17f70416b743da536b598ee09d2960 Mon Sep 17 00:00:00 2001 From: Facundo Date: Tue, 30 Jul 2024 13:20:55 +0200 Subject: [PATCH 6/9] use sync.Map :( --- x/tx/signing/context.go | 12 ++++++++---- x/tx/signing/context_test.go | 21 ++++----------------- 2 files changed, 12 insertions(+), 21 deletions(-) diff --git a/x/tx/signing/context.go b/x/tx/signing/context.go index b13a51b0b0b1..b1a0d4ed2e42 100644 --- a/x/tx/signing/context.go +++ b/x/tx/signing/context.go @@ -3,6 +3,7 @@ package signing import ( "errors" "fmt" + "sync" cosmos_proto "github.com/cosmos/cosmos-proto" gogoproto "github.com/cosmos/gogoproto/proto" @@ -29,7 +30,7 @@ type Context struct { typeResolver protoregistry.MessageTypeResolver addressCodec address.Codec validatorAddressCodec address.Codec - getSignersFuncs map[protoreflect.FullName]GetSignersFunc + getSignersFuncs sync.Map customGetSignerFuncs map[protoreflect.FullName]GetSignersFunc maxRecursionDepth int } @@ -110,7 +111,7 @@ func NewContext(options Options) (*Context, error) { typeResolver: protoTypes, addressCodec: options.AddressCodec, validatorAddressCodec: options.ValidatorAddressCodec, - getSignersFuncs: map[protoreflect.FullName]GetSignersFunc{}, + getSignersFuncs: sync.Map{}, customGetSignerFuncs: customGetSignerFuncs, maxRecursionDepth: options.MaxRecursionDepth, } @@ -338,14 +339,17 @@ func (c *Context) getGetSignersFn(messageDescriptor protoreflect.MessageDescript if ok { return f, nil } - f, ok = c.getSignersFuncs[messageDescriptor.FullName()] + + loadedFn, ok := c.getSignersFuncs.Load(messageDescriptor.FullName()) if !ok { var err error f, err = c.makeGetSignersFunc(messageDescriptor) if err != nil { return nil, err } - c.getSignersFuncs[messageDescriptor.FullName()] = f + c.getSignersFuncs.Store(messageDescriptor.FullName(), f) + } else { + f = loadedFn.(GetSignersFunc) } return f, nil diff --git a/x/tx/signing/context_test.go b/x/tx/signing/context_test.go index f64c0978c2ae..9367fc561742 100644 --- a/x/tx/signing/context_test.go +++ b/x/tx/signing/context_test.go @@ -58,24 +58,12 @@ func TestGetGetSignersFnConcurrent(t *testing.T) { }) require.NoError(t, err) - msg := &bankv1beta1.MsgSend{ - FromAddress: hex.EncodeToString([]byte("foo")), - } - - desc := msg.ProtoReflect().Descriptor() - - // err = ctx.Validate() - // require.NoError(t, err) - - for i := 0; i < 100; i++ { + desc := (&testpb.RepeatedSigner{}).ProtoReflect().Descriptor() + for i := 0; i < 50; i++ { go func() { _, _ = ctx.getGetSignersFn(desc) }() } - - // signers, err := fn(msg) - // require.NoError(t, err) - // require.Equal(t, [][]byte{[]byte("foo")}, signers) } func TestGetSigners(t *testing.T) { @@ -277,9 +265,8 @@ func TestDefineCustomGetSigners(t *testing.T) { options.DefineCustomGetSigners(proto.MessageName(simpleSigner), func(msg proto.Message) ([][]byte, error) { return [][]byte{[]byte("qux")}, nil }) - context, err = NewContext(options) - require.NoError(t, err) - require.ErrorContains(t, context.Validate(), "a custom signer function as been defined for message SimpleSigner") + _, err = NewContext(options) + require.ErrorContains(t, err, "a custom signer function as been defined for message SimpleSigner") } type dummyAddressCodec struct{} From c07798856b1986c5829ac320a3d52ca48a1b465c Mon Sep 17 00:00:00 2001 From: Facundo Date: Tue, 30 Jul 2024 13:22:39 +0200 Subject: [PATCH 7/9] cl --- x/tx/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/tx/CHANGELOG.md b/x/tx/CHANGELOG.md index d83545c56088..4a13d399cab3 100644 --- a/x/tx/CHANGELOG.md +++ b/x/tx/CHANGELOG.md @@ -35,7 +35,7 @@ Since v0.13.0, x/tx follows Cosmos SDK semver: https://github.com/cosmos/cosmos- ### Improvements -* [#21073](https://github.com/cosmos/cosmos-sdk/pull/21073) Add write-only mutex in Context to protect `getSignersFuncs` map from concurrent writes. +* [#21073](https://github.com/cosmos/cosmos-sdk/pull/21073) In Context use sync.Map `getSignersFuncs` map from concurrent writes, we also call Validate when creating the Context. ## [v0.13.3](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v0.13.3) - 2024-04-22 From 313f9ebab36f97429de7e95bd72aab9ad71a1b0d Mon Sep 17 00:00:00 2001 From: Facundo Date: Wed, 31 Jul 2024 13:36:02 +0200 Subject: [PATCH 8/9] do not do validate on init --- simapp/app.go | 4 ++++ x/tx/signing/context.go | 4 ---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/simapp/app.go b/simapp/app.go index df3e524922d6..cf809cf762b0 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -213,6 +213,10 @@ func NewSimApp( signingCtx := interfaceRegistry.SigningContext() txConfig := authtx.NewTxConfig(appCodec, signingCtx.AddressCodec(), signingCtx.ValidatorAddressCodec(), authtx.DefaultSignModes) + if err := signingCtx.Validate(); err != nil { + panic(err) + } + std.RegisterLegacyAminoCodec(legacyAmino) std.RegisterInterfaces(interfaceRegistry) diff --git a/x/tx/signing/context.go b/x/tx/signing/context.go index b1a0d4ed2e42..f44be0118cca 100644 --- a/x/tx/signing/context.go +++ b/x/tx/signing/context.go @@ -116,10 +116,6 @@ func NewContext(options Options) (*Context, error) { maxRecursionDepth: options.MaxRecursionDepth, } - if err := c.Validate(); err != nil { - return nil, err - } - return c, nil } From a042f819fd8b759dc7180762962b87607c8b4831 Mon Sep 17 00:00:00 2001 From: Facundo Date: Wed, 31 Jul 2024 16:16:24 +0200 Subject: [PATCH 9/9] do not do validate on init --- x/tx/signing/context_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/x/tx/signing/context_test.go b/x/tx/signing/context_test.go index 9367fc561742..88f0bf4aadf9 100644 --- a/x/tx/signing/context_test.go +++ b/x/tx/signing/context_test.go @@ -265,8 +265,9 @@ func TestDefineCustomGetSigners(t *testing.T) { options.DefineCustomGetSigners(proto.MessageName(simpleSigner), func(msg proto.Message) ([][]byte, error) { return [][]byte{[]byte("qux")}, nil }) - _, err = NewContext(options) - require.ErrorContains(t, err, "a custom signer function as been defined for message SimpleSigner") + context, err = NewContext(options) + require.NoError(t, err) + require.ErrorContains(t, context.Validate(), "a custom signer function as been defined for message SimpleSigner") } type dummyAddressCodec struct{}