fix(x/tx): concurrent map writes when calling GetSigners

[facundomedica]

Description

Whenever GetSigners is called and the getSignerFn hasn't been set for the message type, it will set it on an internal map. If this is called in 2 or more places at the same time it will cause a concurrent map write issue.

To avoid this, developers must call interfaceRegistry.SigningContext().Validate() when creating the app (depinject users don't need this as it's being done when providing the interfaceRegistry).

Also, a write only mutex has been added as a defensive measure in case there's any unhandled case (most likely because of a misconfiguration, or usage of multiple instances of SigningContexts). This shouldn't add any overhead to well configured binaries, and little to misconfigured ones.

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title, you can find examples of the prefixes below:
• confirmed ! in the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• reviewed "Files changed" and left comments if necessary
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• updated the relevant documentation or specification, including comments for documenting Go code
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.

I have...

• confirmed the correct type prefix in the PR title
• confirmed all author checklist items have been addressed
• reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

• New Features

  • Introduced a validation step during context initialization to enhance robustness and error handling.
  • Improved the method for retrieving signer functions to utilize thread-safe access for better reliability.
  • Added new tests to ensure concurrency behavior of signer function retrieval.

• Bug Fixes

  • Resolved potential data corruption issues by safeguarding access to the signers' function map with concurrency controls.

• Documentation

  • Updated the changelog for the x/tx module to reflect recent improvements and enhancements.

[coderabbitai]

Walkthrough

Walkthrough

The recent changes enhance the robustness and thread safety of the application. A new validation step in the NewSimApp function ensures the integrity of the SigningContext. Additionally, a sync.Map and a write-only mutex have been introduced in the Context struct to prevent concurrent write access issues in the getSignersFuncs map. Further improvements include validation logic in the NewContext function and the addition of concurrency tests for signing functions.

Changes

File(s)	Change Summary
simapp/app.go	Added validation for SigningContext in the NewSimApp function, enhancing initialization robustness.
x/tx/CHANGELOG.md	Updated changelog to include improvements related to mutex implementation for thread safety.
x/tx/signing/context.go	Introduced a sync.Map in the Context struct to safeguard the getSignersFuncs map from concurrent access. Modified getGetSignersFn method for thread-safe operation.
x/tx/signing/context_test.go	Added TestGetGetSignersFnConcurrent to test concurrency behavior of getGetSignersFn method.

Sequence Diagram(s)

sequenceDiagram
    participant App as SimApp
    participant Context as SigningContext
    
    App->>Context: Initialize SigningContext
    alt Validation Check
        Context->>App: Valid SigningContext
    else Validation Fails
        App->>App: Panic (Critical Error)
    end

Loading

sequenceDiagram
    participant Context as Context
    participant Signers as GetSignersFuncs

    Context->>Signers: Lock Mutex
    Signers->>Context: Access Functions
    Context->>Signers: Unlock Mutex

Loading

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 683371f and 33348e7.

Files selected for processing (3)

• simapp/app.go (1 hunks)
• x/tx/CHANGELOG.md (1 hunks)
• x/tx/signing/context.go (4 hunks)

Additional context used

Path-based instructions (3)

x/tx/CHANGELOG.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

x/tx/signing/context.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

simapp/app.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (6)

x/tx/CHANGELOG.md (1)

36-38: Changelog entry looks good!

The changelog entry is correctly formatted and provides a clear description of the improvement.

x/tx/signing/context.go (4)

6-6: Import statement looks good!

The import of the sync package is necessary for the mutex implementation.

---

35-37: Context struct changes look good!

The addition of the mtx field of type sync.Mutex is appropriate for enhancing thread safety.

---

116-116: NewContext function changes look good!

The initialization of the mtx field in the NewContext function is correct and necessary.

---

343-344: Mutex locking in getGetSignersFn looks good!

The addition of c.mtx.Lock() and defer c.mtx.Unlock() ensures thread safety for the getSignersFuncs map.

simapp/app.go (1)

606-610: SigningContext validation in NewSimApp looks good!

The addition of the validation step for SigningContext ensures that the internal signers' functions map is correctly pre-filled and any issues are caught early.

[damiannolan]

Nice! Thank you @facundomedica.

I think in future I'd be favour of moving the current Validate() logic (greedy startup approach) in NewContext() rather than in the validation func itself.

That, or change the return on L122 to be the following, since NewContext already has an error return:

return c, c.Validate()

Maybe I'm missing something but I'm not sure I see the use case for creating getSignersFunc at runtime/on the fly

[kocubinski]

LGTM

[kocubinski]

lgtm because makeGetSignersFunc is stateless right? so worst case we get multiple blocked writes which will quickly resolve

[facundomedica]

Yeap, before the worst case would bring the node down, now the worst case just creates some tiny delays

[facundomedica]

Nice! Thank you @facundomedica.

I think in future I'd be favour of moving the current Validate() logic (greedy startup approach) in NewContext() rather than in the validation func itself.

That, or change the return on L122 to be the following, since NewContext already has an error return:

return c, c.Validate()

Maybe I'm missing something but I'm not sure I see the use case for creating getSignersFunc at runtime/on the fly

I think we can't do this as there are going to be other messages being registered later. Maybe every time we register a new message we call the getSignersFunc? 🤔

[kocubinski]

Sorry, I took a deeper look at the golang map spec; even with a single writer and single concurrent reader this is still a panic vector.

https://go.dev/play/p/ws2BvhfI3-p

[kocubinski]

2 options I see:

1. commit to sync.Map
2. return err if signer func not found with a nag msg like "hey did you run Validate()" ?.

I think (2) is OK, protoc generated types register their file descriptors in init() which runs before main(), so we can expect any type which has been imported will be ranged over.

[kocubinski]

return c, c.Validate()

Maybe I'm missing something but I'm not sure I see the use case for creating getSignersFunc at runtime/on the fly

IIRC we were worried about runtime overhead of map building and memory usage but that doesn't seem to carry much weight, I like this solution

[julienrbrt]

I'll hold off tagging v0.50.9 until this is resolved.

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 33348e7 and 8f8322d.

Files selected for processing (1)

• x/tx/signing/context.go (4 hunks)

Additional context used

Path-based instructions (1)

x/tx/signing/context.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (4)

x/tx/signing/context.go (4)

118-120: LGTM! Validation step enhances robustness.

The inclusion of the validation step ensures that the context is properly configured before use, enhancing the robustness of the code.

---

168-168: LGTM! Validation logic is sound.

The validation logic checks for errors in message signers and pre-populates the internal cache, improving performance and error handling.

---

336-348: LGTM! Improved error handling with add parameter.

The introduction of the add parameter enhances error handling by preventing the creation of new signer functions if validation has not been called.

---

360-362: LGTM! Adheres to new validation logic.

The update ensures that the GetSigners function adheres to the new validation logic, preventing the creation of new signer functions if validation has not been called.

[coderabbitai]

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)

x/tx/signing/context_test.go (1)

54-79: Good concurrency test, but add validation checks.

The function effectively tests concurrent access to getGetSignersFn. However, uncommenting the validation checks would ensure the correctness of the results.

-	// err = ctx.Validate()
-	// require.NoError(t, err)
+	err = ctx.Validate()
+	require.NoError(t, err)

-	// signers, err := fn(msg)
-	// require.NoError(t, err)
-	// require.Equal(t, [][]byte{[]byte("foo")}, signers)
+	fn, err := ctx.getGetSignersFn(desc)
+	require.NoError(t, err)
+	signers, err := fn(msg)
+	require.NoError(t, err)
+	require.Equal(t, [][]byte{[]byte("foo")}, signers)

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 8f8322d and 4f31098.

Files selected for processing (2)

• x/tx/signing/context.go (1 hunks)
• x/tx/signing/context_test.go (1 hunks)

Files skipped from review as they are similar to previous changes (1)

• x/tx/signing/context.go

Additional context used

Path-based instructions (1)

x/tx/signing/context_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

---

Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

Additional comments not posted (3)

x/tx/signing/context_test.go (3)

Line range hint 80-153:
Comprehensive test coverage for GetSigners.

The function thoroughly tests various message types for the GetSigners method.

---

Line range hint 155-174:
Effective test for maximum recursion depth.

The function effectively tests the recursion depth limits for the GetSigners method.

---

Line range hint 176-211:
Comprehensive test for custom signer definitions.

The function thoroughly tests various scenarios for custom signer definitions in the GetSigners method.

[julienrbrt]

I'll hold off tagging v0.50.9 until this is resolved.

I see now it is only touching x/tx. So no need to backport anything. I'll check if the last tagged version of x/tx corresponds to what x/tx main is. And do some branching if not to release this.

[julienrbrt]

ACK

[facundomedica]

I think I'll have to remove the Validate when we create it as there is one instance in which we create a new interface registry and we don't pass the custom signers func and fails (see the integration test)

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between c077988 and 313f9eb.

Files selected for processing (2)

• simapp/app.go (1 hunks)
• x/tx/signing/context.go (4 hunks)

Files skipped from review as they are similar to previous changes (2)

• simapp/app.go
• x/tx/signing/context.go

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 313f9eb and a042f81.

Files selected for processing (1)

• x/tx/signing/context_test.go (1 hunks)

Files skipped from review as they are similar to previous changes (1)

• x/tx/signing/context_test.go