Skip to content

Apply the safe minimum fee floor to all wallet transactions - #4179

Closed
mswilkison wants to merge 2 commits into
threshold-network:mainfrom
mswilkison:feat/floor-all-wallet-tx-fees
Closed

Apply the safe minimum fee floor to all wallet transactions#4179
mswilkison wants to merge 2 commits into
threshold-network:mainfrom
mswilkison:feat/floor-all-wallet-tx-fees

Conversation

@mswilkison

@mswilkison mswilkison commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Follows up on #4172 (lrsaturnino review): the fee floor there covers deposit sweeps only. EstimateRedemptionFee, EstimateMovingFundsFee, and EstimateMovedFundsSweepFee still returned the raw oracle fee, so a redemption, moving-funds, or moved-funds-sweep transaction could be broadcast at the ~1 sat/vByte relay floor and jam the wallet the same way a sweep can.

Stacked on #4172 (based on branch fix/deposit-sweep-min-fee), so this PR's diff shows only its incremental change: the shared helper plus its application to the three other fee types. Merge #4172 first.

Change

  • Extracts the 25% buffer + minimum floor + Bridge-max bound into a shared applyWalletTxFeeFloor helper and a shared minWalletTxSatPerVByteFee constant (pkg/tbtcpg/fee.go).
  • Refactors deposit-sweep fee estimation onto the helper (no behavior change; identical math).
  • Applies the helper to redemptions, moving funds, and moved funds sweeps.
  • EstimateRedemptionFee now takes the redemption tx max total fee so the floor can be bounded by it; the caller fetches it from GetRedemptionParameters.

Note on redemption scope

Redemption fee estimation previously deferred the fee cap entirely to on-chain validation. To apply a client-side floor safely, it now also bounds the fee by the max total fee (clamp, not reject) so the floor cannot produce a fee the Bridge would reject, and errors if the raw estimate already exceeds the cap (consistent with the other transaction types) rather than broadcasting an underpriced transaction. The per-request max fee is still validated on-chain.

Tests

  • New TestApplyWalletTxFeeFloor covers buffer / floor / cap-clamp / floor-over-cap error / invalid-vsize.
  • Redemption, moving-funds, and moved-funds-sweep fee and propose tests updated for the buffered values.
  • go test ./pkg/tbtcpg/... passes; go build ./... and go vet clean.

Follow-ups still open from the #4172 review

  • Follower-side / on-chain enforcement of the floor (a leader can still propose a low fee; validators do not re-check the minimum) — being handled separately because it is rollout-sensitive.

@coderabbitai

coderabbitai Bot commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown

Warning

Review limit reached

@piotr-roslaniec, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 8 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0dfb1317-dcc5-47fd-8b06-751da67d6135

📥 Commits

Reviewing files that changed from the base of the PR and between f041178 and 4be8129.

📒 Files selected for processing (10)
  • pkg/tbtcpg/deposit_sweep.go
  • pkg/tbtcpg/deposit_sweep_fee_test.go
  • pkg/tbtcpg/fee.go
  • pkg/tbtcpg/fee_test.go
  • pkg/tbtcpg/moved_funds_sweep.go
  • pkg/tbtcpg/moved_funds_sweep_test.go
  • pkg/tbtcpg/moving_funds.go
  • pkg/tbtcpg/moving_funds_test.go
  • pkg/tbtcpg/redemptions.go
  • pkg/tbtcpg/redemptions_test.go
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This was referenced Jul 20, 2026
Closed
Merged
@mswilkison
mswilkison changed the base branch from main to fix/deposit-sweep-min-fee July 20, 2026 14:21
@mswilkison
mswilkison force-pushed the feat/floor-all-wallet-tx-fees branch from af7c525 to 9785c88 Compare July 20, 2026 15:02
@mswilkison
mswilkison marked this pull request as ready for review July 20, 2026 15:59
piotr-roslaniec added a commit that referenced this pull request Jul 22, 2026
@piotr-roslaniec
Enforce a minimum fee rate for deposit sweep transactions (#4172)
6d97e5a 
Fixes part **A** of #4171: a deposit sweep could be broadcast at the ~1
sat/vByte relay floor (a low-but-valid oracle estimate in an uncongested
mempool), and since sweeps are not RBF-enabled it could get stuck and
jam the wallet. Observed live on mainnet.

## Change

`estimateDepositsSweepFee`:

- **Errors** (instead of silently lowering the fee) if the raw estimate
exceeds the Bridge maximum (uneconomical), or if the minimum safe fee
(`minSweepTxSatPerVByteFee`, 5 sat/vByte) exceeds the maximum — so a
sweep is never broadcast below the intended floor.
- Otherwise applies `max(floor, ceil(rate × 1.25))`: a 25% buffer over
the oracle estimate to keep a margin during the estimate-to-broadcast
delay and stay adaptive under congestion (per the #4171 reference
design), floored at 5 sat/vByte, bounded above by the Bridge maximum.

The floor constant documents that it (and the buffer) is a stopgap for
the current non-RBF path and should be revisited when RBF lands; the
P2SH-vsize interaction is noted at the floor site.

## Tests

Unit test asserts a low estimate is floored to 5, an estimate above the
floor is buffered by 25%, and a Bridge maximum below the floor returns
an error (message-pinned to the floor branch). The deposit-sweep
scenario testdata is updated for the buffered fee. `go test
./pkg/tbtcpg/...` passes.

## Follow-ups

- **#4179** — applies the same floor to redemptions, moving funds, and
moved funds sweeps via a shared helper (stacked on this PR).
- **#4180** — follower-side soft check that a leader's proposed sweep
fee is not below the floor (log-only; full enforcement belongs on-chain
in `WalletProposalValidator`). Stacked on #4179.
- **Part B of #4171 (RBF + fee-bumping)** — the durable recovery fix; a
stuck sweep should be replaceable rather than jamming the wallet. Larger
change, targeted at FROST/ROAST. The static floor + buffer here is a
stopgap and should be revisited when RBF lands.


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **Bug Fixes**
- Improved deposit sweep fee estimation to avoid underpriced,
non-RBF-able sweeps stalling in the mempool.
- Introduced a minimum safe fee-rate floor and updated failure behavior
when safe fees can’t fit within the Bridge maximum.
- Added a 25% buffered fee-rate (rounded up), enforced minimum/maximum
caps, and updated informational sat/vByte reporting.
- Enhanced proposal-time warning logs when minimum-safe-fee behavior
prevents fee selection.
- **Tests**
- Added coverage for minimum-floor, buffer, and maximum-cap fee
behaviors, including rounding and error cases.
  - Updated the expected sweep fee in the proposal scenario test data.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
@piotr-roslaniec
piotr-roslaniec changed the base branch from fix/deposit-sweep-min-fee to main July 22, 2026 12:03
mswilkison and others added 2 commits July 22, 2026 13:27
@mswilkison @claude @piotr-roslaniec
Apply the safe minimum sweep-fee floor to all wallet transactions
5ca1b39 
Extract the 25% buffer + minimum floor + Bridge-max bound into a shared
applyWalletTxFeeFloor helper and a shared minWalletTxSatPerVByteFee const,
then apply it to redemptions, moving funds, and moved funds sweeps in
addition to deposit sweeps. These are all non-RBF wallet transactions that
jam the wallet if they get stuck at the relay floor, so the same protection
applies (per lrsaturnino review on threshold-network#4172).

EstimateRedemptionFee now takes the redemption tx max total fee so the floor
can be bounded by it; the caller fetches it from GetRedemptionParameters.
deposit sweep fee estimation is refactored onto the shared helper with no
behavior change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@piotr-roslaniec
docs: correct fee-floor per-request cap claims
4be8129 
The redemption fee estimator bounds the floored/buffered fee by the total
cap (txMaxTotalFee) only. The prior comment claimed this made the floor
unable to produce a fee the Bridge would reject, which is inaccurate: the
per-request TxMaxFee (snapshotted per request) is a separate cap enforced
only by on-chain validation, and the floor can raise a request's fee share
above it while the total stays within bounds.

Correct the ProposeRedemption and EstimateRedemptionFee comments, document
that applyWalletTxFeeFloor bounds only the total fee (and its caller
precondition), and note why the final clamp is floor-safe. Comments only;
no behavior change.
@piotr-roslaniec
piotr-roslaniec force-pushed the feat/floor-all-wallet-tx-fees branch from e55ec08 to 4be8129 Compare July 22, 2026 13:32
This was referenced Jul 23, 2026
Open
Open
@piotr-roslaniec

piotr-roslaniec commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Closing in favor of #4192, which carries this work forward rebased cleanly onto current main and folds in fixes from a multi-agent review of the change:

  • restored the fee-floor truncation invariant dropped during the shared-helper extraction,
  • corrected the estimatedFee/txVsize unit docs,
  • added a redemption diagnostic WARN when a floored per-request fee share would exceed the per-request TxMaxFee, and
  • added test coverage for the redemption raw-estimate-above-cap error and the moving-funds / moved-funds-sweep floor-clamp paths.

#4192 is on a threshold-network branch so it can be iterated on directly. Thanks @mswilkison for the original implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lrsaturnino lrsaturnino Awaiting requested review from lrsaturnino
@piotr-roslaniec piotr-roslaniec Awaiting requested review from piotr-roslaniec

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mswilkison @piotr-roslaniec