Reject prioritisetransaction on pooled dust like Core - #593
Conversation
Core v31 refuses to modify the fee overlay of a mempool transaction with dust outputs when require_standard is set. That flag defaults on except regtest. Absent txids are overlay-only and are not checked. Co-authored-by: metaphorics <metaphorics@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_467380bc-4962-44d5-aedd-d2c92a277b1b) |
PR Summary by QodoMatch Core dust rejection in prioritisetransaction
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1. Dust check is non-atomic
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // Core `require_standard` defaults on except regtest (`-acceptnonstdtxn`). | ||
| // Dust in the pool cannot have its fee overlay modified afterwards. |
There was a problem hiding this comment.
1. require_standard rule copied in comments 📘 Rule violation ⚙ Maintainability
The new comments restate the network-dependent standardness rule already defined by AcceptContext and the API-20 contract instead of referencing its authoritative definition. This creates another textual copy that can drift when standardness configuration is wired as a node option.
Agent Prompt
## Issue description
The comments duplicate the documented `require_standard` and pooled-dust business rules.
## Issue Context
`AcceptContext` already documents the network default, while `docs/contracts/external-api.md` defines the authoritative `API-20` behavior. Replace the restatement with a brief contract reference.
## Fix Focus Areas
- crates/rpc/src/handlers/mining.rs[179-180]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| #[test] | ||
| fn prioritisetransaction_rejects_dust_outputs_like_core() { |
There was a problem hiding this comment.
2. api-20 tests lack contract markers 📘 Rule violation ▣ Testability
The three new RPC tests and the modified dust-boundary test are listed as durable proof for API-20, but none identifies that contract in its test name or an adjacent test-level comment. Future maintainers cannot determine the documented contract from the tests themselves.
Agent Prompt
## Issue description
Tests serving as permanent proof for `API-20` do not carry an explicit contract marker.
## Issue Context
The contract documentation maps these tests to `API-20`, but rule 3086699 requires the test name, test-level comment, or metadata to reference the current contract directly. Add an adjacent `CONTRACT: API-20` comment or include `api_20` in each relevant test name.
## Fix Focus Areas
- crates/rpc/src/handlers/mining.rs[1577-1624]
- crates/mempool/src/standardness.rs[879-895]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| let pool = ctx.mempool.read(); | ||
| if let Some(entry) = pool.entry_by_txid(&txid) { | ||
| let dust_relay_fee = pool.policy_snapshot().standardness.dust_relay_fee; | ||
| if tx_has_dust_outputs(&entry.tx, dust_relay_fee) { |
There was a problem hiding this comment.
3. Dust check is non-atomic 🐞 Bug ≡ Correctness
prioritisetransaction releases the mempool read lock after checking for dust and then reacquires a separate write lock to apply the overlay. A reorg can resurrect a dust transaction between those operations, causing the RPC to successfully prioritise a pooled dust transaction that should return -8.
Agent Prompt
## Issue description
Make the pooled-dust check and fee-overlay update one atomic mempool operation. The current read-lock check followed by a separate write-lock prioritisation permits reorg resurrection to insert a dust transaction between them.
## Issue Context
`MempoolGateway::prioritise` currently acquires its own write lock. Add an operation that checks the entry under that same write guard and applies the overlay only if the entry is absent or non-dust, while preserving overflow and absent-txid behavior.
## Fix Focus Areas
- crates/rpc/src/handlers/mining.rs[179-192]
- crates/mempool/src/gateway.rs[775-780]
- crates/mempool/src/pool.rs[962-993]
- crates/node/src/reorg.rs[623-642]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| pub fn tx_has_dust_outputs(tx: &Tx, dust_relay_fee: u64) -> bool { | ||
| tx.outputs | ||
| .iter() | ||
| .any(|output| is_dust(output, dust_relay_fee)) |
There was a problem hiding this comment.
4. Dust threshold rounds down 🐞 Bug ≡ Correctness
The new tx_has_dust_outputs API delegates to minimal_non_dust, which floors fee × size / 1000, while Core v31's CFeeRate::GetFee rounds fractional fees up. For non-integral custom dust relay rates, boundary outputs can therefore be accepted and prioritised here even though Core classifies them as dust.
Agent Prompt
## Issue description
Update dust-threshold calculation to use Core v31's upward fee rounding rather than floor division, and add a boundary test using a dust relay rate whose product with the output size is not divisible by 1000.
## Issue Context
The default 3000 sat/kvB rate produces integral thresholds, masking the discrepancy. Preserve the zero threshold for unspendable outputs and avoid overflow while implementing ceiling division.
## Fix Focus Areas
- crates/script/src/script.rs[330-346]
- crates/mempool/src/standardness.rs[527-540]
- crates/mempool/src/standardness.rs[880-895]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo Fixer🍒 Ready to be cherry-picked — ✅ Merged (0) · ☑ Fixed (4) 🔗 Fix PR: #594 This fix PR was closed automatically. Its branch is preserved so you can cherry pick the changes into the original PR. Prompt for coding agent Process — 4 fixed
|
2 participants
Stacked on #589 (
cursor/mining-prioritise-dummy-1522). Merge order: #428 → #436 → #442 → #451 → #479 → #499 → #514 → #535 → #553 → #557 → #565 → #566 → #581 → #586 → #589 → this.Why
Core v31
prioritisetransactionrefuses to modify the fee overlay of a mempool transaction with dust outputs whenrequire_standardis set:Priority is not supported for transactions with dust outputs.require_standarddefaults on except regtest (-acceptnonstdtxn). Absent txids are overlay-only and are not checked.Contract (
API-20)Owner:
prioritisetransactionincrates/rpc/src/handlers/mining.rs. Dust classification istx_has_dust_outputsincrates/mempool/src/standardness.rs.Proof
prioritisetransaction_rejects_dust_outputs_like_coreprioritisetransaction_allows_dust_overlay_on_regtestprioritisetransaction_allows_absent_txid_overlaydust_relay_fee_changes_the_boundaryNot in this PR
-acceptnonstdtxnas a node option (still Core's network default)duplicate