Enforce a minimum fee rate for deposit sweep transactions - #4172
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughDeposit sweep fee estimation now enforces a 5 sat/vByte minimum, applies a 25% ceiling-rounded buffer, validates Bridge maximum limits, and caps final fees. Tests and sweep proposal fixture expectations are updated. ChangesDeposit sweep fee estimation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
This PR only covers deposit sweeps. |
|
Two small things in the description are out of date with the diff: it says the floor is applied "before" the Bridge maximum-fee check, but the code checks the cap first (line 660) and floors after (line 670). It also says the low-fee test was "flagged rather than included," but |
aa730ce to
a21be7f
Compare
|
This closes the fee-floor gap on the generation side ( |
|
The fee floor/buffer this PR adds is computed off a P2WSH-only deposit-input estimate — That pre-existing underestimation interacts directly with the new floor: since the floor/buffer is applied to the estimated vsize rather than the actual on-wire vsize, a sweep with a meaningful share of P2SH deposits can still be assembled at an effective rate well below the intended 5 sat/vByte. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/tbtcpg/deposit_sweep_fee_test.go (1)
27-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test case to cover cap validation.
The PR summary states that tests cover cap validation, but there doesn't appear to be a case explicitly verifying that a buffered estimate exceeding the cap is correctly truncated down to the maximum fee. Consider adding a test case to guarantee this logic branch is fully exercised.
💡 Proposed test case for cap validation
"minimum floor above the cap returns an error": { estimateSatPerVByte: 1, // Cap sits below 5*size (the floor) but above the raw fee (1*size), // so the minimum-fee check must error rather than lower the fee. The // substring pins this to the floor-exceeds-cap branch specifically, // distinguishing it from the raw-fee-exceeds-cap error. perDepositMaxFee: uint64(3 * size), expectErrorContains: "minimum safe sweep fee", }, + "buffered estimate above the cap is capped": { + estimateSatPerVByte: 20, + perDepositMaxFee: uint64(22 * size), + expectedSatPerVByteFee: 22, + }, }🤖 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_fee_test.go` around lines 27 - 52, Add a table case to the existing tests map in deposit sweep fee coverage where the 25%-buffered estimate exceeds perDepositMaxFee, and assert the returned expectedSatPerVByteFee is capped at the maximum allowed fee rather than the buffered estimate. Keep the existing floor and error cases unchanged, and choose inputs that exercise truncation without triggering an error.
🤖 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/deposit_sweep_fee_test.go`:
- Around line 27-52: Add a table case to the existing tests map in deposit sweep
fee coverage where the 25%-buffered estimate exceeds perDepositMaxFee, and
assert the returned expectedSatPerVByteFee is capped at the maximum allowed fee
rather than the buffered estimate. Keep the existing floor and error cases
unchanged, and choose inputs that exercise truncation without triggering an
error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60647019-d7df-46fa-aedc-6ce2c3dac435
📒 Files selected for processing (3)
pkg/tbtcpg/deposit_sweep.gopkg/tbtcpg/deposit_sweep_fee_test.gopkg/tbtcpg/internal/test/testdata/propose_sweep_scenario_0.json
Cover the branch where the 25%-buffered fee exceeds the Bridge maximum and is bounded down to the cap (per CodeRabbit review on threshold-network#4172). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/tbtcpg/deposit_sweep_fee_test.go (1)
11-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid duplicating
depositScriptByteSizehere.pkg/tbtcpg/deposit_sweep_fee_test.gois inpackage tbtcpg_test, so it can’t reference the unexported constant inpkg/tbtcpg/deposit_sweep.go. Expose a tiny test-facing helper or move this check into the internal package so the literal doesn’t drift.🤖 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_fee_test.go` around lines 11 - 27, Update sweepVirtualSize in the sweep fee tests to avoid hardcoding the deposit script size literal; expose a minimal test-facing helper for the existing depositScriptByteSize value or relocate the check into the internal tbtcpg package, ensuring the estimator continues using the shared constant and cannot drift.
🤖 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/deposit_sweep_fee_test.go`:
- Around line 11-27: Update sweepVirtualSize in the sweep fee tests to avoid
hardcoding the deposit script size literal; expose a minimal test-facing helper
for the existing depositScriptByteSize value or relocate the check into the
internal tbtcpg package, ensuring the estimator continues using the shared
constant and cannot drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b3424641-c37a-46d7-b3a9-ca52d0c1f7ff
📒 Files selected for processing (2)
pkg/tbtcpg/deposit_sweep.gopkg/tbtcpg/deposit_sweep_fee_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/tbtcpg/deposit_sweep.go
A deposit sweep could be broadcast at the 1 sat/vByte relay floor when the fee oracle returned an unusably low estimate in an uncongested mempool. Because sweeps are not RBF-enabled, such a transaction can get stuck in the mempool and jam the wallet: no new sweep can be built while the previous one is unconfirmed, so eligible deposits pile up until it confirms or is evicted. Clamp the estimated sweep fee up to a conservative minimum (minSweepTxSatPerVByteFee) before the existing Bridge maximum-fee check, so a sweep is never broadcast near the relay floor. Refs threshold-network#4171 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Apply the Bridge maximum-fee check to the raw estimate before raising the fee to the minimum, and bound the minimum itself by the maximum, so raising the fee to the floor can never spuriously trigger the "exceeds maximum fee" error (the two were previously coupled in the wrong order). - Add a unit test asserting a low estimate is floored to the minimum while a healthy estimate is left unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eeds the cap - Return an error when the minimum safe fee (minSweepTxSatPerVByteFee sat/vByte) exceeds the Bridge maximum, instead of silently lowering the fee below the floor this PR is meant to enforce (lrsaturnino review). - Apply a 25% buffer over the oracle estimate (max(floor, ceil(rate*1.25))) so the fee keeps a margin during the estimate-to-broadcast delay and stays adaptive under congestion, per the threshold-network#4171 reference design. - Add a test asserting the cap-below-floor case returns an error, and update the buffered-fee expectation in the deposit sweep scenario testdata. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Assert the error message contains "minimum safe sweep fee" so the test distinguishes the floor-exceeds-cap error from the raw-fee-exceeds-cap error, rather than accepting any non-nil error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Note that the static floor + 25% buffer exist only because the current sweep path is fire-and-forget and non-RBF; when RBF/fee-bumping (Part B, threshold-network#4171) lands the policy should be revisited rather than carried forward. Also note the floor is computed against a witness-only vsize estimate, so sweeps with legacy P2SH deposits can land slightly below the floor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Cover the branch where the 25%-buffered fee exceeds the Bridge maximum and is bounded down to the cap (per CodeRabbit review on threshold-network#4172). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nches Address review findings on the deposit sweep minimum-fee change: - Assert the on-chain TotalFee in every success case, not just the informational SatPerVByteFee (which is lossy through math.Round). - Cover the raw-estimate-exceeds-max error branch, previously untested. - Exercise multi-deposit sweeps (N>1) on both the happy path and the floor-exceeds-cap error branch, where tx size grows sub-linearly while the Bridge max grows linearly. - Log fee-estimation failures at WARN in ProposeDepositsSweep so a safe-minimum-fee abort is distinguishable from a benign no-op. - Document the new minimum-safe-fee error in the exported godoc, note the buffer's dependency on exact rate truncation, and the narrowing-cast overflow invariant.
54cf39b to
1b58142
Compare
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>
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>
Resolve conflicts by final behavior, preserving the candidate's OOB hardening and the no-cap / exact-target SPV semantics while taking main's threshold-network#4039 work-accumulation model, threshold-network#4165 script-type isolation, threshold-network#4172 sweep fee floor, and the threshold-network#3998 v1-random-beacon extraction. SPV conflict resolution: - pkg/maintainer/spv/spv.go: keep candidate side (no 144-header cap, header cache, exact decoded-target DIFF1 comparison + zero-target guard). Removed the `const maxProofHeaders = 144` that the 3-way merge silently re-added from main (merge-base lacked it, so the candidate's iteration-1 removal was invisible to the merge). - pkg/maintainer/spv/deposit_sweep.go: combine candidate's bounds-checked OutputAt accessor with main's bitcoin.GetScriptType classification (threshold-network#4165). - pkg/maintainer/spv/spv_test.go: keep candidate's superset of cases (non-DIFF1 rounding, zero-target, 145- and 201-header long-run cases); drop main's removed "minimum difficulty run exceeds header bound" cap test. Accepted main's deletion of the extracted v1 random beacon and dead Ropsten infra. NOTE: this drops two candidate-only v1 hardenings that main moved out of this repo — require(entryCreatedSuccess) in KeepRandomBeaconOperator.sol and the registry-approval modifier in KeepRandomBeaconServiceImplV1.sol. If those are in security scope they must be ported to the keep-core-v1 repo. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve conflicts by final behavior, preserving the candidate's OOB hardening and the no-cap / exact-target SPV semantics while taking main's threshold-network#4039 work-accumulation model, threshold-network#4165 script-type isolation, threshold-network#4172 sweep fee floor, and the threshold-network#3998 v1-random-beacon extraction. SPV conflict resolution: - pkg/maintainer/spv/spv.go: keep candidate side (no 144-header cap, header cache, exact decoded-target DIFF1 comparison + zero-target guard). Removed the `const maxProofHeaders = 144` that the 3-way merge silently re-added from main (merge-base lacked it, so the candidate's iteration-1 removal was invisible to the merge). - pkg/maintainer/spv/deposit_sweep.go: combine candidate's bounds-checked OutputAt accessor with main's bitcoin.GetScriptType classification (threshold-network#4165). - pkg/maintainer/spv/spv_test.go: keep candidate's superset of cases (non-DIFF1 rounding, zero-target, 145- and 201-header long-run cases); drop main's removed "minimum difficulty run exceeds header bound" cap test. Accepted main's deletion of the extracted v1 random beacon and dead Ropsten infra. NOTE: this drops two candidate-only v1 hardenings that main moved out of this repo — require(entryCreatedSuccess) in KeepRandomBeaconOperator.sol and the registry-approval modifier in KeepRandomBeaconServiceImplV1.sol. If those are in security scope they must be ported to the keep-core-v1 repo.
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:minSweepTxSatPerVByteFee, 5 sat/vByte) exceeds the maximum — so a sweep is never broadcast below the intended floor.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 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 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
WalletProposalValidator). Stacked on Apply the safe minimum fee floor to all wallet transactions #4179.Summary by CodeRabbit