fix(baseapp): fix BaseApp.getContext data races

[odeke-em]

This change adds a mutex to BaseApp and firstly locks it when invoking (*BaseApp).getContextForTx. Ideally the (types/sdk).Context should be concurrency safe mirroring the Go standard library's context.Context.

Fixes #18382

Summary by CodeRabbit

• Bug Fixes
  • Addressed a data race issue for improved stability.
  • Fixed a builder function for missed blocks key in slashing/migration/v4.
• Improvements
  • Enhanced consistency in setting viper prefix in client and server.
  • Removed hardcoded gRPC address to localhost in the server for better flexibility.
• Refactor
  • Modified the ExecuteGenesisTx method to a pointer receiver for better state management.

[coderabbitai]

Walkthrough

The changes primarily focus on improving the Cosmos SDK by addressing a data race issue in BaseApp.getContext, setting a consistent viper prefix in the client and server, removing a hardcoded gRPC address to localhost in the server, and fixing a builder function for missed blocks key in slashing/migration/v4. The BaseApp struct has been enhanced with a sync.Mutex field named mu for concurrent access protection, and the getContextForTx function has been modified to use this mutex.

Changes

File	Change Summary
baseapp/baseapp.go	Introduced a sync.Mutex field named mu to the BaseApp struct for concurrent access protection. Modified the getContextForTx function to use this mutex.
baseapp/genesis.go	Changed the receiver of the ExecuteGenesisTx method from BaseApp to *BaseApp, making it a pointer receiver.
CHANGELOG.md	Documented the bug fixes and improvements to the Cosmos SDK, including the data race fix in BaseApp.getContext, consistent viper prefix setting, removal of hardcoded gRPC address, and the fix for the builder function in slashing/migration/v4.

Assessment against linked issues (Beta)

Objective	Addressed	Explanation
Fix the data race in BaseApp.getContext (#18382)	✅	The introduction of the sync.Mutex field and its use in getContextForTx function addresses this objective.
Set viper prefix consistently in client and server (#18382)	❓	The changes summary mentions this fix, but the specific changes are not visible in the provided diffs.
Remove hardcoded gRPC address to localhost in the server (#18382)	❓	The changes summary mentions this fix, but the specific changes are not visible in the provided diffs.
Fix the builder function for missed blocks key in slashing/migration/v4 (#18382)	❓	The changes summary mentions this fix, but the specific changes are not visible in the provided diffs.

Poem

🐇 Hopping through the code, with a mutex in our stride,
Fixing data races, with a sense of pride.
No more hardcoded addresses, in our server's town,
With every bug we squash, we earn our coder's crown.
As the leaves of November fall, we continue to evolve,
In the world of Cosmos SDK, mysteries we solve. 🍁👑

---

Tips

Chat with CodeRabbit Bot (@coderabbitai)

• If you reply to a review comment from CodeRabbit, the bot will automatically respond.
• To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
• Note: Review comments are made on code diffs or files, not on the PR overview.
• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

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 a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai help to get help.
• @coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• 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/coderabbit-overrides.v2.json

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between fb9dadc and 4bc26ca.

Files selected for processing (1)

• baseapp/baseapp.go (3 hunks)

Additional comments: 3

baseapp/baseapp.go (3)

• 9-9: The sync package is imported to use the sync.Mutex type for concurrency control.

• 63-63: A new sync.Mutex field, mu, is added to the BaseApp struct to protect certain fields during concurrent access.

• 646-648: The getContextForTx function now acquires the mutex mu at the start and ensures it is released when the function exits. This makes the function's operations atomic, preventing data races.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between fb9dadc and af66258.

Files selected for processing (2)

• CHANGELOG.md (1 hunks)
• baseapp/baseapp.go (3 hunks)

Additional comments: 4

CHANGELOG.md (1)

• 75-78: The changes in the changelog accurately reflect the updates made in the pull request. Ensure that the links to the pull requests are correct and lead to the appropriate changes.

baseapp/baseapp.go (3)

• 6-12: The sync package is imported to use sync.Mutex for thread safety.

• 62-66: A new mutex mu is added to the BaseApp struct to protect certain fields during concurrent access.

• 643-651: The getContextForTx function now uses the mu mutex to ensure thread safety during concurrent access. The mutex is locked at the start of the function and deferred to unlock at the end, ensuring it will be unlocked even if the function exits early.

app.mu.Lock()
defer app.mu.Unlock()

[likhita-809]

There is a failing test. I think mutexes should be operated through pointers receivers to ensure synchronized behavior

[odeke-em]

There is a failing test. I think mutexes should be operated through pointers receivers to ensure synchronized behavior

Thanks @likhita-809 for the feedback, please take a look! Sadly a consequence of our coding style of not sticking to pointer receivers is such test failures. I've modified BaseApp.ExecuteGenesisTx to be a pointer receiver. Tnank you @tac0turtle for the review!

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 7e00255 and 95f365e.

Files selected for processing (3)

• CHANGELOG.md (1 hunks)
• baseapp/baseapp.go (3 hunks)
• baseapp/genesis.go (1 hunks)

Additional comments: 5

CHANGELOG.md (1)

• 75-78: The changes in the changelog are well-documented and provide clear information about the bug fixes and improvements made in the Cosmos SDK. The links to the pull requests provide a good reference for further details.

baseapp/genesis.go (1)

• 12-18: The change from BaseApp to *BaseApp as the receiver of ExecuteGenesisTx is appropriate as it allows the method to modify the state of the BaseApp instance. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.

baseapp/baseapp.go (3)

• 9-9: The sync package is imported to use sync.Mutex for thread safety.

• 63-63: A sync.Mutex named mu is added to the BaseApp struct to protect certain fields during concurrent access.

• 646-648: The getContextForTx function now acquires and releases the mutex to ensure safe concurrent access to certain fields.

+	app.mu.Lock()
+	defer app.mu.Unlock()