diff --git a/pkg/tbtcpg/deposit_sweep.go b/pkg/tbtcpg/deposit_sweep.go index b5c2346a25..2872af7bc9 100644 --- a/pkg/tbtcpg/deposit_sweep.go +++ b/pkg/tbtcpg/deposit_sweep.go @@ -21,26 +21,6 @@ import ( // This will ensure that deposit sweep transaction fees are not underestimated. const depositScriptByteSize = 126 -// minSweepTxSatPerVByteFee is the minimum fee rate, in sat/vByte, applied to -// deposit sweep transactions. 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 a sweep consolidates significant wallet value -// and is not RBF-enabled, it cannot be replaced once broadcast, so a floor-rate -// sweep can get stuck in the mempool and jam the wallet: no new sweep can be -// built while the previous one is unconfirmed. This minimum keeps the sweep fee -// safely above the relay floor while remaining far below the Bridge's -// per-deposit 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 below are a stopgap for -// the current fire-and-forget, non-RBF sweep path: because a stuck sweep 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 minSweepTxSatPerVByteFee = 5 - // DepositSweepLookBackBlocks is the look-back period in blocks used // when searching for submitted deposit-related events. It's equal to // 30 days assuming 12 seconds per block. @@ -510,7 +490,7 @@ func (dst *DepositSweepTask) ProposeDepositsSweep( // the deposits stay unswept. Log it distinctly at WARN so operators // can tell this apart from a benign "no deposits to sweep" outcome; // in particular, a safe-minimum-fee abort (see - // minSweepTxSatPerVByteFee) can indicate a misconfigured, too-low + // minWalletTxSatPerVByteFee) can indicate a misconfigured, too-low // per-deposit maximum fee that will strand deposits until governance // raises it. taskLogger.Warnf("cannot estimate sweep transaction fee: [%v]", err) @@ -580,9 +560,9 @@ func (dst *DepositSweepTask) ProposeDepositsSweep( // - 1 P2WPKH output // // An error is returned if any estimated fee exceeds the maximum fee allowed by -// the Bridge contract, or if the minimum safe sweep fee (see -// minSweepTxSatPerVByteFee) required to avoid a stuck, unbumpable sweep would -// itself exceed that Bridge maximum. +// the Bridge contract, or if the minimum safe fee (see minWalletTxSatPerVByteFee) +// required to avoid a stuck, unbumpable sweep would itself exceed that Bridge +// maximum. func EstimateDepositsSweepFee( chain Chain, btcChain bitcoin.Chain, @@ -677,24 +657,9 @@ func estimateDepositsSweepFee( return 0, 0, fmt.Errorf("estimated fee exceeds the maximum fee") } - // A sweep must never be broadcast below a safe minimum fee rate, or it may - // get stuck in the mempool and jam the wallet (see minSweepTxSatPerVByteFee). - // If even that minimum fee exceeds the Bridge maximum, a safe sweep cannot be - // constructed; return an error rather than silently broadcasting an - // underpriced transaction. - if uint64(minSweepTxSatPerVByteFee*transactionSize) > totalMaxFee { - return 0, 0, fmt.Errorf( - "minimum safe sweep fee [%d] exceeds the maximum fee [%d]", - minSweepTxSatPerVByteFee*transactionSize, - totalMaxFee, - ) - } - - // Add a 25% buffer over the oracle estimate so there is margin during the - // estimate-to-broadcast delay and the fee stays adaptive under congestion - // (see threshold-network/keep-core#4171), then enforce the minimum floor and - // bound the result by the Bridge maximum (which the floor cannot exceed, per - // the check above). + // 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 @@ -702,20 +667,9 @@ func estimateDepositsSweepFee( // 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 } // Compute the actual sat/vbyte fee for informational purposes. diff --git a/pkg/tbtcpg/deposit_sweep_fee_test.go b/pkg/tbtcpg/deposit_sweep_fee_test.go index eb79da7b4d..185da44781 100644 --- a/pkg/tbtcpg/deposit_sweep_fee_test.go +++ b/pkg/tbtcpg/deposit_sweep_fee_test.go @@ -100,7 +100,7 @@ func TestEstimateDepositsSweepFee_MinimumFloorAndBuffer(t *testing.T) { // substring pins this to the floor-exceeds-cap branch specifically, // distinguishing it from the raw-fee-exceeds-cap error. perDepositMaxFee: uint64(3 * size1), - expectErrorContains: "minimum safe sweep fee", + expectErrorContains: "minimum safe transaction fee", }, "multi-deposit minimum floor above the cap returns an error": { depositsCount: 3, @@ -111,7 +111,7 @@ func TestEstimateDepositsSweepFee_MinimumFloorAndBuffer(t *testing.T) { // the cap, so the raw-fee check passes and the floor branch is the // one exercised. perDepositMaxFee: uint64(size3), - expectErrorContains: "minimum safe sweep fee", + expectErrorContains: "minimum safe transaction fee", }, } diff --git a/pkg/tbtcpg/fee.go b/pkg/tbtcpg/fee.go new file mode 100644 index 0000000000..b1e2acc43e --- /dev/null +++ b/pkg/tbtcpg/fee.go @@ -0,0 +1,106 @@ +package tbtcpg + +import ( + "errors" + "fmt" +) + +// ErrMaxFeeTooLow indicates that the Bridge maximum total fee is too low to +// build a wallet transaction at the safe minimum fee rate, so a non-underpriced +// transaction cannot be constructed. It mirrors the raw-estimate-too-high +// sentinels (ErrFeeTooHigh, ErrSweepTxFeeTooHigh): both signal an unserviceable +// fee configuration, one bounding from above and one from below. +var ErrMaxFeeTooLow = errors.New( + "minimum safe transaction fee exceeds the maximum fee", +) + +// 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 total fee for the +// transaction). +// +// It returns ErrMaxFeeTooLow 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 in satoshis +// and txVsize is the estimated transaction virtual size in vBytes. +// +// The 25% buffer is applied to the truncated per-vByte rate +// (estimatedFee / txVsize). This is lossless only because EstimateFee returns +// the fee as satPerVByteFee * txVsize (an exact multiple of the vsize), so the +// integer division recovers the exact rate. If that contract ever changes so +// estimatedFee is no longer an exact multiple of txVsize, apply the buffer to +// estimatedFee directly instead of to the truncated rate; otherwise up to +// txVsize-1 sat is silently dropped before buffering and the tx is underpriced. +// +// maxTotalFee bounds only the total transaction fee. Where the Bridge also +// enforces a per-request cap (e.g. the redemption TxMaxFee), satisfying that +// cap is the caller's or on-chain validation's responsibility; this helper is +// unaware of it. Callers are expected to reject a raw estimate already above +// maxTotalFee before calling (all current callers do); the result is in any +// case clamped down to maxTotalFee. +// +// 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( + "%w: minimum fee [%d], maximum fee [%d]", + ErrMaxFeeTooLow, + minWalletTxSatPerVByteFee*txVsize, + maxTotalFee, + ) + } + + rate := estimatedFee / txVsize + rate = (rate*5 + 3) / 4 // ceil(rate * 1.25) + if rate < minWalletTxSatPerVByteFee { + rate = minWalletTxSatPerVByteFee + } + + // Clamp down to the Bridge maximum total fee. This can never drop the fee + // below the floor: the floor-vs-cap guard above already guaranteed + // maxTotalFee is at least the minimum floor total. + 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..cdb522c53e --- /dev/null +++ b/pkg/tbtcpg/fee_test.go @@ -0,0 +1,93 @@ +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, + }, + "buffered fee exactly at the cap is not clamped": { + estimatedFee: 4000, // rate 20 -> buffered 25 sat/vByte * 200 = 5000 + txVsize: vsize, + maxTotalFee: 5000, // exactly the buffered total + expectedFee: 5000, + }, + "minimum floor exactly at the cap is allowed": { + estimatedFee: 100, // rate 0 -> floored to 5 sat/vByte + txVsize: vsize, + maxTotalFee: 1000, // exactly the 5 sat/vByte floor total (5*200) + expectedFee: 1000, + }, + "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/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..d010b6613f 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), }, }, } @@ -423,35 +425,62 @@ func TestMovedFundsSweepAction_ProposeMovedFundsSweep(t *testing.T) { func TestEstimateMovedFundsSweepFee(t *testing.T) { var tests = map[string]struct { - sweepTxMaxTotalFee uint64 - hasMainUtxo bool - expectedFee uint64 - expectedError error + estimateSatPerVByte int64 + sweepTxMaxTotalFee uint64 + hasMainUtxo bool + expectedFee uint64 + expectedError error }{ "estimated fee correct, one input": { - sweepTxMaxTotalFee: 3000, - hasMainUtxo: false, - expectedFee: 1760, - expectedError: nil, + estimateSatPerVByte: 16, + sweepTxMaxTotalFee: 3000, + hasMainUtxo: false, + // 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, + estimateSatPerVByte: 16, + sweepTxMaxTotalFee: 3000, + hasMainUtxo: true, + // 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, + }, + "low estimate is raised to the minimum floor": { + estimateSatPerVByte: 1, + sweepTxMaxTotalFee: 3000, + hasMainUtxo: false, + // raw 110 (110 vByte * 1 sat/vByte), buffered ceil(1*1.25)=2 is + // below the 5 sat/vByte floor, so clamped to 5 * 110 = 550. + expectedFee: 550, + expectedError: nil, }, "estimated fee too high": { - sweepTxMaxTotalFee: 2500, - hasMainUtxo: true, + estimateSatPerVByte: 16, + sweepTxMaxTotalFee: 2500, + hasMainUtxo: true, + expectedFee: 0, + expectedError: tbtcpg.ErrSweepTxFeeTooHigh, + }, + "minimum floor exceeds the max total fee": { + estimateSatPerVByte: 1, + // raw 110 (110 vByte * 1 sat/vByte) is below the cap, so it passes + // the raw-estimate guard, but the 5 sat/vByte floor total (550) + // exceeds the cap, so a safe sweep cannot be built. + sweepTxMaxTotalFee: 400, + hasMainUtxo: false, expectedFee: 0, - expectedError: tbtcpg.ErrSweepTxFeeTooHigh, + expectedError: tbtcpg.ErrMaxFeeTooLow, }, } for testName, test := range tests { t.Run(testName, func(t *testing.T) { btcChain := tbtcpg.NewLocalBitcoinChain() - btcChain.SetEstimateSatPerVByteFee(1, 16) + btcChain.SetEstimateSatPerVByteFee(1, test.estimateSatPerVByte) actualFee, err := tbtcpg.EstimateMovedFundsSweepFee( btcChain, 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..c0b8a1913f 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), }, }, } @@ -649,26 +651,48 @@ func TestMovingFundsAction_ProposeMovingFunds(t *testing.T) { func TestEstimateMovingFundsFee(t *testing.T) { var tests = map[string]struct { - txMaxTotalFee uint64 - expectedFee uint64 - expectedError error + estimateSatPerVByte int64 + txMaxTotalFee uint64 + expectedFee uint64 + expectedError error }{ "estimated fee correct": { - txMaxTotalFee: 6000, - expectedFee: 3248, + estimateSatPerVByte: 16, + txMaxTotalFee: 6000, + // 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, + }, + "low estimate is raised to the minimum floor": { + estimateSatPerVByte: 1, + txMaxTotalFee: 6000, + // raw 203 (203 vByte * 1 sat/vByte), buffered ceil(1*1.25)=2 is + // below the 5 sat/vByte floor, so clamped to 5 * 203 = 1015. + expectedFee: 1015, expectedError: nil, }, "estimated fee too high": { - txMaxTotalFee: 3000, + estimateSatPerVByte: 16, + txMaxTotalFee: 3000, + expectedFee: 0, + expectedError: tbtcpg.ErrFeeTooHigh, + }, + "minimum floor exceeds the max total fee": { + estimateSatPerVByte: 1, + // raw 203 (203 vByte * 1 sat/vByte) is below the cap, so it passes + // the raw-estimate guard, but the 5 sat/vByte floor total (1015) + // exceeds the cap, so a safe transaction cannot be built. + txMaxTotalFee: 500, expectedFee: 0, - expectedError: tbtcpg.ErrFeeTooHigh, + expectedError: tbtcpg.ErrMaxFeeTooLow, }, } for testName, test := range tests { t.Run(testName, func(t *testing.T) { btcChain := tbtcpg.NewLocalBitcoinChain() - btcChain.SetEstimateSatPerVByteFee(1, 16) + btcChain.SetEstimateSatPerVByteFee(1, test.estimateSatPerVByte) targetWalletsCount := 4 diff --git a/pkg/tbtcpg/redemptions.go b/pkg/tbtcpg/redemptions.go index 981e9a8eb7..6587d2dffe 100644 --- a/pkg/tbtcpg/redemptions.go +++ b/pkg/tbtcpg/redemptions.go @@ -210,16 +210,29 @@ 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. Here we bound the estimate by the maximum + // total fee only. The per-request maximum fee (TxMaxFee, snapshotted per + // request at request creation) is enforced solely by the on-chain + // validation of the proposal and is not checked here. Note that the + // safe-minimum floor (see EstimateRedemptionFee) raises the total fee and + // therefore each request's fee share, so it can push a share above that + // request's per-request maximum even while the total stays within + // txMaxTotalFee; such a proposal is rejected only by on-chain validation. if fee <= 0 { taskLogger.Infof("estimating redemption transaction fee") + _, _, txMaxFee, 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( @@ -229,6 +242,31 @@ func (rt *RedemptionTask) ProposeRedemption( } fee = estimatedFee + + // The safe-minimum floor raises the total fee and therefore each + // request's fee share. When a share exceeds the per-request maximum + // fee, on-chain validation rejects the whole proposal and the batch is + // aborted with no lower-fee retry. Emit a distinct warning so operators + // can tell this apart from a generic validation failure. The largest + // share is the worst case to check: the on-chain fee distribution + // (see withRedemptionTotalFee) splits the total evenly and assigns the + // division remainder to the last request, so that request pays + // floor(total/count) + total%count. Checking only the even floor share + // would miss a rejection caused solely by the remainder. txMaxFee is + // the current governance parameter; the actually enforced cap is the + // value snapshotted per request at creation, so this is a best-effort + // diagnostic rather than an exact predictor. + requestsCount := int64(len(redeemersOutputScripts)) + maxShare := fee/requestsCount + fee%requestsCount + if uint64(maxShare) > txMaxFee { + taskLogger.Warnf( + "floored redemption fee share [%d] exceeds the per-request "+ + "maximum fee [%d]; the proposal will likely be rejected "+ + "by on-chain validation", + maxShare, + txMaxFee, + ) + } } taskLogger.Infof("redemption transaction fee: [%d]", fee) @@ -462,10 +500,15 @@ 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 total-fee +// maximum), so a non-RBF redemption is not proposed below the floor where it +// could get stuck and jam the wallet. Only the total fee is bounded here; the +// per-request maximum fee is enforced separately by on-chain validation. func EstimateRedemptionFee( btcChain bitcoin.Chain, redeemersOutputScripts []bitcoin.Script, + txMaxTotalFee uint64, ) (int64, error) { sizeEstimator := bitcoin.NewTransactionSizeEstimator(). // 1 P2WPKH main UTXO input. @@ -500,5 +543,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..2aef02fa73 100644 --- a/pkg/tbtcpg/redemptions_test.go +++ b/pkg/tbtcpg/redemptions_test.go @@ -2,7 +2,9 @@ package tbtcpg_test import ( "encoding/hex" + "fmt" "math/big" + "strings" "testing" "github.com/go-test/deep" @@ -24,9 +26,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 +33,66 @@ 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", + }, + "raw estimate above the cap returns an error": { + estimateSatPerVByte: 16, // raw 16*250 = 4000 + txMaxTotalFee: 3000, // below the raw estimate + expectErrorContains: "estimated fee exceeds the maximum 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 +220,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 +234,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, @@ -216,3 +274,124 @@ func TestRedemptionAction_ProposeRedemption(t *testing.T) { }) } } + +// warnCapturingLogger records Warnf messages so tests can assert on the +// per-request fee warning emitted during redemption fee estimation. All other +// log methods are inherited as no-ops from testutils.MockLogger. +type warnCapturingLogger struct { + testutils.MockLogger + warnings []string +} + +func (l *warnCapturingLogger) Warnf(format string, args ...interface{}) { + l.warnings = append(l.warnings, fmt.Sprintf(format, args...)) +} + +// TestRedemptionAction_ProposeRedemption_PerRequestFeeWarning verifies that the +// diagnostic warning about a floored fee share exceeding the per-request maximum +// fee (TxMaxFee) is emitted for the worst-case (largest) share. The on-chain fee +// distribution assigns the division remainder to the last request, so that +// request pays floor(total/count)+total%count. The warning must reflect that +// last-request share, not the smaller even share. +func TestRedemptionAction_ProposeRedemption_PerRequestFeeWarning(t *testing.T) { + fromHex := func(hexString string) []byte { + bytes, err := hex.DecodeString(hexString) + if err != nil { + t.Fatal(err) + } + return bytes + } + + var walletPublicKeyHash [20]byte + copy(walletPublicKeyHash[:], fromHex("")) + + // Three redeemer output scripts make the estimated fee (6496 at 25 sat/vByte + // buffered to 32) indivisible by the request count: the even share is + // 6496/3 = 2165 and the last request pays 2165 + 6496%3 = 2166. + redeemersOutputScripts := []bitcoin.Script{ + fromHex("00140000000000000000000000000000000000000001"), + fromHex("00140000000000000000000000000000000000000002"), + fromHex("00140000000000000000000000000000000000000003"), + } + + var tests = map[string]struct { + txMaxFee uint64 + expectWarning bool + }{ + "worst-case share within the per-request cap": { + txMaxFee: 3000, // 2166 <= 3000 + expectWarning: false, + }, + "even share at the cap but last-request share exceeds it": { + // The even share 2165 equals the cap (a floor-division check would + // not warn), but the last request pays 2166 and would be rejected. + txMaxFee: 2165, + expectWarning: true, + }, + } + + for testName, test := range tests { + t.Run(testName, func(t *testing.T) { + tbtcChain := tbtcpg.NewLocalChain() + btcChain := tbtcpg.NewLocalBitcoinChain() + + btcChain.SetEstimateSatPerVByteFee(1, 25) + + // txMaxFee at index 2; txMaxTotalFee at index 3, set comfortably + // above the estimated total (6496) so it does not bound the fee. + tbtcChain.SetRedemptionParameters(0, 0, test.txMaxFee, 8000, 0, nil, 0) + + for _, script := range redeemersOutputScripts { + tbtcChain.SetPendingRedemptionRequest( + walletPublicKeyHash, + &tbtc.RedemptionRequest{ + RedeemerOutputScript: script, + }, + ) + } + + expectedProposal := &tbtc.RedemptionProposal{ + RedeemersOutputScripts: redeemersOutputScripts, + RedemptionTxFee: big.NewInt(6496), + } + + err := tbtcChain.SetRedemptionProposalValidationResult( + walletPublicKeyHash, + expectedProposal, + true, + ) + if err != nil { + t.Fatal(err) + } + + task := tbtcpg.NewRedemptionTask(tbtcChain, btcChain) + + logger := &warnCapturingLogger{} + + _, err = task.ProposeRedemption( + logger, + walletPublicKeyHash, + redeemersOutputScripts, + 0, // trigger fee estimation + ) + if err != nil { + t.Fatal(err) + } + + warned := false + for _, w := range logger.warnings { + if strings.Contains(w, "exceeds the per-request maximum fee") { + warned = true + break + } + } + + if warned != test.expectWarning { + t.Errorf( + "per-request fee warning emitted = %v, want %v\nwarnings: %v", + warned, test.expectWarning, logger.warnings, + ) + } + }) + } +}