fix(mempool): avoid concurrent map read and write from priority nonce mempool #21379
Conversation
WalkthroughWalkthroughThe changes involve enhancements to the mempool module, focusing on improving concurrency handling and data structures for transaction prioritization. A new 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 Documentation and Community
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for k, v := range i.priorityCounts { | ||
| counts[k] = v | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
![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
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (6)
- CHANGELOG.md (1 hunks)
- scripts/build/localnet.mk (1 hunks)
- types/mempool/priority_index.go (1 hunks)
- types/mempool/priority_index_test.go (1 hunks)
- types/mempool/priority_nonce.go (17 hunks)
- types/mempool/priority_nonce_test.go (2 hunks)
Files skipped from review due to trivial changes (1)
- scripts/build/localnet.mk
Additional context used
Path-based instructions (5)
types/mempool/priority_index.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.types/mempool/priority_index_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"types/mempool/priority_nonce.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.types/mempool/priority_nonce_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"
GitHub Check: CodeQL
types/mempool/priority_index.go
[warning] 113-115: Iteration over map
Iteration over map may be a possible source of non-determinism
Additional comments not posted (34)
types/mempool/priority_index.go (12)
9-15: LGTM!The
ConcurrentListElementstruct is well-designed for concurrent access.
17-31: LGTM!The
Nextfunction is correctly implemented with proper use of read locks for concurrency.
43-50: LGTM!The
ConcurrentSkipListstruct is well-structured for concurrent priority indexing.
52-66: LGTM!The
newConcurrentPriorityIndexfunction is correctly implemented with optional priority tracking.
68-74: LGTM!The
Lenfunction is correctly implemented with a read lock for concurrency.
76-90: LGTM!The
Frontfunction is correctly implemented with a read lock for concurrency.
92-101: LGTM!The
GetCountfunction is correctly implemented with a read lock for concurrency.
103-117: LGTM!The
CloneCountsfunction is correctly implemented with a read lock for concurrency.Tools
GitHub Check: CodeQL
[warning] 113-115: Iteration over map
Iteration over map may be a possible source of non-determinism
119-130: LGTM!The
GetScorefunction is correctly implemented with a read lock for concurrency.
132-146: LGTM!The
Getfunction is correctly implemented with a read lock for concurrency.
148-174: LGTM!The
Setfunction is correctly implemented with a write lock for concurrency.
176-188: LGTM!The
Removefunction is correctly implemented with a write lock for concurrency.types/mempool/priority_index_test.go (10)
11-38: LGTM!The
TestConcurrentPriorityNode_Nextfunction provides comprehensive test coverage for theNextmethod.
40-51: LGTM!The
TestConcurrentPriorityIndex_Lenfunction provides correct test coverage for theLenmethod.
53-65: LGTM!The
TestConcurrentPriorityIndex_Frontfunction provides correct test coverage for theFrontmethod.
67-89: LGTM!The
TestConcurrentSkipList_GetCountfunction provides comprehensive test coverage for theGetCountmethod.
91-114: LGTM!The
TestConcurrentSkipList_CloneCountsfunction provides comprehensive test coverage for theCloneCountsmethod.
116-136: LGTM!The
TestConcurrentSkipList_GetScorefunction provides comprehensive test coverage for theGetScoremethod.
138-160: LGTM!The
TestConcurrentSkipList_Getfunction provides comprehensive test coverage for theGetmethod.
162-196: LGTM!The
TestConcurrentSkipList_Setfunction provides comprehensive test coverage for theSetmethod.
198-228: LGTM!The
TestConcurrentSkipList_Removefunction provides comprehensive test coverage for theRemovemethod.
230-263: LGTM!The
TestConcurrentPriorityIndex_Concurrentfunction provides comprehensive test coverage for concurrent access.types/mempool/priority_nonce.go (10)
55-57: LGTM!The
PriorityNonceMempoolstruct changes align with the objective to improve concurrency handling.
64-65: LGTM!The
PriorityNonceIteratorstruct changes align with the objective to improve concurrency handling.
164-166: LGTM!The
NewPriorityMempoolfunction changes align with the objective to improve concurrency handling.
Line range hint
222-252: LGTM!The
Insertfunction changes align with the objective to improve concurrency handling.
Line range hint
269-282: LGTM!The
iteratePriorityfunction changes align with the objective to improve concurrency handling.
Line range hint
284-307: LGTM!The
Nextfunction changes align with the objective to improve concurrency handling.
Line range hint
364-379: LGTM!The
reorderPriorityTiesfunction changes align with the objective to improve concurrency handling.
Line range hint
388-406: LGTM!The
senderWeightfunction changes align with the objective to improve concurrency handling.
Line range hint
427-437: LGTM!The
Removefunction changes align with the objective to improve concurrency handling.
Line range hint
450-465: LGTM!The
IsEmptyfunction changes align with the objective to improve concurrency handling.Tools
GitHub Check: CodeQL
[warning] 451-455: Iteration over map
Iteration over map may be a possible source of non-determinismtypes/mempool/priority_nonce_test.go (1)
387-408: Concurrency handling looks good!The use of
sync.WaitGroupto manage the concurrent insertion of transactions is appropriate and ensures that the test waits for the goroutine to complete. This enhances the robustness of the test by preventing race conditions.However, consider verifying if additional test cases are needed to cover edge cases related to concurrency.
CHANGELOG.md (1)
267-267: Changelog entry looks good.The entry is clear, concise, and accurately describes the fix related to concurrent map access in the mempool.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- types/mempool/priority_nonce.go (16 hunks)
Files skipped from review as they are similar to previous changes (1)
- types/mempool/priority_nonce.go
| require.Equal(t, tt.txs[tx.id].p, int(tx.priority)) | ||
| require.Equal(t, tt.txs[tx.id].n, int(tx.nonce)) | ||
| require.Equal(t, tt.txs[tx.id].a, tx.address) | ||
| if tx.id < len(tt.txs) { |
There was a problem hiding this comment.
why is this condition needed?
There was a problem hiding this comment.
just to ensure the originally inserted transactions are validated, it's mainly to reproduce concurrent read and write.
since priorityCounts, priorityScores, priorityIndex, element from Next need lock together, it's harder to go wrong when wrap to handle in Concurrent struct |
|
I think the issue is more than data races, the iterator should see a consistent view when there are concurrent modifications, so I'd suggest we simply lock the mempool when starting iteration, blocking any concurrent modifications during iteration, and release the lock when done, that means we need a |
If API breaking change is inevitable, I'd suggest to change the func (mp *PriorityNonceMempool[C]) Select(callback func(Tx) bool) {
mp.mtx.Lock()
defer mp.mtx.Unlock()
for tx in ordered index {
if !callback(tx) {
break
}
}
}EDIT: did an implementation here: #21413 |
|
As the alternative PR (#21413) has been approved by @kocubinski, let's close this. Thank you! |
Description
There is a chance that concurrent map read and write occurs when optimistic execution is enabled:
goroutine 511 [running]:
github.com/cosmos/cosmos-sdk/types/mempool.(*PriorityNonceIterator[...]).Next(0x741ef80)
github.com/cosmos/cosmos-sdk/types/mempool/priority_nonce.go:341 +0x188
github.com/cosmos/cosmos-sdk/types/mempool.(*PriorityNonceIterator[...]).iteratePriority(0x401a533ef0?)
github.com/cosmos/cosmos-sdk/types/mempool/priority_nonce.go:310 +0x134
github.com/cosmos/cosmos-sdk/types/mempool.(*PriorityNonceIterator[...]).Next(0x741ef80)
github.com/cosmos/cosmos-sdk/types/mempool/priority_nonce.go:329 +0x288
github.com/crypto-org-chain/cronos/v2/app.New.func1.(*DefaultProposalHandler).PrepareProposalHandler.3({{0x740f028, 0x98e71e0}, {0xffff3cdc8da0, 0x401af5e640}, {{0x0, 0x0}, {0x4000ac19b0, 0xc}, 0xb7, {0x571bf59, ...}, ...}, ...}, ...)
github.com/cosmos/cosmos-sdk/baseapp/abci_utils.go:356 +0x658
github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).PrepareProposal(0x4001cfc6c8, 0x4002d8e160)
github.com/cosmos/cosmos-sdk/baseapp/abci.go:431 +0x94c
github.com/cosmos/cosmos-sdk/server.cometABCIWrapper.PrepareProposal(...)
github.com/cosmos/cosmos-sdk/server/cmt_abci.go:36
github.com/cometbft/cometbft/abci/client.(*localClient).PrepareProposal(0x401dc16000?, {0x740f450?, 0x98e71e0?}, 0xffff3d680288?)
github.com/cometbft/cometbft/abci/client/local_client.go:157 +0xe8
github.com/cometbft/cometbft/proxy.(*appConnConsensus).PrepareProposal(0x4002d8c0f0, {0x740f450, 0x98e71e0}, 0x4002d8e160)
github.com/cometbft/cometbft/proxy/app_conn.go:84 +0x130
github.com/cometbft/cometbft/state.(*BlockExecutor).CreateProposalBlock(, {, _}, _, {{{0xb, 0x0}, {0x40016ea7c0, 0x7}}, {0x40016ea7d0, 0xc}, ...}, ...)
github.com/cometbft/cometbft/state/execution.go:129 +0x5d8
github.com/cometbft/cometbft/consensus.(*State).createProposalBlock(0x4001889888, {0x740f450, 0x98e71e0})
github.com/cometbft/cometbft/consensus/state.go:1307 +0x190
github.com/cometbft/cometbft/consensus.(*State).defaultDecideProposal(0x4001889888, 0xb7, 0x1)
github.com/cometbft/cometbft/consensus/state.go:1214 +0x50
github.com/cometbft/cometbft/consensus.(*State).enterPropose(0x4001889888, 0xb7, 0x1)
github.com/cometbft/cometbft/consensus/state.go:1193 +0x794
github.com/cometbft/cometbft/consensus.(*State).enterNewRound(0x4001889888, 0xb7, 0x1)
github.com/cometbft/cometbft/consensus/state.go:1112 +0x9a4
github.com/cometbft/cometbft/consensus.(*State).handleTimeout(0x4001889888, {0x0?, 0x0?, 0x0?, 0x0?}, {0xb7, 0x0, 0x6, {0x2cbdf087, 0xede573d07, ...}, ...})
github.com/cometbft/cometbft/consensus/state.go:1008 +0x838
github.com/cometbft/cometbft/consensus.(*State).receiveRoutine(0x4001889888, 0x0)
github.com/cometbft/cometbft/consensus/state.go:865 +0x4b4
created by github.com/cometbft/cometbft/consensus.(*State).OnStart in goroutine 326
github.com/cometbft/cometbft/consensus/state.go:398 +0xf0
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
New Features
Bug Fixes
Tests
Documentation