Skip to content

fix(mempool): compare RPC package fees exactly and preserve Core precedence - #1712

Merged
gosunuts merged 1 commit into
mainfrom
fix/package-fee-cap-and-precedence
Oct 29, 2026
Merged

gosunuts merged 1 commit into
mainfrom
fix/package-fee-cap-and-precedence

Conversation

@gosunuts

Copy link
Copy Markdown
Member

A submitted member can exceed maxfeerate by one satoshi while its displayed integer sat/kvB rate still equals the limit. RPC package underpayment can also be reported as a later cluster/dust failure.

Preserve only these remaining fixes from #1667 after the scoped submitpackage implementation landed in #1693:

  • Compare the current signed i128 modified fee exactly against max_rate * vsize / 1000. The nonnegative u64 * u32 limit fits i128; this avoids signed-fee multiplication overflow and works even when no u64 rate quote exists. This implements the strict rational comparison for integer fees.
  • For RPC package submission only, report aggregate MinRelayFeeNotMet after TRUC checks and before cluster/ephemeral-spend checks. Other planner errors and native/peer submission keep their existing precedence.

This is a fresh adaptation to current main, not a copy of #1667's unsigned/GetFee implementation. Pinned Core 31.1 directly compares CFeeRate(modified_fee, vsize) to the caller's rate in single/package submission, using FeeRateCompare cross-products. It does not apply GetFee's one-satoshi minimum to this comparison. The aggregate floor precedes cluster/dust checks. Existing non-RPC fee-rounding behavior is outside this change.

Regression evidence:

  • Both public regression tests failed before the fix: the 1,341-vB member with modified fee 1,342 sat was accepted under a 1,000-sat/kvB cap; the underfunded pair reported PackageCluster instead of MinRelayFeeNotMet.
  • New tests cover cap equality and ±1-sat boundaries, a sub-one-satoshi fractional allowance, explicit zero disabling, signed extremes/missing rate quotes, floor versus cluster/dust errors, unchanged capacity-error precedence, and refusal without membership/sequence/estimator/observer changes.
  • cargo test -p bitcoin-rs-mempool --lib package: 44 passed, including current RPC/native/peer package and snapshot CPFP proofs.
  • cargo clippy -p bitcoin-rs-mempool --lib --tests -- -D warnings: passed.
  • cargo fmt --all -- --check and git diff --check: passed.

No sync impact: this changes only policy decisions over captured mempool admission facts, before the existing stamped mutation boundary. It does not cross authoritative sync, chain validation, durable chainstate commit, retention, or lock-order boundaries; no SYNC-* clause is changed. No schema, protocol, counter, dependency, or RPC surface expansion. The Core evidence is source comparison plus local behavioral tests; a new Core process campaign was not run.

@amazon-q-developer amazon-q-developer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

This PR makes two targeted fixes to mempool RPC package fee policy:

  1. check_package_max_feerate (gateway.rs): Replaces the effective_fee_rate u64 rate-quote comparison with an exact signed i128 modified-fee cross-product comparison. The arithmetic is correct: u64 × u32 fits i128, the comparison is equivalent to Core's cross-product check for integer fees, and signed extremes (e.g., i128::MIN, i128::MAX) are handled correctly by the guard.

  2. finish_package (package.rs): Inserts a new early-return for PackageRpcSubmit mode that reports MinRelayFeeNotMet after TRUC checks and before cluster/dust/capacity checks. The singleton exemption logic (skipping the per-member cap when the aggregate floor already rejects a singleton) is also correctly structured.

Defect Found

One test assertion appears inconsistent with the new precedence rule. In rpc_package_fee_failure_precedence_preserves_other_planner_errors (package/tests.rs), the (dust=true, full=true) case asserts MissingEphemeralSpends as the package_error. However, the new block in finish_package (lines 211–216) returns MinRelayFeeNotMet before the dust or capacity checks are reached whenever aggregate_error == MinRelayFeeNotMet — which should still hold for the full=true case, since the aggregate fee floor error is computed independently of the capacity constraint. The test comment ("Capacity refusal retains its old position after dust checks") describes the intended behavior but is not matched by the new code path. This assertion should either be corrected to MinRelayFeeNotMet, or the precedence logic needs a guard to suppress the early return when capacity fails.


You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.

Comment on lines +814 to +822
assert_eq!(
result.package_error,
Some(if full {
// Capacity refusal retains its old position after dust checks.
AcceptanceRejectReason::MissingEphemeralSpends
} else {
AcceptanceRejectReason::MinRelayFeeNotMet
})
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛑 Logic Error: The test assertion for (dust=true, full=true) expects MissingEphemeralSpends, but the new MinRelayFeeNotMet precedence block in finish_package (lines 211–216) returns MinRelayFeeNotMet before the dust/capacity checks are reached. The aggregate_error passed to finish_package is the fee-floor result (MinRelayFeeNotMet), which is independent of the capacity constraint. With the new code, the result for full=true should be MinRelayFeeNotMet, not MissingEphemeralSpends. Either the test assertion is wrong, or the intended precedence requires that capacity failures suppress the MinRelayFeeNotMet early-return — but there is no such guard in the new code.

@gormes-agent

gormes-agent Bot commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Approve (comment only, not a formal review) — reviewed head ec830f3fe0a333f61f645ce232ed1f71e5b07306.

  • check_package_max_feerate: for integer fees, fee > max * vsize / 1000 (floor division) is equivalent to Core 31.1's CFeeRate cross-product comparison (fee * 1000 > max * vsize). The nonnegative u64 * u32 limit fits i128, so signed/extreme modified fees stay exact, and a missing u64 rate quote no longer disables the cap.
  • finish_package: the new RPC-only early return matches Core's AcceptMultipleTransactionsInternal order (TRUC → aggregate CheckFeeRate → package RBF → cluster → CheckEphemeralSpends); native/peer precedence is unchanged.
  • Re the Amazon Q note on package/tests.rs:822: prepare_package returns MempoolError::Full before its fee-floor check, so with full=true the aggregate error is not MinRelayFeeNotMet, the new early return does not fire, and MissingEphemeralSpends is the correct expectation.

Non-blocking, pre-existing and outside this PR's stated scope: when the pool is full and the package underpays, we report cluster/dust rather than Core's fee-floor failure, because capacity is checked before the fee floor in prepare_package.

Verification: locally ran cargo test --locked -p bitcoin-rs-mempool --lib package (44 passed, including both new package tests; the gateway signed-extremes test was not in that filter). CI test-crates succeeded at this head.

@gosunuts
gosunuts merged commit fffeac4 into main Oct 29, 2026
16 checks passed
@gosunuts
gosunuts deleted the fix/package-fee-cap-and-precedence branch October 29, 2026 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant