[OTE-151] Create ante handlers which parallelize processing per account until a global lock is necessary. #1108
Conversation
…nt until a global lock is necessary.
WalkthroughThe recent update enhances the application's transaction handling by refining the AnteHandler logic, introducing new decorators, and improving control flow. It also adds thread safety to critical components, integrates better testing mechanisms, and updates the handling of specific message types. Additionally, the update includes performance monitoring enhancements and adjustments in data management, ensuring a more robust and efficient application operation. Changes
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
protocol/go.modis excluded by:!**/*.modprotocol/go.sumis excluded by:!**/*.sum
Files selected for processing (10)
- protocol/app/ante.go (3 hunks)
- protocol/app/ante/basic_test.go (3 hunks)
- protocol/app/ante_test.go (3 hunks)
- protocol/app/app.go (2 hunks)
- protocol/app/datadog.go (1 hunks)
- protocol/app/e2e/ante_test.go (1 hunks)
- protocol/app/e2e/query_test.go (3 hunks)
- protocol/lib/ante/app_injected_msg_test.go (2 hunks)
- protocol/testutil/app/app.go (8 hunks)
- protocol/testutil/constants/positions.go (1 hunks)
Additional comments: 22
protocol/lib/ante/app_injected_msg_test.go (2)
- 4-4: The addition of the
testdataimport fromgithub.com/cosmos/cosmos-sdk/testutil/testdatais appropriate for testing purposes.- 17-19: The introduction of the global variable
testMsginitialized with&testdata.TestMsg{Signers: []string{"meh"}}is suitable for simulating test messages. However, be mindful of potential test isolation issues when using global variables in test environments.protocol/app/ante_test.go (2)
- 4-5: The addition of imports for
cosmossdk.io/store/rootmultiandgithub.com/cosmos/cosmos-sdk/x/auth/typessupports the new functionality introduced in this file.- 59-66: The error checks for
nil Codecandnil AuthStoreKeyinTestNewAnteHandler_Errorare important for validating the initialization process of the ante handler. These checks ensure that the handler is correctly configured before use.protocol/app/e2e/query_test.go (1)
- 6-6: Renaming the package alias from
testApptotestappimproves consistency with Go's convention for package aliases. This is a good practice for readability and maintainability.protocol/app/ante/basic_test.go (1)
- 35-41: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [38-74]
Simplifying message validation in
TestValidateBasic_AppInjectedMsgWrapperby removing thelibantepackage usage and adjusting test cases is a positive change for maintainability. Ensure that the updated test cases still provide comprehensive coverage for all relevant scenarios.protocol/testutil/constants/positions.go (1)
- 81-84: The addition of the
Usdc_Asset_1constant withQuantumsset to1_000_000is a useful enhancement for testing and development. Ensure that the naming convention is consistent with other constants in the file for clarity and maintainability.protocol/app/datadog.go (1)
- 113-114: Adding
profiler.BlockProfileandprofiler.GoroutineProfileto the Datadog profiler initialization enhances the profiling capabilities, providing valuable insights into the application's performance. This is a positive change for identifying and addressing performance bottlenecks.protocol/app/e2e/ante_test.go (1)
- 31-270: The test function
TestParallelAnteHandler_ClobAndOtheris well-structured and aims to validate the concurrent processing capabilities of the updated ante handlers. It uses a combination of account setup, block advancement, and concurrent transaction sending to simulate a realistic scenario. Here are some observations and recommendations:
- Correctness and Logic: The test logic appears sound, with a clear setup of test accounts, balances, and concurrent transaction sending. The use of atomic variables and wait groups to manage concurrency is appropriate.
- Performance: While the test is designed to detect data races and not specifically to benchmark performance, it's important to ensure that the number of iterations and the setup do not inadvertently lead to excessively long test times. Consider parameterizing the number of iterations or accounts based on the testing environment if needed.
- Concurrency Handling: The use of Go's
-raceflag, atomic variables, and wait groups is appropriate for the intended concurrency testing. However, ensure that the test adequately covers the scenarios where data races might occur, especially around the global lock's usage in the actual ante handlers.- Best Practices: The test follows good practices in terms of structuring and isolating the test scenario. However, consider adding more detailed comments explaining the rationale behind specific concurrency choices, such as the use of atomic variables and the specific setup of test accounts and balances. This will help future maintainers understand the test's intent more clearly.
Overall, the test function is well-implemented for its intended purpose of validating concurrent processing capabilities and detecting data races. Ensure thorough coverage of different transaction types and concurrency scenarios to maximize the test's effectiveness.
protocol/app/ante.go (3)
- 25-27: The addition of
ClobKeeperto theHandlerOptionsstruct is a crucial change that supports the new ante handlers' functionality. This change allows the ante handlers to interact with the clob module, which is essential for processing clob-related transactions. Ensure that theClobKeeperis properly initialized and passed to theNewAnteHandlerfunction to avoid runtime errors.- 124-233: The
clobAnteHandlefunction introduces a specialized handling path for clob messages, including a per-account locking mechanism duringcheckTxand a global lock for operations that are not thread-safe. This approach is aligned with the PR's objectives to optimize performance by minimizing locking. However, it's crucial to ensure that the locking logic is thoroughly tested to prevent deadlocks and to verify that it does not introduce significant performance bottlenecks. Additionally, consider the implications of branching the state store and ensure that state mutations are correctly managed across different transaction types.- 278-369: The
otherMsgAnteHandlefunction processes non-clob and non-app injected messages. It includes a comprehensive set of ante decorators and introduces a global lock for certain operations. While the function appears to be correctly implemented, it's important to ensure that the global lock is used judiciously to avoid unnecessary performance bottlenecks. Additionally, verify that the state store branching and writing logic is correctly handled in different transaction contexts (checkTx,deliverTx, and simulation).Verification successful
To accurately verify the handling of state store branching and writing in different transaction contexts (
checkTx,deliverTx, and simulation) as well as the use of the global lock, we need to analyze the relevant parts of the codebase more closely. Since the previous script did not perform this analysis, let's proceed with generating new shell scripts aimed at examining the implementation details related to state store management and global lock usage.
The analysis of the
protocol/app/ante.gofile confirms that theotherMsgAnteHandlefunction correctly implements state store branching and writing logic for different transaction contexts (checkTx,deliverTx, and simulation). The use of a global lock to ensure thread safety during operations that could potentially lead to concurrent access and mutation of state is also appropriately handled. The code explicitly manages state store writing during non-simulatedcheckTxandreCheckTx, while delegating the responsibility to the Cosmos SDK duringdeliverTxand simulation. This confirms that the concerns raised in the review comment regarding state store management and global lock usage have been addressed in the implementation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Add a script to verify the correct handling of state store branching and writing in different transaction contexts. echo "Script to verify state store management across transaction contexts."Length of output: 146
Script:
#!/bin/bash # Search for references to state store branching and writing logic, and global lock usage in the relevant file. grep -C 10 -E "MultiStore|CacheMultiStore|Write|Lock" protocol/app/ante.goLength of output: 5660
Script:
#!/bin/bash # Search for usage of transaction contexts to verify correct handling in different contexts. grep -C 5 -E "IsCheckTx|IsReCheckTx|IsDeliverTx" protocol/app/ante.goLength of output: 2417
protocol/testutil/app/app.go (8)
- 435-435: The addition of the
sync.RWMutexnamedmtxto theTestAppstruct is a critical change for managing concurrent access and ensuring thread safety. This is a necessary addition given the PR's objectives to enable parallel processing and improve transaction processing efficiency.- 444-446: The usage of
mtx.Lock()andmtx.Unlock()in theInitChainmethod correctly implements locking to ensure thread safety during the initialization of the chain. This pattern is crucial for preventing race conditions when the chain is being initialized concurrently from multiple goroutines.- 675-677: Similarly, in the
AdvanceToBlockmethod, the use ofmtx.Lock()andmtx.Unlock()is appropriate for ensuring that advancing the block is done in a thread-safe manner. This method's critical section, which modifies the state of theTestApp, is correctly protected by the mutex.- 1048-1049: The use of
mtx.RLock()andmtx.RUnlock()in theGetHeadermethod is a good practice for read-only operations, allowing multiple goroutines to read the header concurrently without blocking each other, as long as no write operation is happening.- 1055-1056: The
GetBlockHeightmethod also correctly uses read locks withmtx.RLock()andmtx.RUnlock(), which is suitable for concurrent read access to the block height. This ensures thread safety without unnecessary blocking.- 1062-1063: The
GetHaltedmethod's use of read locks is consistent with best practices for concurrent reads, ensuring that accessing the halted state is thread-safe.- 1141-1143: The
PrepareProposalmethod correctly acquires a lock before proceeding with the preparation of a proposal. This ensures that the operation is thread-safe and prevents concurrent modifications that could lead to inconsistent states.- 1158-1160: The
GetProposedOperationsTxmethod's use of locking is consistent with ensuring thread safety during the retrieval of proposed operations. This pattern is crucial for maintaining consistency in a concurrent environment.protocol/app/app.go (2)
- 396-396: The addition of locking to the
authtypes.StoreKeyusingWithLocking()method is a critical change for ensuring thread safety when accessing the store key. This change is aligned with the PR's objective to enhance efficiency through parallel processing and strategic locking. However, it's important to ensure that all access patterns to this store key across the application have been audited to prevent deadlocks.- 1642-1644: The modifications to the assignment of
ClobKeeperandAuthStoreKeyfields in theHandlerOptionsstruct are crucial for the ante handler setup. It's important to verify that these changes are consistent with the intended use of these fields throughout the application. Additionally, ensure that theCodecandAuthStoreKeyare correctly utilized in the ante handler logic to maintain the application's integrity and security.
| tApp.mtx.RLock() | ||
| defer tApp.mtx.RUnlock() | ||
|
|
There was a problem hiding this comment.
In the CheckTx method, the decision to use read locks (mtx.RLock() and mtx.RUnlock()) during the execution of CheckTx on the underlying App is appropriate. However, it's crucial to ensure that any state modifications made as a result of CheckTx are also protected by appropriate locking mechanisms, especially when modifying passingCheckTxs.
Consider using a write lock (mtx.Lock() and mtx.Unlock()) when modifying passingCheckTxs to ensure full thread safety during state modifications.
- tApp.mtx.RLock()
- defer tApp.mtx.RUnlock()
+ tApp.mtx.Lock()
+ defer tApp.mtx.Unlock()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| tApp.mtx.RLock() | |
| defer tApp.mtx.RUnlock() | |
| tApp.mtx.Lock() | |
| defer tApp.mtx.Unlock() | |
| ClobKeeper: dydxApp.ClobKeeper, | ||
| Codec: encodingConfig.Codec, | ||
| AuthStoreKey: dydxApp.CommitMultiStore().(*rootmulti.Store).StoreKeysByName()[authtypes.StoreKey], |
There was a problem hiding this comment.
Including Codec and AuthStoreKey in app.HandlerOptions is a necessary enhancement for ensuring the ante handler has all required dependencies. Consider adding comments to explain the purpose of these additions for future maintainability.
+ // Codec is used for encoding/decoding transactions.
Codec: encodingConfig.Codec,
+ // AuthStoreKey is used to access the auth module's store.
AuthStoreKey: dydxApp.CommitMultiStore().(*rootmulti.Store).StoreKeysByName()[authtypes.StoreKey],Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| ClobKeeper: dydxApp.ClobKeeper, | |
| Codec: encodingConfig.Codec, | |
| AuthStoreKey: dydxApp.CommitMultiStore().(*rootmulti.Store).StoreKeysByName()[authtypes.StoreKey], | |
| ClobKeeper: dydxApp.ClobKeeper, | |
| // Codec is used for encoding/decoding transactions. | |
| Codec: encodingConfig.Codec, | |
| // AuthStoreKey is used to access the auth module's store. | |
| AuthStoreKey: dydxApp.CommitMultiStore().(*rootmulti.Store).StoreKeysByName()[authtypes.StoreKey], |
| h := &lockingAnteHandler{ | ||
| authStoreKey: options.AuthStoreKey, | ||
| setupContextDecorator: ante.NewSetUpContextDecorator(), | ||
| freeInfiniteGasDecorator: customante.NewFreeInfiniteGasDecorator(), | ||
| extensionOptionsChecker: ante.NewExtensionOptionsDecorator(options.ExtensionOptionChecker), | ||
| validateMsgType: customante.NewValidateMsgTypeDecorator(), | ||
| txTimeoutHeight: ante.NewTxTimeoutHeightDecorator(), | ||
| validateMemo: ante.NewValidateMemoDecorator(options.AccountKeeper), | ||
| validateBasic: ante.NewValidateBasicDecorator(), | ||
| validateSigCount: ante.NewValidateSigCountDecorator(options.AccountKeeper), | ||
| incrementSequence: ante.NewIncrementSequenceDecorator(options.AccountKeeper), | ||
| sigVerification: customante.NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler), | ||
| consumeTxSizeGas: ante.NewConsumeGasForTxSizeDecorator(options.AccountKeeper), | ||
| deductFee: ante.NewDeductFeeDecorator( | ||
| options.AccountKeeper, | ||
| options.BankKeeper, | ||
| options.FeegrantKeeper, | ||
| options.TxFeeChecker, | ||
| ), | ||
| setPubKey: ante.NewSetPubKeyDecorator(options.AccountKeeper), | ||
| sigGasConsume: ante.NewSigGasConsumeDecorator(options.AccountKeeper, options.SigGasConsumer), | ||
| clobRateLimit: clobante.NewRateLimitDecorator(options.ClobKeeper), | ||
| clob: clobante.NewClobDecorator(options.ClobKeeper), | ||
| } | ||
| return h.AnteHandle, nil |
There was a problem hiding this comment.
The restructuring of the AnteHandler chain to include new decorators and the introduction of a locking mechanism (lockingAnteHandler) are significant changes aimed at supporting parallel processing and ensuring data consistency. It's important to ensure that each decorator is correctly implemented and that their order in the chain is logical and efficient. Additionally, the locking mechanism must be carefully reviewed to prevent deadlocks and ensure it does not introduce significant performance bottlenecks, especially under high load. Consider adding more detailed comments explaining the locking strategy and its implications on performance and concurrency.
+ // Add detailed comments explaining the locking strategy and its implications on performance and concurrency.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| h := &lockingAnteHandler{ | |
| authStoreKey: options.AuthStoreKey, | |
| setupContextDecorator: ante.NewSetUpContextDecorator(), | |
| freeInfiniteGasDecorator: customante.NewFreeInfiniteGasDecorator(), | |
| extensionOptionsChecker: ante.NewExtensionOptionsDecorator(options.ExtensionOptionChecker), | |
| validateMsgType: customante.NewValidateMsgTypeDecorator(), | |
| txTimeoutHeight: ante.NewTxTimeoutHeightDecorator(), | |
| validateMemo: ante.NewValidateMemoDecorator(options.AccountKeeper), | |
| validateBasic: ante.NewValidateBasicDecorator(), | |
| validateSigCount: ante.NewValidateSigCountDecorator(options.AccountKeeper), | |
| incrementSequence: ante.NewIncrementSequenceDecorator(options.AccountKeeper), | |
| sigVerification: customante.NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler), | |
| consumeTxSizeGas: ante.NewConsumeGasForTxSizeDecorator(options.AccountKeeper), | |
| deductFee: ante.NewDeductFeeDecorator( | |
| options.AccountKeeper, | |
| options.BankKeeper, | |
| options.FeegrantKeeper, | |
| options.TxFeeChecker, | |
| ), | |
| setPubKey: ante.NewSetPubKeyDecorator(options.AccountKeeper), | |
| sigGasConsume: ante.NewSigGasConsumeDecorator(options.AccountKeeper, options.SigGasConsumer), | |
| clobRateLimit: clobante.NewRateLimitDecorator(options.ClobKeeper), | |
| clob: clobante.NewClobDecorator(options.ClobKeeper), | |
| } | |
| return h.AnteHandle, nil | |
| h := &lockingAnteHandler{ | |
| authStoreKey: options.AuthStoreKey, | |
| setupContextDecorator: ante.NewSetUpContextDecorator(), | |
| freeInfiniteGasDecorator: customante.NewFreeInfiniteGasDecorator(), | |
| extensionOptionsChecker: ante.NewExtensionOptionsDecorator(options.ExtensionOptionChecker), | |
| validateMsgType: customante.NewValidateMsgTypeDecorator(), | |
| txTimeoutHeight: ante.NewTxTimeoutHeightDecorator(), | |
| validateMemo: ante.NewValidateMemoDecorator(options.AccountKeeper), | |
| validateBasic: ante.NewValidateBasicDecorator(), | |
| validateSigCount: ante.NewValidateSigCountDecorator(options.AccountKeeper), | |
| incrementSequence: ante.NewIncrementSequenceDecorator(options.AccountKeeper), | |
| sigVerification: customante.NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler), | |
| consumeTxSizeGas: ante.NewConsumeGasForTxSizeDecorator(options.AccountKeeper), | |
| deductFee: ante.NewDeductFeeDecorator( | |
| options.AccountKeeper, | |
| options.BankKeeper, | |
| options.FeegrantKeeper, | |
| options.TxFeeChecker, | |
| ), | |
| setPubKey: ante.NewSetPubKeyDecorator(options.AccountKeeper), | |
| sigGasConsume: ante.NewSigGasConsumeDecorator(options.AccountKeeper, options.SigGasConsumer), | |
| clobRateLimit: clobante.NewRateLimitDecorator(options.ClobKeeper), | |
| clob: clobante.NewClobDecorator(options.ClobKeeper), | |
| } | |
| return h.AnteHandle, nil |
protocol/app/datadog.go
Outdated
| @@ -110,6 +110,8 @@ func initDatadogProfiler(logger log.Logger, ddAgentHost string, ddAgentPort uint | |||
| profiler.CPUProfile, | |||
| profiler.HeapProfile, | |||
| profiler.MutexProfile, | |||
| profiler.BlockProfile, | |||
There was a problem hiding this comment.
Didn't mean to include this.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/app/e2e/ante_test.go (1 hunks)
- protocol/lib/ante/app_injected_msg_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/app/e2e/ante_test.go
- protocol/lib/ante/app_injected_msg_test.go
protocol/testutil/app/app.go
Outdated
| @@ -432,6 +432,7 @@ type TestApp struct { | |||
| passingCheckTxs [][]byte | |||
| passingCheckTxsMtx sync.Mutex | |||
| halted bool | |||
| mtx sync.RWMutex | |||
There was a problem hiding this comment.
Why does test app need a separate mutex?
| } | ||
|
|
||
| // NewAnteHandler returns an AnteHandler that checks and increments sequence | ||
| // numbers, checks signatures & account numbers, deducts fees from the first | ||
| // signer, and handles in-memory clob messages. | ||
| // | ||
| // Note that the contract for the forked version of Cosmos SDK is that during `checkTx` the ante handler |
There was a problem hiding this comment.
Can you make explicit what locking requirements ante handlers now have? So when folks add new antehandlers or update forks they know what to look out for.
There was a problem hiding this comment.
Updated the comments.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/app/ante.go (3 hunks)
- protocol/testutil/app/app.go (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/testutil/app/app.go
Additional comments: 9
protocol/app/ante.go (9)
- 5-12: The imports have been updated to include necessary packages for the new ante handlers and decorators. It's important to ensure that all imported packages are used within the file to avoid unnecessary dependencies.
- 25-27: The addition of
Codec,AuthStoreKey, andClobKeeperto theHandlerOptionsstruct is crucial for supporting the new ante handlers. These fields enable the ante handlers to interact with the necessary components of the Cosmos SDK and the custom logic for clob transactions.- 34-55: The detailed comments explaining the locking requirements and their implications are valuable for understanding the concurrency model adopted in the ante handlers. It's essential for future developers to be aware of these requirements when adding new ante handlers or updating existing ones.
- 75-81: The error handling in the
NewAnteHandlerfunction is well-implemented, ensuring that all required components are provided before constructing the ante handler. This prevents runtime errors due to missing dependencies.- 115-135: The
lockingAnteHandlerstruct is well-defined, encapsulating all the necessary ante decorators and the global lock. This struct serves as the foundation for the custom ante handler logic, enabling the parallel processing and locking strategy.- 137-149: The
AnteHandlemethod correctly delegates processing to specific handlers (clobAnteHandle,appInjectedMsgAnteHandle, orotherMsgAnteHandle) based on the message type. This approach simplifies the handling of different message types and allows for more targeted optimizations.- 151-246: The
clobAnteHandlemethod implements the locking strategy for clob transactions, ensuring data consistency during parallel processing. The use of a per-account lock and a global lock is a sound approach to managing concurrency. However, it's crucial to monitor the performance impact of these locks, especially under high load, to prevent bottlenecks.- 248-289: The
appInjectedMsgAnteHandlemethod correctly processes app-injected messages without unnecessary steps like gas consumption or signature verification. This optimization is appropriate given the nature of app-injected messages.- 291-382: The
otherMsgAnteHandlemethod processes non-clob and non-app injected messages through the necessary ante decorators. The locking strategy, including the use of a global lock for certain operations, is consistent with the overall approach to concurrency management. It's important to ensure that this method does not introduce unnecessary performance overhead.
Changelist
[OTE-151] Create ante handlers which parallelize processing per account until a global lock is necessary.
Test Plan
Added tests which execute CheckTx's concurrently and rely on
-racefor primary validation for data races.Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.