refactor(mempool)!: match server/v2/cometbft and sdk mempool interface #21744
Conversation
julienrbrt
requested review from
a team,
kocubinski,
facundomedica,
sontrinh16,
aaronc and
testinginprod
as code owners
WalkthroughWalkthroughThe pull request introduces significant updates to the Cosmos SDK, primarily focusing on the Changes
Possibly related PRs
Suggested labels
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
julienrbrt
added
the
backport/v0.52.x
| @@ -14,10 +14,10 @@ type Mempool interface { | |||
|
|
|||
| // Select returns an Iterator over the app-side mempool. If txs are specified, | |||
| // then they shall be incorporated into the Iterator. The Iterator is not thread-safe to use. | |||
| Select(context.Context, [][]byte) Iterator | |||
| Select(context.Context, []sdk.Tx) Iterator | |||
There was a problem hiding this comment.
As otherwise in server/v2 we would decode twice if the mempool implementation decodes the txs (ours do not, but we cannot know about users):
https://github.com/cosmos/cosmos-sdk/blob/95b8092/server/v2/cometbft/handlers/defaults.go#L52
https://github.com/cosmos/cosmos-sdk/blob/95b8092/server/v2/cometbft/handlers/defaults.go#L72
There was a problem hiding this comment.
Should we use a generic instead of either []byte or sdk.Tx? if we use sdk.Tx how will this work with server/v2?
There was a problem hiding this comment.
err.. this interface is only used for app v1 correct?
There was a problem hiding this comment.
Yea, the one of server/v2 is identical but use generic
Co-authored-by: Marko <[email protected]>
Co-authored-by: Marko <[email protected]>
github-actions
bot
added
C:server/v2
Co-authored-by: Marko <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (4)
server/v2/cometbft/internal/mock/mock_mempool.go (1)
20-20: LGTM! Consider adding error handling.The modification to the
Removefunction signature to accept a single transaction instead of a slice simplifies the removal process and enhances usability.However, the function always returns nil, suggesting that it does not handle any errors that may occur during the removal process. Consider adding error handling to ensure that any failures during the removal process are properly propagated to the caller.
server/v2/cometbft/mempool/noop.go (2)
7-8: Remove the empty line between import statements.To conform to the Uber Go Style Guide, group the import statements into two groups: standard library imports and external imports, with a single empty line between the two groups. Remove the empty line between the two external import statements.
Apply this diff to fix the issue:
import ( "context" - sdk "github.com/cosmos/cosmos-sdk/types" )
23-24: Consider modifying theSelectBymethod to return a slice of transactions.The
SelectBymethod currently does not return anything, which deviates from the common practice of returning a slice of transactions that satisfy the predicate function. Consider modifying the method to return a slice of transactions of typeTthat satisfy the predicate function, for consistency with common practices and potential future use cases.Apply this diff to modify the
SelectBymethod:-func (NoOpMempool[T]) SelectBy(context.Context, []T, func(T) bool) {} +func (NoOpMempool[T]) SelectBy(context.Context, []T, func(T) bool) []T { return nil }CHANGELOG.md (1)
Line range hint
1-1: Consider adding more context.
The postHandler API change is described clearly, but it would help to also briefly mention:
- The motivation for adding this new handler
- How it impacts existing apps that don't utilize it
This added context will help devs better assess the scope of this breaking change.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (14)
- CHANGELOG.md (2 hunks)
- baseapp/abci_utils.go (2 hunks)
- baseapp/abci_utils_test.go (1 hunks)
- server/v2/cometbft/abci.go (1 hunks)
- server/v2/cometbft/handlers/defaults.go (1 hunks)
- server/v2/cometbft/internal/mock/mock_mempool.go (1 hunks)
- server/v2/cometbft/mempool/mempool.go (1 hunks)
- server/v2/cometbft/mempool/noop.go (2 hunks)
- simapp/v2/go.mod (1 hunks)
- simapp/v2/simdv2/cmd/config.go (1 hunks)
- types/mempool/mempool.go (1 hunks)
- types/mempool/noop.go (1 hunks)
- types/mempool/priority_nonce.go (2 hunks)
- types/mempool/sender_nonce.go (2 hunks)
Files skipped from review due to trivial changes (1)
- server/v2/cometbft/handlers/defaults.go
Additional context used
Path-based instructions (12)
server/v2/cometbft/internal/mock/mock_mempool.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.types/mempool/noop.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/mempool/noop.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.types/mempool/mempool.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/mempool/mempool.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.types/mempool/sender_nonce.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.types/mempool/priority_nonce.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.baseapp/abci_utils.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.baseapp/abci_utils_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"CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Additional comments not posted (28)
types/mempool/noop.go (2)
20-20: LGTM!The change in the
Selectmethod signature to accept[]sdk.Txinstead of[][]bytealigns with the overall refactoring goal and improves type safety and code clarity. The no-op implementation still honors theMempoolinterface contract.
21-21: LGTM!The change in the
SelectBymethod signature to accept[]sdk.Txinstead of[][]bytealigns with the overall refactoring goal and improves type safety and code clarity. The no-op implementation still honors theMempoolinterface contract by accepting a filtering function.server/v2/cometbft/mempool/noop.go (3)
11-11: LGTM!The type assertions correctly verify that the
NoOpMempooltype implements theMempoolinterface for bothsdk.Txandtransaction.Txtypes at compile time, preventing potential runtime errors.
22-22: LGTM!The
Insertmethod is correctly implemented as a no-op, returningnilwithout performing any action. This aligns with the purpose of theNoOpMempooltype to discard and ignore transactions.
25-26: LGTM!The
CountTxandRemovemethods are correctly implemented as no-ops, consistent with the purpose of theNoOpMempooltype. TheCountTxmethod always returns 0, indicating an empty mempool, and theRemovemethod returnsnil, suggesting a successful removal operation, even though no actual removal takes place.types/mempool/mempool.go (2)
17-17: Clarify the impact on server/v2 and the use of generics.The change from
[][]byteto[]sdk.Tximproves type safety and clarity. However, please address the following:
What is the impact of this change on server/v2? The past comments indicate concerns about decoding transactions twice.
Should generics be used instead of
sdk.Txto align with server/v2's interface?Is this interface only used for app v1 as mentioned in the past comments?
20-20: Address the same concerns as theSelectmethod.The change from
[][]byteto[]sdk.Tximproves type safety and clarity, similar to theSelectmethod. Please address the same concerns raised for theSelectmethod regarding the impact on server/v2, the use of generics, and the applicability to app v1.server/v2/cometbft/mempool/mempool.go (4)
22-22: Excellent clarification about the thread-safety of the iterator.The updated comment provides a clear warning that the returned iterator is not thread-safe. This information is crucial for developers to ensure they handle the iterator correctly in concurrent scenarios and avoid potential race conditions or unexpected behavior.
25-26: Great addition of the thread-safeSelectBymethod.The new
SelectBymethod is a valuable addition to theMempoolinterface, providing a thread-safe way to iterate over the mempool using a callback function. This method addresses the thread-safety concerns of theSelectmethod and offers developers a safer alternative for processing transactions concurrently.The method signature is clear, follows the established naming convention, and the documentation adequately explains its functionality and thread-safety.
28-29: Useful addition of theCountTxmethod.The new
CountTxmethod is a handy addition to theMempoolinterface, providing a simple way to retrieve the current number of transactions in the mempool. This method is particularly useful for monitoring purposes and making decisions based on the transaction count, such as triggering batch processing or adjusting resource allocation.The method signature is clear, follows the established naming convention, and the documentation adequately explains its functionality.
33-33: Appropriate simplification of theRemovemethod signature.The modification of the
Removemethod signature to accept a single transaction instead of a slice of transactions is a sensible change. This simplification aligns with the common use case of removing individual transactions from the mempool and makes the method more focused and easier to use.The change maintains the original error handling behavior by returning an error upon failure, ensuring consistency with the previous implementation.
simapp/v2/simdv2/cmd/config.go (1)
97-98: Mempool interface update looks good, but ensure it's well-tested.The change to use
sdkmempool.NewSenderNonceMempoolfor initializing the mempool aligns with the PR objective of matching the SDK mempool interface. This is a positive step towards consistency and resolving previous discrepancies.However, given the potential impact on transaction processing, it's crucial to ensure that the new mempool implementation has been thoroughly tested and validated. Please confirm if adequate tests have been added to cover various scenarios and edge cases.
types/mempool/sender_nonce.go (3)
170-173: Approve the change in function signature.The modification to accept
[]sdk.Txinstead of[][]byteimproves type safety and code clarity by leveraging a more structured transaction type. This change enhances the robustness of the code while maintaining the existing locking mechanism and selection behavior.
Line range hint
176-197: Approve the change in function signature.The modification to accept
[]sdk.Txinstead of[][]bytealigns with the change in theSelectfunction and maintains consistency. The core logic of creating an ordered list of senders and initializing sender cursors remains unaltered. The function continues to return asenderNonceMempoolIteratorinstance with the same set of fields, preserving its original behavior.
Line range hint
205-212: Approve the change in function signature.The modification to accept
[]sdk.Txinstead of[][]bytealigns with the changes in theSelectanddoSelectfunctions, maintaining consistency throughout the codebase. The function preserves its original behavior of iterating over the transactions using the obtained iterator and invoking the provided callback function for each transaction. The locking mechanism ensures thread safety during the selection and iteration process.simapp/v2/go.mod (1)
38-38: LGTM!Adding
github.com/spf13/castas an indirect dependency is fine. It's a well-known and widely used Go package for safe type casting.types/mempool/priority_nonce.go (3)
364-367: LGTM!The change in the parameter type from
[][]byteto[]sdk.Txenhances type safety and clarity by ensuring that the method directly works with transaction objects rather than raw byte representations. The overall functionality remains intact.
Line range hint
370-382: LGTM!The change in the parameter type from
[][]byteto[]sdk.Txis consistent with the changes made to theSelectmethod. The overall functionality remains intact, as the passed in parameters are ignored.
Line range hint
386-393: LGTM!The change in the parameter type from
[][]byteto[]sdk.Txis consistent with the changes made to theSelectanddoSelectmethods. The overall functionality remains intact, as the method iterates over the selected transactions and invokes the callback function for each transaction.server/v2/cometbft/abci.go (1)
470-473: LGTM, but consider adding a TODO for concurrent removal.The changes to remove transactions one by one from the mempool look good and align with the
types/mempool.Mempoolinterface from the SDK.However, as mentioned in the previous review comments, removing transactions sequentially could potentially block. Consider adding a TODO comment to revisit this in the future and explore removing transactions concurrently to avoid blocking.
baseapp/abci_utils.go (3)
267-276: Refactoring to decode transactions before processing improves code clarity.Decoding all transactions at the beginning and storing them in a
decodedTxsslice enhances code clarity by separating the concerns of decoding transactions from their subsequent selection for proposals.
Line range hint
284-292: Using decoded transactions when mempool is nil or no-op reduces redundancy.The updated logic iterates over the pre-decoded transactions (
decodedTxs) instead of decoding them again when the mempool is nil or a no-op. This change reduces redundancy and improves performance by ensuring that each transaction is decoded only once.
Line range hint
300-369: Selecting transactions from mempool using decoded transactions streamlines the process.The selection of transactions for proposals now uses the decoded transactions, which streamlines the process and maintains the integrity of the transaction data being processed.
baseapp/abci_utils_test.go (1)
694-694: LGTM!The added mock expectation for the
TxDecodemethod enhances the reliability of the test by simulating the transaction decoding process correctly. This ensures that theNewDefaultProposalHandlerbehavior is validated properly.CHANGELOG.md (4)
Line range hint
1-1: Looks good! The unreleased improvements are documented clearly.
Line range hint
1-1: Breaking change documented well.
The x/gov state machine breaking change is captured with the right level of detail.
Line range hint
1-1: Bug fixes look good!
The entries clearly capture the bugs that were resolved, with good links to the PR for more details.
Line range hint
1-1: Deprecation notice is clear.
The deprecated function is called out with instructions on what to use instead. Looks good.
| @@ -15,5 +15,6 @@ type MockMempool[T transaction.Tx] struct{} | |||
|
|
|||
| func (MockMempool[T]) Insert(context.Context, T) error { return nil } | |||
| func (MockMempool[T]) Select(context.Context, []T) mempool.Iterator[T] { return nil } | |||
| func (MockMempool[T]) SelectBy(context.Context, []T, func(T) bool) {} | |||
There was a problem hiding this comment.
Complete the implementation of the SelectBy function.
The SelectBy function has been added to facilitate the selection of transactions based on a predicate. However, the current implementation is empty.
Please complete the function logic to ensure it behaves as expected when called. Consider iterating over the input transactions and applying the predicate to each transaction to determine which ones should be selected.
There was a problem hiding this comment.
LGTM;
Although I think having the Remove step (or any other step) accept multiple txs might be a good idea to avoid locking the mempool multiple times.
It is also true that the mempool could implement its own batching mechanism for multiple txs to avoid multiple lokcs and remove txs in batches.
#21744) Co-authored-by: Marko <[email protected]> (cherry picked from commit 356df96) # Conflicts: # CHANGELOG.md # simapp/v2/go.mod
…e (backport #21744) (#21803) Co-authored-by: Julien Robert <[email protected]>
Description
ref: #21741 (comment)
Make the server/v2/cometbft/mempool interface and sdk mempool interface match.
#21741 must be merged BEFORE.
I'll add changelog if this is accepted.
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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...
Summary by CodeRabbit