Apply the safe minimum fee floor to all wallet transactions#4192
Open
piotr-roslaniec wants to merge 4 commits into
Open
Apply the safe minimum fee floor to all wallet transactions#4192piotr-roslaniec wants to merge 4 commits into
piotr-roslaniec wants to merge 4 commits into
Conversation
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 #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>
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.
Restore the truncation invariant comment dropped when the deposit-sweep fee logic was extracted into applyWalletTxFeeFloor: the 25% buffer is applied to the truncated per-vByte rate, which is lossless only because EstimateFee returns an exact multiple of the vsize. Also correct the estimatedFee/txVsize unit description (satoshis and vBytes, not sat/vByte). Emit a distinct warning in ProposeRedemption when the floored per-request fee share would exceed the per-request maximum fee, so operators can tell this apart from a generic on-chain validation failure. Fee behavior is unchanged. Add coverage for previously untested branches: the redemption raw-estimate-above-cap error, and the minimum-floor clamp on the moving-funds and moved-funds-sweep paths.
📝 WalkthroughWalkthroughThe PR adds a shared wallet transaction fee-floor helper and applies it to deposit sweeps, moved-funds sweeps, moving-funds transactions, and redemptions. Redemption fee estimation now accepts and enforces the Bridge total-fee cap. ChangesWallet fee floor enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ProposeRedemption
participant GetRedemptionParameters
participant EstimateRedemptionFee
participant applyWalletTxFeeFloor
ProposeRedemption->>GetRedemptionParameters: fetch txMaxTotalFee
GetRedemptionParameters-->>ProposeRedemption: return redemption parameters
ProposeRedemption->>EstimateRedemptionFee: estimate with total-fee cap
EstimateRedemptionFee->>applyWalletTxFeeFloor: apply floor and cap
applyWalletTxFeeFloor-->>EstimateRedemptionFee: return adjusted fee or error
EstimateRedemptionFee-->>ProposeRedemption: return fee result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/tbtcpg/redemptions_test.go (1)
38-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the post-buffer cap clamp.
Add a case where the raw estimate is below the cap but its 25% buffer exceeds it (for example, 16 sat/vByte with a 4,500 sat cap should return 4,500). This verifies
EstimateRedemptionFeeforwards the cap to the shared helper correctly.Proposed test case
+ "buffered fee is clamped to the cap": { + estimateSatPerVByte: 16, // raw: 4000 + txMaxTotalFee: 4500, // buffered: 5000 + expectedFee: 4500, + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tbtcpg/redemptions_test.go` around lines 38 - 63, Add a table-driven case in the redemption fee tests for a raw estimate below the cap whose 25% buffer exceeds it, such as 16 sat/vByte with a 4,500 sat maximum, and assert the returned fee equals the cap. Keep the case focused on validating EstimateRedemptionFee’s cap forwarding to the shared helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/tbtcpg/deposit_sweep.go`:
- Around line 664-672: Update the fee-floor calculation in the sweep flow around
applyWalletTxFeeFloor so transactionSize accounts for each deposit’s actual
input type, including larger legacy P2SH inputs, or uses a conservative
upper-bound vsize. Pass that corrected size to applyWalletTxFeeFloor so
legacy-deposit sweeps cannot fall below the intended 5 sat/vByte floor.
---
Nitpick comments:
In `@pkg/tbtcpg/redemptions_test.go`:
- Around line 38-63: Add a table-driven case in the redemption fee tests for a
raw estimate below the cap whose 25% buffer exceeds it, such as 16 sat/vByte
with a 4,500 sat maximum, and assert the returned fee equals the cap. Keep the
case focused on validating EstimateRedemptionFee’s cap forwarding to the shared
helper.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a8859ec-4ed4-482b-87e6-3be44603d1e8
📒 Files selected for processing (10)
pkg/tbtcpg/deposit_sweep.gopkg/tbtcpg/deposit_sweep_fee_test.gopkg/tbtcpg/fee.gopkg/tbtcpg/fee_test.gopkg/tbtcpg/moved_funds_sweep.gopkg/tbtcpg/moved_funds_sweep_test.gopkg/tbtcpg/moving_funds.gopkg/tbtcpg/moving_funds_test.gopkg/tbtcpg/redemptions.gopkg/tbtcpg/redemptions_test.go
Comment on lines
664
to
+672
| // Caveat: transactionSize assumes all deposit inputs are witness (P2WSH), per | ||
| // this function's doc comment. A sweep that includes legacy P2SH deposits has | ||
| // a larger on-wire vsize than estimated here, so the effective on-wire rate | ||
| // can land slightly below the floor for such (rare) sweeps. It still dominates | ||
| // the 1 sat/vByte relay floor this fix targets; a fully accurate floor would | ||
| // require deposit-type-aware sizing. | ||
| // rate is an exact integer here because EstimateFee returns totalFee as | ||
| // satPerVByteFee * transactionSize (an exact multiple of the size), so the | ||
| // buffer is applied without truncation loss. If that contract changes, apply | ||
| // the buffer to totalFee directly instead of to the truncated rate. | ||
| rate := totalFee / transactionSize | ||
| rate = (rate*5 + 3) / 4 // ceil(rate * 1.25) | ||
| if rate < minSweepTxSatPerVByteFee { | ||
| rate = minSweepTxSatPerVByteFee | ||
| } | ||
| totalFee = rate * transactionSize | ||
| if uint64(totalFee) > totalMaxFee { | ||
| // totalMaxFee is bounded by Bitcoin's total supply (~2.1e15 sat), far | ||
| // below math.MaxInt64, so this narrowing cast cannot overflow. | ||
| totalFee = int64(totalMaxFee) | ||
| totalFee, err = applyWalletTxFeeFloor(totalFee, transactionSize, totalMaxFee) | ||
| if err != nil { | ||
| return 0, 0, err |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not knowingly underprice legacy-deposit sweeps.
The estimator charges the floor against an all-P2WSH vsize, while this comment confirms P2SH inputs can make the serialized transaction larger. Those sweeps can therefore be broadcast below 5 sat/vByte—the condition this change is intended to prevent. Calculate vsize from the actual deposit input types (or use a conservative size) before applying the floor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/tbtcpg/deposit_sweep.go` around lines 664 - 672, Update the fee-floor
calculation in the sweep flow around applyWalletTxFeeFloor so transactionSize
accounts for each deposit’s actual input type, including larger legacy P2SH
inputs, or uses a conservative upper-bound vsize. Pass that corrected size to
applyWalletTxFeeFloor so legacy-deposit sweeps cannot fall below the intended 5
sat/vByte floor.
This was referenced Jul 23, 2026
…ning Address review findings on the wallet fee-floor work: - Add an exported ErrMaxFeeTooLow sentinel returned by applyWalletTxFeeFloor when the safe minimum floor exceeds the Bridge maximum, mirroring the existing ErrFeeTooHigh / ErrSweepTxFeeTooHigh raw-estimate sentinels so the floor-too-high condition is programmatically distinguishable. - Base the redemption per-request fee warning on the worst-case (last-request) share floor(total/count)+total%count, matching the on-chain fee distribution, instead of the even floor share that could miss a remainder-only rejection. - Cover the floor-exceeds-cap error path in the moving-funds and moved-funds-sweep estimators, add exact-cap boundary cases to the fee-floor helper test, and assert the per-request fee warning fires on the last-request share.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/tbtcpg/redemptions_test.go (1)
308-356: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the actual minimum-fee-floor path.
The 25 sat/vByte estimate buffers to 32 sat/vByte, so this test never invokes the 5 sat/vByte floor. Use a 1 sat/vByte estimate and assert the 1015-sat floored fee with 339/338-sat boundary caps.
Proposed test adjustment
- btcChain.SetEstimateSatPerVByteFee(1, 25) + btcChain.SetEstimateSatPerVByteFee(1, 1) - txMaxFee: 3000, // 2166 <= 3000 + txMaxFee: 339, // 339 <= 339 - txMaxFee: 2165, + txMaxFee: 338, - RedemptionTxFee: big.NewInt(6496), + RedemptionTxFee: big.NewInt(1015),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tbtcpg/redemptions_test.go` around lines 308 - 356, Update the redemption fee test around the redeemersOutputScripts table and SetEstimateSatPerVByteFee call to use a 1 sat/vByte estimate, exercising the 5 sat/vByte minimum floor. Expect the resulting RedemptionProposal.RedemptionTxFee to be 1015 satoshis, and adjust the test cases to cover 339-sat per-request caps versus the 338-sat boundary where the final 338/339 split triggers the warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/tbtcpg/redemptions_test.go`:
- Around line 308-356: Update the redemption fee test around the
redeemersOutputScripts table and SetEstimateSatPerVByteFee call to use a 1
sat/vByte estimate, exercising the 5 sat/vByte minimum floor. Expect the
resulting RedemptionProposal.RedemptionTxFee to be 1015 satoshis, and adjust the
test cases to cover 339-sat per-request caps versus the 338-sat boundary where
the final 338/339 split triggers the warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a3b3917-38ea-44c3-b0fc-3ee9b66fb2b5
📒 Files selected for processing (6)
pkg/tbtcpg/fee.gopkg/tbtcpg/fee_test.gopkg/tbtcpg/moved_funds_sweep_test.gopkg/tbtcpg/moving_funds_test.gopkg/tbtcpg/redemptions.gopkg/tbtcpg/redemptions_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/tbtcpg/fee_test.go
- pkg/tbtcpg/moved_funds_sweep_test.go
- pkg/tbtcpg/redemptions.go
- pkg/tbtcpg/moving_funds_test.go
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.
What
Applies the safe minimum fee-rate floor (5 sat/vByte) plus a 25% buffer to
all wallet Bitcoin transactions - deposit sweeps, redemptions, moving funds,
and moved-funds sweeps - via the shared
applyWalletTxFeeFloorhelper inpkg/tbtcpg/fee.go. A fee oracle can return an unusably low estimate in anuncongested mempool; because these transactions are not RBF-enabled, an
underpriced one can get stuck and jam the wallet. The floor keeps the fee safely
above the relay floor while staying far below the Bridge maximum.
This carries forward the work from #4179 (opened from a fork that could not be
iterated on) rebased cleanly onto current
main, and folds in the fixes from amulti-agent review of that change.
Review fixes included
was extracted into the shared helper: the 25% buffer is applied to the
truncated per-vByte rate, which is lossless only because
EstimateFeereturnsan exact multiple of the vsize. Documented so a future change to that contract
is caught rather than silently under-pricing.
estimatedFee/txVsizeunit description (satoshis andvBytes, not "sat/vByte").
ProposeRedemptionwhen thefloored per-request fee share would exceed the per-request maximum fee, so
operators can distinguish this from a generic on-chain validation failure. Fee
behavior is unchanged; the per-request cap remains enforced by on-chain
validation.
raw-estimate-above-cap error, and the minimum-floor clamp on the moving-funds
and moved-funds-sweep paths.
Notes
minWalletTxSatPerVByteFeeand the buffer are a stopgap for the currentnon-RBF, fire-and-forget wallet-transaction path; once fee-bumping lands
(Deposit sweep can broadcast at the 1 sat/vByte relay floor and get stuck (no fee margin; sweeps are non-RBF with no fee-bumping) #4171) this policy should be revisited.
Testing
gofmt,go build,go vet,staticcheck, andgo test ./pkg/tbtcpg/...(91 tests) all pass on current
main.Summary by CodeRabbit