fix(abcipp): reject mempool insert no-ops#514
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMempool insertion now returns explicit error reasons for rejected transactions; spec text clarifies rejection semantics for lower-priority same-(sender, nonce) candidates and queued capacity edge cases. Call sites and tests were updated to propagate and validate the new error behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
abcipp/mempool_test.go (1)
294-314: This test currently validates active replacement, not queued replacement.At Line 301 and Line 306, nonce
0targets the active path. Consider adding a queued-nonce variant (e.g., nonce2with a gap) to explicitly coverinsertQueuedLockedlower-priority rejection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abcipp/mempool_test.go` around lines 294 - 314, The test TestLowerPriorityReplacementRejected currently uses nonce 0 which exercises the active replacement path; change it to also add a queued-nonce variant to cover insertQueuedLocked behavior by creating a second low-priority tx with a non-contiguous nonce (e.g., nonce 2 while sequence remains 0) instead of nonce 0, insert it with lower priority (using newTestTxWithPriv and mp.Insert with sdkCtx.WithPriority(10)), and assert that the insert is rejected with sdkerrors.ErrTxInMempoolCache and that the mempool contents/count and event expectations remain unchanged; reference the TestLowerPriorityReplacementRejected test, newTestTxWithPriv helper, and the insertQueuedLocked path to locate where to add this queued-nonce case.abcipp/mempool_remove.go (1)
176-182: Capture demotion insert errors instead of discarding them.At Line 176, the ignored error makes demotion failures opaque. Handling it explicitly will improve diagnosability without changing behavior.
Suggested refactor
- inserted, evicted, _ := p.insertQueuedLocked(ss, entry.key, entry) + inserted, evicted, insertErr := p.insertQueuedLocked(ss, entry.key, entry) + if insertErr != nil { + // demotion fallback keeps current behavior: tx is removed if not re-queued + removed = append(removed, entry) + continue + } if evicted != nil { removed = append(removed, evicted) } if !inserted { removed = append(removed, entry) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abcipp/mempool_remove.go` around lines 176 - 182, The call to p.insertQueuedLocked currently ignores its error return making demotion insert failures invisible; change the assignment to capture the error (e.g., "inserted, evicted, err := p.insertQueuedLocked(ss, entry.key, entry)"), and then handle err explicitly: if err != nil, append the original entry to removed (like when !inserted) and record/log the error for diagnostics (preserving existing behavior for inserted/evicted). Update any surrounding control flow that checks inserted/evicted to account for the new err variable and ensure the function name insertQueuedLocked, variables inserted, evicted, err, entry and removed are used to locate and modify the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@abcipp/mempool_remove.go`:
- Around line 176-182: The call to p.insertQueuedLocked currently ignores its
error return making demotion insert failures invisible; change the assignment to
capture the error (e.g., "inserted, evicted, err := p.insertQueuedLocked(ss,
entry.key, entry)"), and then handle err explicitly: if err != nil, append the
original entry to removed (like when !inserted) and record/log the error for
diagnostics (preserving existing behavior for inserted/evicted). Update any
surrounding control flow that checks inserted/evicted to account for the new err
variable and ensure the function name insertQueuedLocked, variables inserted,
evicted, err, entry and removed are used to locate and modify the code.
In `@abcipp/mempool_test.go`:
- Around line 294-314: The test TestLowerPriorityReplacementRejected currently
uses nonce 0 which exercises the active replacement path; change it to also add
a queued-nonce variant to cover insertQueuedLocked behavior by creating a second
low-priority tx with a non-contiguous nonce (e.g., nonce 2 while sequence
remains 0) instead of nonce 0, insert it with lower priority (using
newTestTxWithPriv and mp.Insert with sdkCtx.WithPriority(10)), and assert that
the insert is rejected with sdkerrors.ErrTxInMempoolCache and that the mempool
contents/count and event expectations remain unchanged; reference the
TestLowerPriorityReplacementRejected test, newTestTxWithPriv helper, and the
insertQueuedLocked path to locate where to add this queued-nonce case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 99f34427-7756-4ebf-8095-b2983e8b6648
📒 Files selected for processing (4)
abcipp/docs/spec.mdabcipp/mempool_insert.goabcipp/mempool_remove.goabcipp/mempool_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #514 +/- ##
==========================================
+ Coverage 38.08% 38.17% +0.09%
==========================================
Files 325 325
Lines 30817 30840 +23
==========================================
+ Hits 11737 11774 +37
+ Misses 17186 17176 -10
+ Partials 1894 1890 -4
🚀 New features to boost your workflow:
|
traviolus
approved these changes
Apr 22, 2026
Contributor
traviolus
left a comment
traviolus
left a comment
There was a problem hiding this comment.
LGTM just a minor issue there
| if insertErr != nil { | ||
| return insertErr | ||
| } | ||
| return sdkmempool.ErrMempoolTxMaxCapacity |
Contributor
There was a problem hiding this comment.
I think this is unreachable since every !inserted path in insertQueuedLocked sets insertErr
Member
Author
There was a problem hiding this comment.
I think good to keep this as fallback just in case for future!
beer-1
force-pushed
the
fix/abcipp-mempool-insert-rejects
branch
from
c743954 to
cde40b3
Compare
beer-1
force-pushed
the
fix/abcipp-mempool-insert-rejects
branch
from
cde40b3 to
2cae5fc
Compare
beer-1
force-pushed
the
fix/abcipp-mempool-insert-rejects
branch
2 times, most recently
from
29f241f to
c743954
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
abcipp/checktx.go (1)
121-127: Compute recheck hash once for consistency/readability.
TxHash(req.Tx)is computed twice in this block; caching it locally avoids duplicate work and keeps compare/log values identical by construction.♻️ Small refactor
- if currentHash != TxHash(req.Tx) { + recheckHash := TxHash(req.Tx) + if currentHash != recheckHash { h.logger.Debug( "tx from comet mempool was replaced in app-side mempool", "tx", tx, "current_hash", currentHash, - "recheck_hash", TxHash(req.Tx), + "recheck_hash", recheckHash, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abcipp/checktx.go` around lines 121 - 127, Compute TxHash(req.Tx) once into a local variable (e.g., recheckHash) and use that variable for both the comparison and the logger.Debug call to avoid duplicate computation and ensure identical values in the message; update the block where currentHash is compared to TxHash(req.Tx) (referencing currentHash, TxHash, req.Tx and logger.Debug) to use the new recheckHash variable instead.abcipp/mempool_test.go (1)
294-314: Consider adding a queued-lower-priority replacement test case.This test covers lower-priority rejection for same-nonce replacement on the active path. A companion case for future nonce queued replacement would directly lock in coverage for
insertQueuedLocked’s lower-priority rejection branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@abcipp/mempool_test.go` around lines 294 - 314, Add a companion test that exercises the queued-path lower-priority replacement rejection: create a mempool via newTestMempoolWithEvents, set the sender sequence to a higher value so the first tx (txHigh) uses a future nonce and is inserted into the queue, then attempt to insert a second tx with the same future nonce but lower priority using mp.Insert so that insertQueuedLocked is exercised and returns the lower-priority rejection (assert error is sdkerrors.ErrTxInMempoolCache); mirror the assertions in TestLowerPriorityReplacementRejected (check no events removed/inserted, original tx remains in queue, and CountTx behaves as expected) and name the test e.g. TestLowerPriorityReplacementQueuedRejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@abcipp/checktx.go`:
- Around line 121-127: Compute TxHash(req.Tx) once into a local variable (e.g.,
recheckHash) and use that variable for both the comparison and the logger.Debug
call to avoid duplicate computation and ensure identical values in the message;
update the block where currentHash is compared to TxHash(req.Tx) (referencing
currentHash, TxHash, req.Tx and logger.Debug) to use the new recheckHash
variable instead.
In `@abcipp/mempool_test.go`:
- Around line 294-314: Add a companion test that exercises the queued-path
lower-priority replacement rejection: create a mempool via
newTestMempoolWithEvents, set the sender sequence to a higher value so the first
tx (txHigh) uses a future nonce and is inserted into the queue, then attempt to
insert a second tx with the same future nonce but lower priority using mp.Insert
so that insertQueuedLocked is exercised and returns the lower-priority rejection
(assert error is sdkerrors.ErrTxInMempoolCache); mirror the assertions in
TestLowerPriorityReplacementRejected (check no events removed/inserted, original
tx remains in queue, and CountTx behaves as expected) and name the test e.g.
TestLowerPriorityReplacementQueuedRejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ce22b39f-0080-4337-ac9a-4c1baec3f5f7
📒 Files selected for processing (6)
abcipp/checktx.goabcipp/checktx_test.goabcipp/docs/spec.mdabcipp/mempool_insert.goabcipp/mempool_remove.goabcipp/mempool_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- abcipp/docs/spec.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@abcipp/docs/spec.md`:
- Line 46: Update the spec text describing "nonce > nextExpected" queued inserts
to also document explicit rejection paths: state that if the candidate itself is
the highest nonce for that sender it may be rejected rather than evicting (the
per-sender eviction only applies when a lower-nonce candidate can be favored),
and that insertion can be rejected when global queued capacity (MaxQueuedTotal)
is already full; reference the same semantics implemented in the mempool insert
logic (mempool_insert.go / the insertion routine and the MaxQueuedTotal and
per-sender queued limit checks) so readers know these are legitimate reject
branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 560bf460-082e-465a-bc59-6fb2697b791c
📒 Files selected for processing (4)
abcipp/docs/spec.mdabcipp/mempool_insert.goabcipp/mempool_remove.goabcipp/mempool_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- abcipp/mempool_remove.go
- abcipp/mempool_test.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes: N/A
This PR makes
PriorityMempool.Insertreturn an error when a tx is not actually accepted into the app-side mempool. Previously, some no-op reject paths returnednil, which letBaseApp.CheckTxreport success. CometBFTProxyMempoolthen cached those txs inknownTxseven though Initia's app-side mempool did not contain them, leaving no later app-side remove event to clear the CometBFT cache.The changed paths are:
ErrTxInMempoolCacheErrMempoolTxMaxCapacityAuthor 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 change: not applicablemainValidation
Reviewers 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.
I have...