feat(baseapp): avoid decoding tx in PrepareProposal when using NopMempool #24207
Conversation
* Problem: tx is decoded in PrepareProposal when using NopMempool
📝 WalkthroughWalkthroughThis pull request introduces a fast-path for transaction proposal preparation in the ABCI application. It adds a new flag and supporting methods to bypass the standard transaction selection when the mempool is a no-op or nil. This change allows the proposal handler to quickly return the incoming transactions without additional processing. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 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
|
mmsqe
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
baseapp/abci_utils.go (1)
255-259: Missing function documentationThe new constructor
NewDefaultProposalHandlerFastis missing a header comment to explain its purpose, unlike other exported functions in the file.+// NewDefaultProposalHandlerFast creates a new DefaultProposalHandler with fastPrepareProposal set to true, +// which optimizes PrepareProposal by avoiding tx decoding when using NoOpMempool. func NewDefaultProposalHandlerFast(mp mempool.Mempool, txVerifier ProposalTxVerifier) *DefaultProposalHandler { h := NewDefaultProposalHandler(mp, txVerifier) h.fastPrepareProposal = true return h }CHANGELOG.md (2)
51-53: Consider adding a blank line before list items [markdownlint MD032]The markdown linter suggests adding a blank line before starting the list items in the "Improvements" section. Following markdown best practices, lists should be surrounded by blank lines to ensure proper rendering across different markdown processors.
### Improvements + * [#24207](https://github.com/cosmos/cosmos-sdk/pull/24207) Avoid decoding tx for in PrepareProposal if it's NopMempool.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
52-52: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
53-53: Lists should be surrounded by blank lines
null(MD032, blanks-around-lists)
53-53: Fix minor typo in improvement descriptionThere's a small grammatical error in the description of improvement #24207.
-* [#24207](https://github.com/cosmos/cosmos-sdk/pull/24207) Avoid decoding tx for in PrepareProposal if it's NopMempool. +* [#24207](https://github.com/cosmos/cosmos-sdk/pull/24207) Avoid decoding tx in PrepareProposal if it's NopMempool.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
53-53: Lists should be surrounded by blank lines
null(MD032, blanks-around-lists)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)baseapp/abci_utils.go(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.go`: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
baseapp/abci_utils.go
`**/*.md`: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
CHANGELOG.md
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
53-53: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (7)
baseapp/abci_utils.go (6)
241-242: Field addition looks goodThe new field
fastPrepareProposalis properly documented and clearly explains its purpose to bypass tx selector when using NoOpMempool.
312-315: Implementation of fast path looks goodThe fastPrepareProposal condition is properly checked and applied only in the correct context (when mempool is nil or NoOp). This optimization avoids unnecessary transaction decoding.
520-524: Interface method is well documentedThe addition of
SelectTxForProposalFastto the TxSelector interface is properly documented with clear comments explaining its purpose and usage.
546-546: Good memory optimizationUsing
ts.selectedTxs[:0]instead of setting tonilpreserves the allocated memory, which is more efficient for repeated operations.
579-581: Simple and effective implementationThe implementation of
SelectTxForProposalFastcorrectly returns the input transactions without additional processing, which aligns with its purpose to provide a fast path.
312-324:❓ Verification inconclusive
Verify all calling code is updated
While the fast path implementation itself looks good, it's important to ensure that any code creating
DefaultProposalHandlerinstances is updated to useNewDefaultProposalHandlerFastwhere appropriate.
🏁 Script executed:
#!/bin/bash # Description: Find all instances where DefaultProposalHandler is created # and check if they might benefit from using NewDefaultProposalHandlerFast # Find all calls to NewDefaultProposalHandler echo "Checking calls to NewDefaultProposalHandler:" rg --type go "NewDefaultProposalHandler" --no-heading # Find places where NopMempool is used or checked echo -e "\nChecking where NopMempool is used:" rg --type go "NopMempool|NoOpMempool" --no-headingLength of output: 5724
Ensure All Usage Sites Leverage the Fast Proposal Handler
The fast-path logic in
baseapp/abci_utils.gois implemented correctly. However, our investigation shows that several instantiation points still useNewDefaultProposalHandlerinstead of the fast-construction helper. Please verify and update the following if applicable:
Production Code:
- In
baseapp/baseapp.go, the proposal handler is created usingNewDefaultProposalHandler. Confirm that the fast path should be enabled here, and update toNewDefaultProposalHandlerFastif so.Test Code:
- In
baseapp/abci_utils_test.goandserver/v2/cometbft/abci_test.go, instances ofDefaultProposalHandlerare still constructed via the old constructor. Verify whether tests should reflect the fast path changes.NoOpMempool Considerations:
- Ensure that the use cases involving
NoOpMempoolare intentionally bypassing the fast path or need to be updated as well.Please review each of these calling sites to ensure they correctly utilize the fast proposal handler where it benefits performance.
CHANGELOG.md (1)
1-3164: The CHANGELOG looks well-structured and comprehensiveThe CHANGELOG adequately documents the changes for this PR within the "Improvements" section. The PR is properly linked and the description concisely explains the change - avoiding transaction decoding in
PrepareProposalwhen usingNopMempool. This aligns with the PR objectives mentioned earlier.The overall structure of the CHANGELOG follows good practices with:
- Versioned headers with release dates
- Categorized changes (Features, Improvements, Bug Fixes, etc.)
- PR references with links to the corresponding GitHub PRs
- Brief but clear descriptions of the changes
This makes it easy for users to understand what's changed between versions.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
52-52: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
53-53: Lists should be surrounded by blank lines
null(MD032, blanks-around-lists)
93-93: Unordered list indentation
Expected: 4; Actual: 2(MD007, ul-indent)
94-94: Unordered list indentation
Expected: 4; Actual: 2(MD007, ul-indent)
95-95: Unordered list indentation
Expected: 4; Actual: 2(MD007, ul-indent)
366-366: Multiple headings with the same content
null(MD024, no-duplicate-heading)
376-376: Multiple headings with the same content
null(MD024, no-duplicate-heading)
418-418: Multiple headings with the same content
null(MD024, no-duplicate-heading)
885-885: Multiple headings with the same content
null(MD024, no-duplicate-heading)
1335-1335: Multiple headings with the same content
null(MD024, no-duplicate-heading)
2125-2125: Multiple headings with the same content
null(MD024, no-duplicate-heading)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
baseapp/abci_utils_test.go (1)
488-490: Good implementation of the fast proposal handler testThe code correctly instantiates the new fast proposal handler which is designed to bypass tx decoding when using NoOpMempool.
Consider adding a brief comment explaining the purpose of the fast handler:
+ // Create a fast proposal handler that avoids decoding txs when using NoOpMempool phf := baseapp.NewDefaultProposalHandlerFast(mempool.NoOpMempool{}, app) handlerFast := phf.PrepareProposalHandler()
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
baseapp/abci_utils_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.go`: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
baseapp/abci_utils_test.go
`**/*_test.go`: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
baseapp/abci_utils_test.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (1)
baseapp/abci_utils_test.go (1)
574-576: Verification logic looks goodThe test correctly verifies that the fast handler returns the original request transactions without modification, confirming it takes the expected fast path.
|
What is the use case for this feature or what is the bug that we are actually fixing with this? |
This allows faster prepare proposal when using NoOpMempool which avoid unnecessary TxDecode |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
53-53: Fix grammar in improvement description and ensure proper list formattingThe improvement description has a small grammatical issue - it includes an unnecessary "for" word. Also, according to markdown best practices (MD032), lists should be surrounded by blank lines.
- * [#24207](https://github.com/cosmos/cosmos-sdk/pull/24207) Avoid decoding tx for in PrepareProposal if it's NoOpMempool. + * [#24207](https://github.com/cosmos/cosmos-sdk/pull/24207) Avoid decoding tx in PrepareProposal if it's NoOpMempool.Additionally, ensure there's a blank line before and after this list of improvements to follow proper markdown formatting.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
53-53: Lists should be surrounded by blank lines
null(MD032, blanks-around-lists)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)baseapp/abci_utils.go(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.go`: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
baseapp/abci_utils.go
`**/*.md`: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
CHANGELOG.md
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
53-53: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (7)
baseapp/abci_utils.go (7)
241-242: Well-documented new feature flag.The
fastPrepareProposalfield with its descriptive comment makes it clear that this is used to bypass the tx selector when using NoOpMempool.
255-259: Clean constructor implementation for the fast proposal handler.Good approach to reuse the existing constructor and only modify the necessary field. This avoids code duplication and maintains a clear separation of concerns.
300-317: Good extraction of decoding logic.Extracting the transaction decoding logic into a separate function with caching mechanism is a performance improvement. This allows for conditional execution based on whether decoding is needed.
319-322: Fast-path implementation for proposal preparation.This is the core of the optimization - allowing direct processing of transactions without decoding when using NoOpMempool with the fast flag enabled. The implementation correctly leverages the new
SelectTxForProposalFastmethod.
535-539: Clear interface extension with good documentation.The
SelectTxForProposalFastmethod is well-documented, explaining its purpose and noting that CometBFT has already performed necessary checks. The comment also clarifies that extra validations are still possible.
594-596: Simple and efficient implementation of fast proposal selection.The implementation directly returns the provided transactions without additional processing, which aligns with the feature's purpose of skipping unnecessary decoding and validation when using NoOpMempool.
561-561: Good memory optimization.Using
ts.selectedTxs[:0]rather than creating a new slice preserves the allocated memory, which is more efficient than allocating a new slice, especially if the selector handles many transactions frequently.
|
Is there a case where you are a no-op mempool and you wouldnt want this behavior? Why not make this the default behavior when using a no-op mempool? |
Not for all cases, our case is based on the fact that cometbft has already checked the block gas/size limit before select so we can skip the tx selection logic. |
| // SelectTxForProposalFast is called in the case of NoOpMempool, | ||
| // where cometbft already checked the block gas/size limit, | ||
| // so the tx selector should simply accept them all to the proposal. | ||
| // But extra validations on the tx are still possible though. |
There was a problem hiding this comment.
// But extra validations on the tx are still possible though.
is it still possible because i can implement my own TxSelector? i'm a bit confused on this
There was a problem hiding this comment.
CHANGELOG.md
Outdated
| @@ -100,6 +100,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
| * (server) [#24072](https://github.com/cosmos/cosmos-sdk/pull/24072) Return BlockHeader by shallow copy in server Context. | |||
| * (x/bank) [#24053](https://github.com/cosmos/cosmos-sdk/pull/24053) Resolve a foot-gun by swapping send restrictions check in `InputOutputCoins` before coin deduction. | |||
| * (codec/types) [#24336](https://github.com/cosmos/cosmos-sdk/pull/24336) Most types definitions were moved to `github.com/cosmos/gogoproto/types/any` with aliases to these left in `codec/types` so that there should be no breakage to existing code. This allows protobuf generated code to optionally reference the SDK's custom `Any` type without a direct dependency on the SDK. This can be done by changing the `protoc` `M` parameter for `any.proto` to `Mgoogle/protobuf/any.proto=github.com/cosmos/gogoproto/types/any`. | |||
| * [#24207](https://github.com/cosmos/cosmos-sdk/pull/24207) Avoid decoding tx for in PrepareProposal if it's NoOpMempool. | |||
There was a problem hiding this comment.
this needs to go in the Unreleased section
Description
Closes: #XXXX
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