diff --git a/pkg/tbtc/deposit_sweep.go b/pkg/tbtc/deposit_sweep.go index 824ce29d28..8920a0b261 100644 --- a/pkg/tbtc/deposit_sweep.go +++ b/pkg/tbtc/deposit_sweep.go @@ -49,6 +49,17 @@ const ( // the transaction is known on the Bitcoin chain. This delay is needed // as spreading the transaction over the Bitcoin network takes time. depositSweepBroadcastCheckDelay = 1 * time.Minute + // minSweepTxSatPerVByteFee mirrors tbtcpg.MinWalletTxSatPerVByteFee, the safe + // minimum sweep fee rate. It is duplicated here because pkg/tbtcpg imports + // pkg/tbtc, so this package cannot import the canonical constant without a + // dependency cycle; keep the two in sync (guarded by TestSweepFeeConstants + // MirrorTbtcpg). It backs a follower-side soft (log-only) check that the + // leader's proposed sweep fee is not below the floor (see + // threshold-network/keep-core#4171). + minSweepTxSatPerVByteFee = 5 + // depositScriptByteSize mirrors tbtcpg.DepositScriptByteSize, the worst-case + // deposit script size used to estimate the sweep transaction virtual size. + depositScriptByteSize = 126 ) // DepositSweepProposal represents a deposit sweep proposal issued by a @@ -461,6 +472,41 @@ func ValidateDepositSweepProposal( "deposit sweep proposal is valid", ) + // Follower-side soft check on the proposed fee. The on-chain + // WalletProposalValidator only bounds the sweep fee from above, not below, + // so a misbehaving or unpatched leader can propose a fee at the ~1 sat/vByte + // relay floor that this node would otherwise sign - the same underpricing + // that jams the wallet (see threshold-network/keep-core#4171). We recompute + // the safe minimum and warn if the proposal is below it. + // + // This is intentionally log-only, not a rejection: rejecting a below-floor + // proposal here would, during a mixed-version rollout, split signers (patched + // nodes reject, unpatched nodes sign) and could stall signing. Hard + // enforcement belongs on-chain in the WalletProposalValidator, or behind a + // coordinated all-nodes upgrade. + if sweepTxSize, sizeErr := bitcoin.NewTransactionSizeEstimator(). + AddPublicKeyHashInputs(1, true). + AddScriptHashInputs(len(proposal.DepositsKeys), depositScriptByteSize, true). + AddPublicKeyHashOutputs(1, true). + VirtualSize(); sizeErr != nil { + validateProposalLogger.Warnf( + "cannot estimate sweep tx size for the fee sanity check: [%v]", + sizeErr, + ) + } else if minSweepTxFee := int64(minSweepTxSatPerVByteFee) * sweepTxSize; proposal.SweepTxFee != nil && + proposal.SweepTxFee.Int64() < minSweepTxFee { + validateProposalLogger.Warnf( + "proposed sweep tx fee [%v] is below the safe minimum [%d] "+ + "([%d] sat/vByte * [%d] vByte); the leader may be underpricing "+ + "the sweep, which risks it getting stuck in the mempool and "+ + "jamming the wallet", + proposal.SweepTxFee, + minSweepTxFee, + minSweepTxSatPerVByteFee, + sweepTxSize, + ) + } + deposits := make([]*Deposit, len(depositExtraInfo)) for i, dei := range depositExtraInfo { deposits[i] = dei.Deposit diff --git a/pkg/tbtc/sweep_fee_sync_test.go b/pkg/tbtc/sweep_fee_sync_test.go new file mode 100644 index 0000000000..3be82f2273 --- /dev/null +++ b/pkg/tbtc/sweep_fee_sync_test.go @@ -0,0 +1,46 @@ +package tbtc_test + +import ( + "testing" + + "github.com/keep-network/keep-core/pkg/tbtcpg" +) + +// TestSweepFeeConstantsMirrorTbtcpg guards the sweep-fee constants that +// pkg/tbtc/deposit_sweep.go duplicates from pkg/tbtcpg. The follower-side soft +// check (threshold-network/keep-core#4171) recomputes the safe minimum sweep +// fee, but pkg/tbtcpg imports pkg/tbtc, so pkg/tbtc cannot import the canonical +// constants without a dependency cycle and hand-copies them instead. +// +// This test lives in the external tbtc_test package precisely because that +// package can import pkg/tbtcpg without forming the cycle. It pins the canonical +// tbtcpg values to the literals mirrored in pkg/tbtc/deposit_sweep.go +// (minSweepTxSatPerVByteFee and depositScriptByteSize). If the canonical values +// drift, this test fails, forcing the pkg/tbtc mirrors - and these expected +// literals - to be updated together. +func TestSweepFeeConstantsMirrorTbtcpg(t *testing.T) { + // Mirrored by pkg/tbtc/deposit_sweep.go:minSweepTxSatPerVByteFee. + const expectedMinWalletTxSatPerVByteFee = 5 + // Mirrored by pkg/tbtc/deposit_sweep.go:depositScriptByteSize. + const expectedDepositScriptByteSize = 126 + + if tbtcpg.MinWalletTxSatPerVByteFee != expectedMinWalletTxSatPerVByteFee { + t.Errorf( + "tbtcpg.MinWalletTxSatPerVByteFee is [%d]; the pkg/tbtc mirror "+ + "minSweepTxSatPerVByteFee [%d] is now stale and must be updated "+ + "along with this test", + tbtcpg.MinWalletTxSatPerVByteFee, + expectedMinWalletTxSatPerVByteFee, + ) + } + + if tbtcpg.DepositScriptByteSize != expectedDepositScriptByteSize { + t.Errorf( + "tbtcpg.DepositScriptByteSize is [%d]; the pkg/tbtc mirror "+ + "depositScriptByteSize [%d] is now stale and must be updated "+ + "along with this test", + tbtcpg.DepositScriptByteSize, + expectedDepositScriptByteSize, + ) + } +} diff --git a/pkg/tbtcpg/deposit_sweep.go b/pkg/tbtcpg/deposit_sweep.go index 491e4411fa..d6f1cfb8cd 100644 --- a/pkg/tbtcpg/deposit_sweep.go +++ b/pkg/tbtcpg/deposit_sweep.go @@ -17,9 +17,10 @@ import ( "github.com/keep-network/keep-core/pkg/tbtc" ) -// Use the worst-case 126-byte deposit script with embedded extra data for estimation. -// This will ensure that deposit sweep transaction fees are not underestimated. -const depositScriptByteSize = 126 +// DepositScriptByteSize is the worst-case 126-byte deposit script with embedded +// extra data used for transaction size estimation. This ensures that deposit +// sweep transaction fees are not underestimated. +const DepositScriptByteSize = 126 // DepositSweepLookBackBlocks is the look-back period in blocks used // when searching for submitted deposit-related events. It's equal to @@ -623,7 +624,7 @@ func estimateDepositsSweepFee( // 1 P2WPKH main UTXO input. AddPublicKeyHashInputs(1, true). // depositsCount P2WSH deposit inputs. - AddScriptHashInputs(depositsCount, depositScriptByteSize, true). + AddScriptHashInputs(depositsCount, DepositScriptByteSize, true). // 1 P2WPKH output. AddPublicKeyHashOutputs(1, true). VirtualSize() @@ -641,10 +642,27 @@ func estimateDepositsSweepFee( // Compute the maximum possible total fee for the entire sweep transaction. totalMaxFee := uint64(depositsCount) * perDepositMaxFee + // A raw estimate already above the Bridge maximum means the sweep is + // uneconomical to perform; return an error. if uint64(totalFee) > totalMaxFee { return 0, 0, fmt.Errorf("estimated fee exceeds the maximum fee") } + // Enforce the safe minimum fee rate and 25% buffer, bounded by the Bridge + // maximum, so a sweep is never broadcast below the floor where it could get + // stuck and jam the wallet. Errors if even the floor exceeds the maximum. + // + // 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. + totalFee, err = applyWalletTxFeeFloor(totalFee, transactionSize, totalMaxFee) + if err != nil { + return 0, 0, err + } + // Compute the actual sat/vbyte fee for informational purposes. satPerVByteFee := math.Round(float64(totalFee) / float64(transactionSize)) diff --git a/pkg/tbtcpg/deposit_sweep_fee_test.go b/pkg/tbtcpg/deposit_sweep_fee_test.go new file mode 100644 index 0000000000..02c35700d2 --- /dev/null +++ b/pkg/tbtcpg/deposit_sweep_fee_test.go @@ -0,0 +1,94 @@ +package tbtcpg_test + +import ( + "strings" + "testing" + + "github.com/keep-network/keep-core/pkg/bitcoin" + "github.com/keep-network/keep-core/pkg/tbtcpg" +) + +// TestEstimateDepositsSweepFee_MinimumFloorAndBuffer verifies the sweep fee +// logic: a low estimate is raised to the minimum floor, an estimate above the +// floor is buffered by 25%, and a Bridge maximum below the minimum floor +// returns an error rather than silently broadcasting an underpriced sweep. +func TestEstimateDepositsSweepFee_MinimumFloorAndBuffer(t *testing.T) { + // Virtual size of a one-deposit sweep, used to size the cap for the error + // case relative to the minimum floor. 126 == DepositScriptByteSize. + size, err := bitcoin.NewTransactionSizeEstimator(). + AddPublicKeyHashInputs(1, true). + AddScriptHashInputs(1, 126, true). + AddPublicKeyHashOutputs(1, true). + VirtualSize() + if err != nil { + t.Fatal(err) + } + + tests := map[string]struct { + estimateSatPerVByte int64 + perDepositMaxFee uint64 + expectedSatPerVByteFee int64 + expectErrorContains string + }{ + "low estimate is raised to the minimum floor": { + estimateSatPerVByte: 1, + perDepositMaxFee: 100000, + expectedSatPerVByteFee: 5, // max(5, ceil(1*1.25)=2) = 5 + }, + "estimate above the floor is buffered by 25%": { + estimateSatPerVByte: 20, + perDepositMaxFee: 100000, + expectedSatPerVByteFee: 25, // ceil(20*1.25) = 25 + }, + "buffered estimate above the cap is bounded to the cap": { + estimateSatPerVByte: 20, + // ceil(20*1.25)=25 sat/vByte buffered fee exceeds the 22*size cap, + // so it is bounded down to the cap (rate 22), not the buffered 25. + perDepositMaxFee: uint64(22 * size), + expectedSatPerVByteFee: 22, + }, + "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 transaction fee", + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + tbtcChain := tbtcpg.NewLocalChain() + tbtcChain.SetDepositParameters(0, 0, test.perDepositMaxFee, 0) + + btcChain := tbtcpg.NewLocalBitcoinChain() + btcChain.SetEstimateSatPerVByteFee(1, test.estimateSatPerVByte) + + fees, err := tbtcpg.EstimateDepositsSweepFee(tbtcChain, btcChain, 1) + + if test.expectErrorContains != "" { + if err == nil { + t.Fatalf("expected an error, got fee result [%v]", fees) + } + if !strings.Contains(err.Error(), test.expectErrorContains) { + t.Fatalf( + "expected error containing [%s]; got [%v]", + test.expectErrorContains, err, + ) + } + return + } + if err != nil { + t.Fatalf("unexpected error: [%v]", err) + } + if got := fees[1].SatPerVByteFee; got != test.expectedSatPerVByteFee { + t.Errorf( + "unexpected sweep fee rate\nexpected: [%d] sat/vByte\nactual: [%d] sat/vByte", + test.expectedSatPerVByteFee, got, + ) + } + }) + } +} diff --git a/pkg/tbtcpg/fee.go b/pkg/tbtcpg/fee.go new file mode 100644 index 0000000000..e66f8f6456 --- /dev/null +++ b/pkg/tbtcpg/fee.go @@ -0,0 +1,74 @@ +package tbtcpg + +import "fmt" + +// MinWalletTxSatPerVByteFee is the minimum fee rate, in sat/vByte, applied to +// wallet Bitcoin transactions (deposit sweeps, redemptions, moving funds, moved +// funds sweeps). A fee oracle can return an unusably low estimate (down to the +// 1 sat/vByte relay floor enforced by the Electrum client) in an uncongested +// mempool. Because these transactions spend or consolidate significant wallet +// value and are not RBF-enabled, they cannot be replaced once broadcast, so a +// floor-rate transaction can get stuck in the mempool and jam the wallet: no +// new wallet transaction can be built while the previous one is unconfirmed. +// This minimum keeps the fee safely above the relay floor while remaining far +// below the Bridge's maximum fee. The value is intentionally conservative and +// could be made configurable; see threshold-network/keep-core#4171. +// +// NOTE: this static floor and the 25% buffer applied in applyWalletTxFeeFloor +// are a stopgap for the current fire-and-forget, non-RBF wallet transaction +// path: because a stuck transaction cannot be fee-bumped, the fee must be right +// on the first broadcast. Once RBF / fee-bumping lands (Part B, tracked in +// #4171) the safety net shifts to monitor-and-bump, and this policy should be +// revisited rather than carried forward unchanged: the defensive buffer can be +// dropped and the floor relaxed toward the live estimate, keeping only a small +// relay-propagation minimum. +const MinWalletTxSatPerVByteFee = 5 + +// applyWalletTxFeeFloor raises a raw oracle fee estimate to a safe value for a +// non-RBF wallet transaction. It: +// - adds a 25% buffer over the oracle estimate so there is margin during the +// estimate-to-broadcast delay and the fee stays adaptive under congestion, +// - enforces a floor of MinWalletTxSatPerVByteFee sat/vByte, and +// - bounds the result by maxTotalFee (the Bridge maximum for the transaction). +// +// It returns an error if the minimum floor alone would exceed maxTotalFee - a +// safe transaction cannot be built, so the caller must not broadcast an +// underpriced one. estimatedFee is the raw oracle fee and txVsize is the +// estimated transaction virtual size, both in the usual sat / vByte units. +// +// The buffer and floor are applied to the estimated vsize; a transaction whose +// real on-wire vsize is larger than estimated (e.g. a deposit sweep containing +// legacy P2SH inputs) can land slightly below the intended rate, but still far +// above the relay floor this guards against. +func applyWalletTxFeeFloor( + estimatedFee int64, + txVsize int64, + maxTotalFee uint64, +) (int64, error) { + if txVsize <= 0 { + return 0, fmt.Errorf("invalid transaction virtual size [%d]", txVsize) + } + + // If even the minimum floor exceeds the Bridge maximum, a safe transaction + // cannot be constructed; error rather than silently broadcast underpriced. + if uint64(MinWalletTxSatPerVByteFee*txVsize) > maxTotalFee { + return 0, fmt.Errorf( + "minimum safe transaction fee [%d] exceeds the maximum fee [%d]", + MinWalletTxSatPerVByteFee*txVsize, + maxTotalFee, + ) + } + + rate := estimatedFee / txVsize + rate = (rate*5 + 3) / 4 // ceil(rate * 1.25) + if rate < MinWalletTxSatPerVByteFee { + rate = MinWalletTxSatPerVByteFee + } + + totalFee := rate * txVsize + if uint64(totalFee) > maxTotalFee { + totalFee = int64(maxTotalFee) + } + + return totalFee, nil +} diff --git a/pkg/tbtcpg/fee_test.go b/pkg/tbtcpg/fee_test.go new file mode 100644 index 0000000000..6aec062e44 --- /dev/null +++ b/pkg/tbtcpg/fee_test.go @@ -0,0 +1,81 @@ +package tbtcpg + +import ( + "strings" + "testing" +) + +func TestApplyWalletTxFeeFloor(t *testing.T) { + const vsize = 200 + + tests := map[string]struct { + estimatedFee int64 + txVsize int64 + maxTotalFee uint64 + expectedFee int64 + expectErrorContains string + }{ + "estimate above the floor is buffered by 25%": { + estimatedFee: 4000, // rate 20 sat/vByte + txVsize: vsize, + maxTotalFee: 100000, + expectedFee: 5000, // ceil(20*1.25)=25 sat/vByte * 200 + }, + "low estimate is raised to the minimum floor": { + estimatedFee: vsize, // rate 1 sat/vByte + txVsize: vsize, + maxTotalFee: 100000, + expectedFee: 1000, // max(5, ceil(1*1.25)=2)=5 sat/vByte * 200 + }, + "buffered fee above the cap is bounded to the cap": { + estimatedFee: 4000, // rate 20 -> buffered 25 sat/vByte * 200 = 5000 + txVsize: vsize, + maxTotalFee: 4500, // below the buffered 5000 + expectedFee: 4500, + }, + "minimum floor above the cap returns an error": { + estimatedFee: 100, + txVsize: vsize, + maxTotalFee: 800, // below the 5 sat/vByte floor (1000) + expectErrorContains: "minimum safe transaction fee", + }, + "non-positive virtual size returns an error": { + estimatedFee: 1000, + txVsize: 0, + maxTotalFee: 100000, + expectErrorContains: "invalid transaction virtual size", + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + fee, err := applyWalletTxFeeFloor( + tc.estimatedFee, + tc.txVsize, + tc.maxTotalFee, + ) + + if tc.expectErrorContains != "" { + if err == nil { + t.Fatalf("expected an error, got fee [%d]", fee) + } + if !strings.Contains(err.Error(), tc.expectErrorContains) { + t.Fatalf( + "expected error containing [%s]; got [%v]", + tc.expectErrorContains, err, + ) + } + return + } + if err != nil { + t.Fatalf("unexpected error: [%v]", err) + } + if fee != tc.expectedFee { + t.Errorf( + "unexpected fee\nexpected: [%d]\nactual: [%d]", + tc.expectedFee, fee, + ) + } + }) + } +} diff --git a/pkg/tbtcpg/internal/test/testdata/propose_sweep_scenario_0.json b/pkg/tbtcpg/internal/test/testdata/propose_sweep_scenario_0.json index 3d020b7622..7a6062d306 100644 --- a/pkg/tbtcpg/internal/test/testdata/propose_sweep_scenario_0.json +++ b/pkg/tbtcpg/internal/test/testdata/propose_sweep_scenario_0.json @@ -37,7 +37,7 @@ "FundingOutputIndex": 3 } ], - "SweepTxFee": 10634, + "SweepTxFee": 13497, "DepositsRevealBlocks": [11, 31, 32] } } diff --git a/pkg/tbtcpg/moved_funds_sweep.go b/pkg/tbtcpg/moved_funds_sweep.go index 628669ed2a..9657950627 100644 --- a/pkg/tbtcpg/moved_funds_sweep.go +++ b/pkg/tbtcpg/moved_funds_sweep.go @@ -411,5 +411,13 @@ func EstimateMovedFundsSweepFee( return 0, ErrSweepTxFeeTooHigh } + // Enforce the safe minimum fee rate and buffer so a non-RBF moved funds + // sweep transaction is never broadcast below the floor where it could get + // stuck and jam the wallet. + totalFee, err = applyWalletTxFeeFloor(totalFee, transactionSize, sweepTxMaxTotalFee) + if err != nil { + return 0, err + } + return totalFee, nil } diff --git a/pkg/tbtcpg/moved_funds_sweep_test.go b/pkg/tbtcpg/moved_funds_sweep_test.go index e0b1963381..85b43d4b19 100644 --- a/pkg/tbtcpg/moved_funds_sweep_test.go +++ b/pkg/tbtcpg/moved_funds_sweep_test.go @@ -365,7 +365,9 @@ func TestMovedFundsSweepAction_ProposeMovedFundsSweep(t *testing.T) { expectedProposal: &tbtc.MovedFundsSweepProposal{ MovingFundsTxHash: movingFundsTxHash, MovingFundsTxOutputIndex: movingFundsTxOutputIndex, - SweepTxFee: big.NewInt(4450), + // raw 4450 (178 vByte * 25 sat/vByte), buffered to + // ceil(25*1.25)=32 sat/vByte * 178 = 5696, below the 6000 cap. + SweepTxFee: big.NewInt(5696), }, }, } @@ -431,14 +433,18 @@ func TestEstimateMovedFundsSweepFee(t *testing.T) { "estimated fee correct, one input": { sweepTxMaxTotalFee: 3000, hasMainUtxo: false, - expectedFee: 1760, - expectedError: nil, + // raw 1760 (110 vByte * 16 sat/vByte), buffered to + // ceil(16*1.25)=20 sat/vByte * 110 = 2200, below the cap. + expectedFee: 2200, + expectedError: nil, }, "estimated fee correct, two inputs": { sweepTxMaxTotalFee: 3000, hasMainUtxo: true, - expectedFee: 2848, - expectedError: nil, + // raw 2848 (178 vByte * 16 sat/vByte); buffered 20 sat/vByte * 178 + // = 3560 exceeds the 3000 cap, so it is bounded down to the cap. + expectedFee: 3000, + expectedError: nil, }, "estimated fee too high": { sweepTxMaxTotalFee: 2500, diff --git a/pkg/tbtcpg/moving_funds.go b/pkg/tbtcpg/moving_funds.go index 22a842abc8..351687ad37 100644 --- a/pkg/tbtcpg/moving_funds.go +++ b/pkg/tbtcpg/moving_funds.go @@ -656,5 +656,13 @@ func EstimateMovingFundsFee( return 0, ErrFeeTooHigh } + // Enforce the safe minimum fee rate and buffer so a non-RBF moving funds + // transaction is never broadcast below the floor where it could get stuck + // and jam the wallet. + totalFee, err = applyWalletTxFeeFloor(totalFee, transactionSize, txMaxTotalFee) + if err != nil { + return 0, err + } + return totalFee, nil } diff --git a/pkg/tbtcpg/moving_funds_test.go b/pkg/tbtcpg/moving_funds_test.go index 4f24ae8ef7..2b3bc36f7d 100644 --- a/pkg/tbtcpg/moving_funds_test.go +++ b/pkg/tbtcpg/moving_funds_test.go @@ -568,8 +568,10 @@ func TestMovingFundsAction_ProposeMovingFunds(t *testing.T) { "fee estimated": { fee: 0, // trigger fee estimation expectedProposal: &tbtc.MovingFundsProposal{ - TargetWallets: targetWallets, - MovingFundsTxFee: big.NewInt(4300), + TargetWallets: targetWallets, + // raw 4300 (172 vByte * 25 sat/vByte), buffered to + // ceil(25*1.25)=32 sat/vByte * 172 = 5504, below the 6000 cap. + MovingFundsTxFee: big.NewInt(5504), }, }, } @@ -655,7 +657,9 @@ func TestEstimateMovingFundsFee(t *testing.T) { }{ "estimated fee correct": { txMaxTotalFee: 6000, - expectedFee: 3248, + // raw 3248 (203 vByte * 16 sat/vByte), buffered to + // ceil(16*1.25)=20 sat/vByte * 203 = 4060, below the cap. + expectedFee: 4060, expectedError: nil, }, "estimated fee too high": { diff --git a/pkg/tbtcpg/redemptions.go b/pkg/tbtcpg/redemptions.go index 981e9a8eb7..978d12e826 100644 --- a/pkg/tbtcpg/redemptions.go +++ b/pkg/tbtcpg/redemptions.go @@ -210,16 +210,25 @@ func (rt *RedemptionTask) ProposeRedemption( taskLogger.Infof("preparing a redemption proposal") - // Estimate fee if it's missing. Do not check the estimated fee against - // the maximum total and per-request fees allowed by the Bridge. This - // is done during the on-chain validation of the proposal so there is no - // need to do it here. + // Estimate fee if it's missing. The per-request maximum fee is still + // checked during the on-chain validation of the proposal; here we bound + // the estimate by the maximum total fee only so the safe-minimum floor + // (see EstimateRedemptionFee) cannot produce a fee the Bridge would reject. if fee <= 0 { taskLogger.Infof("estimating redemption transaction fee") + _, _, _, txMaxTotalFee, _, _, _, err := rt.chain.GetRedemptionParameters() + if err != nil { + return nil, fmt.Errorf( + "cannot get redemption tx max total fee: [%w]", + err, + ) + } + estimatedFee, err := EstimateRedemptionFee( rt.btcChain, redeemersOutputScripts, + txMaxTotalFee, ) if err != nil { return nil, fmt.Errorf( @@ -462,10 +471,14 @@ redemptionRequestedLoop: } // EstimateRedemptionFee estimates fee for the redemption transaction that pays -// the provided redeemers output scripts. +// the provided redeemers output scripts. The estimated fee is floored at a safe +// minimum rate and bounded above by txMaxTotalFee (the Bridge maximum), so a +// non-RBF redemption is never broadcast below the floor where it could get stuck +// and jam the wallet. func EstimateRedemptionFee( btcChain bitcoin.Chain, redeemersOutputScripts []bitcoin.Script, + txMaxTotalFee uint64, ) (int64, error) { sizeEstimator := bitcoin.NewTransactionSizeEstimator(). // 1 P2WPKH main UTXO input. @@ -500,5 +513,19 @@ func EstimateRedemptionFee( return 0, fmt.Errorf("cannot estimate transaction fee: [%v]", err) } + // A raw estimate already above the Bridge maximum means the redemption is + // uneconomical to perform at the required fee; return an error rather than + // clamping to the maximum and broadcasting an underpriced transaction. + if uint64(totalFee) > txMaxTotalFee { + return 0, fmt.Errorf("estimated fee exceeds the maximum fee") + } + + // Enforce the safe minimum fee rate and buffer, bounded by the Bridge + // maximum. + totalFee, err = applyWalletTxFeeFloor(totalFee, transactionSize, txMaxTotalFee) + if err != nil { + return 0, err + } + return totalFee, nil } diff --git a/pkg/tbtcpg/redemptions_test.go b/pkg/tbtcpg/redemptions_test.go index 8f61e2f94b..aa56fdd74b 100644 --- a/pkg/tbtcpg/redemptions_test.go +++ b/pkg/tbtcpg/redemptions_test.go @@ -3,6 +3,7 @@ package tbtcpg_test import ( "encoding/hex" "math/big" + "strings" "testing" "github.com/go-test/deep" @@ -24,9 +25,6 @@ func TestEstimateRedemptionFee(t *testing.T) { return bytes } - btcChain := tbtcpg.NewLocalBitcoinChain() - btcChain.SetEstimateSatPerVByteFee(1, 16) - redeemersOutputScripts := []bitcoin.Script{ fromHex("76a9142cd680318747b720d67bf4246eb7403b476adb3488ac"), // P2PKH fromHex("0014e6f9d74726b19b75f16fe1e9feaec048aa4fa1d0"), // P2WPKH @@ -34,13 +32,61 @@ func TestEstimateRedemptionFee(t *testing.T) { fromHex("0020ef0b4d985752aa5ef6243e4c6f6bebc2a007e7d671ef27d4b1d0db8dcc93bc1c"), // P2WSH } - actualFee, err := tbtcpg.EstimateRedemptionFee(btcChain, redeemersOutputScripts) - if err != nil { - t.Fatal(err) + // The fixture above yields a 250 vByte redemption transaction. + const vsize = 250 + + tests := map[string]struct { + estimateSatPerVByte int64 + txMaxTotalFee uint64 + expectedFee int + expectErrorContains string + }{ + "estimate above the floor is buffered by 25%": { + estimateSatPerVByte: 16, + txMaxTotalFee: 100000, + expectedFee: 5000, // ceil(16*1.25)=20 sat/vByte * 250 vByte + }, + "low estimate is raised to the minimum floor": { + estimateSatPerVByte: 1, + txMaxTotalFee: 100000, + expectedFee: 1250, // max(5, ceil(1*1.25)=2)=5 sat/vByte * 250 vByte + }, + "minimum floor above the cap returns an error": { + estimateSatPerVByte: 1, + txMaxTotalFee: uint64(3 * vsize), // below the 5 sat/vByte floor + expectErrorContains: "minimum safe transaction fee", + }, } - expectedFee := 4000 // transactionVirtualSize * satPerVByteFee = 250 * 16 = 4000 - testutils.AssertIntsEqual(t, "fee", expectedFee, int(actualFee)) + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + btcChain := tbtcpg.NewLocalBitcoinChain() + btcChain.SetEstimateSatPerVByteFee(1, tc.estimateSatPerVByte) + + actualFee, err := tbtcpg.EstimateRedemptionFee( + btcChain, + redeemersOutputScripts, + tc.txMaxTotalFee, + ) + + if tc.expectErrorContains != "" { + if err == nil { + t.Fatalf("expected an error, got fee [%d]", actualFee) + } + if !strings.Contains(err.Error(), tc.expectErrorContains) { + t.Fatalf( + "expected error containing [%s]; got [%v]", + tc.expectErrorContains, err, + ) + } + return + } + if err != nil { + t.Fatal(err) + } + testutils.AssertIntsEqual(t, "fee", tc.expectedFee, int(actualFee)) + }) + } } func TestRedemptionAction_FindPendingRedemptions(t *testing.T) { @@ -168,7 +214,9 @@ func TestRedemptionAction_ProposeRedemption(t *testing.T) { fee: 0, // trigger fee estimation expectedProposal: &tbtc.RedemptionProposal{ RedeemersOutputScripts: redeemersOutputScripts, - RedemptionTxFee: big.NewInt(4300), + // raw 4300 (172 vByte * 25 sat/vByte), buffered to + // ceil(25*1.25)=32 sat/vByte * 172 = 5504, below the cap. + RedemptionTxFee: big.NewInt(5504), }, }, } @@ -180,6 +228,10 @@ func TestRedemptionAction_ProposeRedemption(t *testing.T) { btcChain.SetEstimateSatPerVByteFee(1, 25) + // Fee estimation bounds the safe-minimum floor by the redemption + // tx max total fee; set a cap comfortably above the buffered fee. + tbtcChain.SetRedemptionParameters(0, 0, 0, 6000, 0, nil, 0) + for _, script := range redeemersOutputScripts { tbtcChain.SetPendingRedemptionRequest( walletPublicKeyHash,