From 30e3aaab4ab6f1c2cac8ada81dc21064bb0958cb Mon Sep 17 00:00:00 2001 From: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Date: Wed, 31 Jul 2024 17:15:17 +0200 Subject: [PATCH 1/3] fix(x/tx): concurrent map writes when calling GetSigners (#21073) (cherry picked from commit de0708b40171225d00fc3fe59717efd3280c20c2) # Conflicts: # x/tx/CHANGELOG.md # x/tx/signing/context_test.go --- simapp/app.go | 4 ++ x/tx/CHANGELOG.md | 84 ++++++++++++++++++++++++++++++++++++ x/tx/signing/context.go | 12 ++++-- x/tx/signing/context_test.go | 55 +++++++++++++++++++++++ 4 files changed, 151 insertions(+), 4 deletions(-) diff --git a/simapp/app.go b/simapp/app.go index 13db5d04354..068dd9a2e95 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -209,6 +209,10 @@ func NewSimApp( legacyAmino := codec.NewLegacyAmino() txConfig := tx.NewTxConfig(appCodec, tx.DefaultSignModes) + if err := signingCtx.Validate(); err != nil { + panic(err) + } + std.RegisterLegacyAminoCodec(legacyAmino) std.RegisterInterfaces(interfaceRegistry) diff --git a/x/tx/CHANGELOG.md b/x/tx/CHANGELOG.md index d26a47f4b6b..35838dff85e 100644 --- a/x/tx/CHANGELOG.md +++ b/x/tx/CHANGELOG.md @@ -31,6 +31,90 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [Unreleased] +<<<<<<< HEAD +======= +### Improvements + +* [#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 + +### Improvements + +* [#20049](https://github.com/cosmos/cosmos-sdk/pull/20049) Sort JSON attributes for `inline_json` encoder. + +## [v0.13.2](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v0.13.2) - 2024-04-12 + +### Features + +* [#19786](https://github.com/cosmos/cosmos-sdk/pull/19786)/[#19919](https://github.com/cosmos/cosmos-sdk/pull/19919) Add "inline_json" option to Amino JSON encoder. + +### Improvements + +* [#19845](https://github.com/cosmos/cosmos-sdk/pull/19845) Use hybrid resolver instead of only protov2 registry + +### Bug Fixes + +* [#19955](https://github.com/cosmos/cosmos-sdk/pull/19955) Don't shadow Amino marshalling error messages + +## [v0.13.1](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v0.13.1) - 2024-03-05 + +### Features + +* [#19618](https://github.com/cosmos/cosmos-sdk/pull/19618) Add enum as string option to encoder. + +### Improvements + +* [#18857](https://github.com/cosmos/cosmos-sdk/pull/18857) Moved `FormatCoins` from `core/coins` to this package under `signing/textual`. + +### Bug Fixes + +* [#19265](https://github.com/cosmos/cosmos-sdk/pull/19265) Reject denoms that contain a comma. + +## [v0.13.0](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v0.13.0) - 2023-12-19 + +### Improvements + +* [#18740](https://github.com/cosmos/cosmos-sdk/pull/18740) Support nested messages when fetching signers up to a default depth of 32. + +## v0.12.0 + +### Improvements + +* [#18309](https://github.com/cosmos/cosmos-sdk/pull/18309) Update encoder so that amino types default to msg type url. + +## v0.11.0 + +### Improvements + +* [#17787](https://github.com/cosmos/cosmos-sdk/pull/17787) Drop tip support. + +## v0.10.0 + +### Features + +* [#17681](https://github.com/cosmos/cosmos-sdk/pull/17681) Add encoder `DefineTypeEncoding` method for defining custom type encodings. +* [#17600](https://github.com/cosmos/cosmos-sdk/pull/17600) Add encoder `DefineScalarEncoding` method for defining custom scalar encodings. +* [#17600](https://github.com/cosmos/cosmos-sdk/pull/17600) Add indent option to encoder. + +## v0.9.1 + +### Improvements + +* [#16936](https://github.com/cosmos/cosmos-sdk/pull/16936) Remove extra whitespace when marshalling module accounts. + +## v0.9.0 + +### Bug Fixes + +* [#16681](https://github.com/cosmos/cosmos-sdk/pull/16681): Catch and fix `(*Decoder).Decode` crash from invalid length prefix in Tx bytes. + +### Improvements + +* [#16846](https://github.com/cosmos/cosmos-sdk/pull/16846): Harmonize interface `signing.TypeResolver` with the rest of the codebase (orm and client/v2). +* [#16684](https://github.com/cosmos/cosmos-sdk/pull/16684): Use `io.WriteString`+`fmt.Fprintf` to remove unnecessary `string`->`[]byte` roundtrip. + +>>>>>>> de0708b40 (fix(x/tx): concurrent map writes when calling GetSigners (#21073)) ## v0.8.0 ### Improvements diff --git a/x/tx/signing/context.go b/x/tx/signing/context.go index c4e99bdf200..535838fe9cb 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" "google.golang.org/protobuf/proto" @@ -23,7 +24,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 } @@ -95,7 +96,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, } @@ -323,14 +324,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 0be18c98914..e780f1f3dfd 100644 --- a/x/tx/signing/context_test.go +++ b/x/tx/signing/context_test.go @@ -14,6 +14,61 @@ import ( "cosmossdk.io/x/tx/internal/testpb" ) +<<<<<<< HEAD +======= +var deeplyNestedRepeatedSigner = &testpb.DeeplyNestedRepeatedSigner{ + Inner: []*testpb.DeeplyNestedRepeatedSigner_Inner{ + { + Inner: []*testpb.DeeplyNestedRepeatedSigner_Inner_Inner{ + { + Inner: []*testpb.DeeplyNestedRepeatedSigner_Inner_Inner_Bottom{ + { + Signer: []string{hex.EncodeToString([]byte("foo")), hex.EncodeToString([]byte("bar"))}, + }, + }, + }, + }, + }, + { + Inner: []*testpb.DeeplyNestedRepeatedSigner_Inner_Inner{ + { + Inner: []*testpb.DeeplyNestedRepeatedSigner_Inner_Inner_Bottom{ + { + Signer: []string{hex.EncodeToString([]byte("baz"))}, + }, + }, + }, + { + Inner: []*testpb.DeeplyNestedRepeatedSigner_Inner_Inner_Bottom{ + { + Signer: []string{hex.EncodeToString([]byte("qux")), hex.EncodeToString([]byte("fuz"))}, + }, + { + Signer: []string{hex.EncodeToString([]byte("bing")), hex.EncodeToString([]byte("bap"))}, + }, + }, + }, + }, + }, + }, +} + +func TestGetGetSignersFnConcurrent(t *testing.T) { + ctx, err := NewContext(Options{ + AddressCodec: dummyAddressCodec{}, + ValidatorAddressCodec: dummyValidatorAddressCodec{}, + }) + require.NoError(t, err) + + desc := (&testpb.RepeatedSigner{}).ProtoReflect().Descriptor() + for i := 0; i < 50; i++ { + go func() { + _, _ = ctx.getGetSignersFn(desc) + }() + } +} + +>>>>>>> de0708b40 (fix(x/tx): concurrent map writes when calling GetSigners (#21073)) func TestGetSigners(t *testing.T) { ctx, err := NewContext(Options{ AddressCodec: dummyAddressCodec{}, From d8efd415ae5583aefec9d9f3f0815fd295d33a14 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Wed, 31 Jul 2024 22:53:00 +0200 Subject: [PATCH 2/3] revert x/tx --- x/tx/CHANGELOG.md | 84 ------------------------------------ x/tx/signing/context.go | 12 ++---- x/tx/signing/context_test.go | 55 ----------------------- 3 files changed, 4 insertions(+), 147 deletions(-) diff --git a/x/tx/CHANGELOG.md b/x/tx/CHANGELOG.md index 35838dff85e..d26a47f4b6b 100644 --- a/x/tx/CHANGELOG.md +++ b/x/tx/CHANGELOG.md @@ -31,90 +31,6 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [Unreleased] -<<<<<<< HEAD -======= -### Improvements - -* [#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 - -### Improvements - -* [#20049](https://github.com/cosmos/cosmos-sdk/pull/20049) Sort JSON attributes for `inline_json` encoder. - -## [v0.13.2](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v0.13.2) - 2024-04-12 - -### Features - -* [#19786](https://github.com/cosmos/cosmos-sdk/pull/19786)/[#19919](https://github.com/cosmos/cosmos-sdk/pull/19919) Add "inline_json" option to Amino JSON encoder. - -### Improvements - -* [#19845](https://github.com/cosmos/cosmos-sdk/pull/19845) Use hybrid resolver instead of only protov2 registry - -### Bug Fixes - -* [#19955](https://github.com/cosmos/cosmos-sdk/pull/19955) Don't shadow Amino marshalling error messages - -## [v0.13.1](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v0.13.1) - 2024-03-05 - -### Features - -* [#19618](https://github.com/cosmos/cosmos-sdk/pull/19618) Add enum as string option to encoder. - -### Improvements - -* [#18857](https://github.com/cosmos/cosmos-sdk/pull/18857) Moved `FormatCoins` from `core/coins` to this package under `signing/textual`. - -### Bug Fixes - -* [#19265](https://github.com/cosmos/cosmos-sdk/pull/19265) Reject denoms that contain a comma. - -## [v0.13.0](https://github.com/cosmos/cosmos-sdk/releases/tag/x/tx/v0.13.0) - 2023-12-19 - -### Improvements - -* [#18740](https://github.com/cosmos/cosmos-sdk/pull/18740) Support nested messages when fetching signers up to a default depth of 32. - -## v0.12.0 - -### Improvements - -* [#18309](https://github.com/cosmos/cosmos-sdk/pull/18309) Update encoder so that amino types default to msg type url. - -## v0.11.0 - -### Improvements - -* [#17787](https://github.com/cosmos/cosmos-sdk/pull/17787) Drop tip support. - -## v0.10.0 - -### Features - -* [#17681](https://github.com/cosmos/cosmos-sdk/pull/17681) Add encoder `DefineTypeEncoding` method for defining custom type encodings. -* [#17600](https://github.com/cosmos/cosmos-sdk/pull/17600) Add encoder `DefineScalarEncoding` method for defining custom scalar encodings. -* [#17600](https://github.com/cosmos/cosmos-sdk/pull/17600) Add indent option to encoder. - -## v0.9.1 - -### Improvements - -* [#16936](https://github.com/cosmos/cosmos-sdk/pull/16936) Remove extra whitespace when marshalling module accounts. - -## v0.9.0 - -### Bug Fixes - -* [#16681](https://github.com/cosmos/cosmos-sdk/pull/16681): Catch and fix `(*Decoder).Decode` crash from invalid length prefix in Tx bytes. - -### Improvements - -* [#16846](https://github.com/cosmos/cosmos-sdk/pull/16846): Harmonize interface `signing.TypeResolver` with the rest of the codebase (orm and client/v2). -* [#16684](https://github.com/cosmos/cosmos-sdk/pull/16684): Use `io.WriteString`+`fmt.Fprintf` to remove unnecessary `string`->`[]byte` roundtrip. - ->>>>>>> de0708b40 (fix(x/tx): concurrent map writes when calling GetSigners (#21073)) ## v0.8.0 ### Improvements diff --git a/x/tx/signing/context.go b/x/tx/signing/context.go index 535838fe9cb..c4e99bdf200 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" "google.golang.org/protobuf/proto" @@ -24,7 +23,7 @@ type Context struct { typeResolver protoregistry.MessageTypeResolver addressCodec address.Codec validatorAddressCodec address.Codec - getSignersFuncs sync.Map + getSignersFuncs map[protoreflect.FullName]GetSignersFunc customGetSignerFuncs map[protoreflect.FullName]GetSignersFunc } @@ -96,7 +95,7 @@ func NewContext(options Options) (*Context, error) { typeResolver: protoTypes, addressCodec: options.AddressCodec, validatorAddressCodec: options.ValidatorAddressCodec, - getSignersFuncs: sync.Map{}, + getSignersFuncs: map[protoreflect.FullName]GetSignersFunc{}, customGetSignerFuncs: customGetSignerFuncs, } @@ -324,17 +323,14 @@ func (c *Context) getGetSignersFn(messageDescriptor protoreflect.MessageDescript if ok { return f, nil } - - loadedFn, ok := c.getSignersFuncs.Load(messageDescriptor.FullName()) + f, ok = c.getSignersFuncs[messageDescriptor.FullName()] if !ok { var err error f, err = c.makeGetSignersFunc(messageDescriptor) if err != nil { return nil, err } - c.getSignersFuncs.Store(messageDescriptor.FullName(), f) - } else { - f = loadedFn.(GetSignersFunc) + c.getSignersFuncs[messageDescriptor.FullName()] = f } return f, nil diff --git a/x/tx/signing/context_test.go b/x/tx/signing/context_test.go index e780f1f3dfd..0be18c98914 100644 --- a/x/tx/signing/context_test.go +++ b/x/tx/signing/context_test.go @@ -14,61 +14,6 @@ import ( "cosmossdk.io/x/tx/internal/testpb" ) -<<<<<<< HEAD -======= -var deeplyNestedRepeatedSigner = &testpb.DeeplyNestedRepeatedSigner{ - Inner: []*testpb.DeeplyNestedRepeatedSigner_Inner{ - { - Inner: []*testpb.DeeplyNestedRepeatedSigner_Inner_Inner{ - { - Inner: []*testpb.DeeplyNestedRepeatedSigner_Inner_Inner_Bottom{ - { - Signer: []string{hex.EncodeToString([]byte("foo")), hex.EncodeToString([]byte("bar"))}, - }, - }, - }, - }, - }, - { - Inner: []*testpb.DeeplyNestedRepeatedSigner_Inner_Inner{ - { - Inner: []*testpb.DeeplyNestedRepeatedSigner_Inner_Inner_Bottom{ - { - Signer: []string{hex.EncodeToString([]byte("baz"))}, - }, - }, - }, - { - Inner: []*testpb.DeeplyNestedRepeatedSigner_Inner_Inner_Bottom{ - { - Signer: []string{hex.EncodeToString([]byte("qux")), hex.EncodeToString([]byte("fuz"))}, - }, - { - Signer: []string{hex.EncodeToString([]byte("bing")), hex.EncodeToString([]byte("bap"))}, - }, - }, - }, - }, - }, - }, -} - -func TestGetGetSignersFnConcurrent(t *testing.T) { - ctx, err := NewContext(Options{ - AddressCodec: dummyAddressCodec{}, - ValidatorAddressCodec: dummyValidatorAddressCodec{}, - }) - require.NoError(t, err) - - desc := (&testpb.RepeatedSigner{}).ProtoReflect().Descriptor() - for i := 0; i < 50; i++ { - go func() { - _, _ = ctx.getGetSignersFn(desc) - }() - } -} - ->>>>>>> de0708b40 (fix(x/tx): concurrent map writes when calling GetSigners (#21073)) func TestGetSigners(t *testing.T) { ctx, err := NewContext(Options{ AddressCodec: dummyAddressCodec{}, From 8cfad2ba83807a34edb17fe0ea832077e1bea6cc Mon Sep 17 00:00:00 2001 From: Facundo Date: Thu, 1 Aug 2024 15:25:48 +0200 Subject: [PATCH 3/3] fix --- simapp/app.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/simapp/app.go b/simapp/app.go index 068dd9a2e95..35ce2e5bac2 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -209,7 +209,7 @@ func NewSimApp( legacyAmino := codec.NewLegacyAmino() txConfig := tx.NewTxConfig(appCodec, tx.DefaultSignModes) - if err := signingCtx.Validate(); err != nil { + if err := txConfig.SigningContext().Validate(); err != nil { panic(err) }