Enforce a minimum fee rate for deposit sweep transactions

pkg/tbtcpg/deposit_sweep.go

@@ -21,6 +21,26 @@ 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.
@@ -486,6 +506,14 @@ func (dst *DepositSweepTask) ProposeDepositsSweep(
 			perDepositMaxFee,
 		)
 		if err != nil {
+			// A failure here means no sweep proposal is produced this round, so
+			// 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
+			// per-deposit maximum fee that will strand deposits until governance
+			// raises it.
+			taskLogger.Warnf("cannot estimate sweep transaction fee: [%v]", err)
 			return nil, fmt.Errorf("cannot estimate sweep transaction fee: [%v]", err)
 		}
 
@@ -551,8 +579,10 @@ func (dst *DepositSweepTask) ProposeDepositsSweep(
 //     be underestimated in some rare cases.
 //   - 1 P2WPKH output
 //
-// If any of the estimated fees exceed the maximum fee allowed by the Bridge
-// contract, an error is returned as result.
+// 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.
 func EstimateDepositsSweepFee(
 	chain Chain,
 	btcChain bitcoin.Chain,
@@ -641,10 +671,53 @@ 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")
 	}
 
+	// 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).
+	//
+	// 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)
+	}
+
 	// Compute the actual sat/vbyte fee for informational purposes.
 	satPerVByteFee := math.Round(float64(totalFee) / float64(transactionSize))
 

pkg/tbtcpg/deposit_sweep_fee_test.go

@@ -0,0 +1,166 @@
+package tbtcpg_test
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/keep-network/keep-core/pkg/bitcoin"
+	"github.com/keep-network/keep-core/pkg/tbtcpg"
+)
+
+// sweepVirtualSize returns the estimated virtual size of a sweep transaction
+// with the given number of deposit inputs, mirroring the sizing that
+// EstimateDepositsSweepFee performs internally: 1 P2WPKH main-UTXO input,
+// depositsCount P2WSH deposit inputs, and 1 P2WPKH output. 126 ==
+// depositScriptByteSize.
+func sweepVirtualSize(t *testing.T, depositsCount int) int64 {
+	t.Helper()
+	size, err := bitcoin.NewTransactionSizeEstimator().
+		AddPublicKeyHashInputs(1, true).
+		AddScriptHashInputs(depositsCount, 126, true).
+		AddPublicKeyHashOutputs(1, true).
+		VirtualSize()
+	if err != nil {
+		t.Fatal(err)
+	}
+	return size
+}
+
+// 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%, the buffered fee is bounded by the Bridge maximum,
+// and a Bridge maximum below either the raw estimate or the minimum floor
+// returns an error rather than silently broadcasting an underpriced sweep. Both
+// the informational SatPerVByteFee and the TotalFee actually broadcast on-chain
+// are asserted, and multi-deposit sweeps (where transactionSize grows
+// sub-linearly while totalMaxFee grows linearly) are exercised on both the happy
+// path and the floor-exceeds-cap error branch.
+func TestEstimateDepositsSweepFee_MinimumFloorAndBuffer(t *testing.T) {
+	// Virtual sizes used to pin the cap and the expected total fee (the on-chain
+	// value) relative to the fee rate. The cap and expected-total expectations
+	// are given as explicit multiples of the size rather than derived from the
+	// rounded rate, so a TotalFee bug that rounds back to the expected rate still
+	// fails the test.
+	size1 := sweepVirtualSize(t, 1)
+	size3 := sweepVirtualSize(t, 3)
+
+	tests := map[string]struct {
+		depositsCount          int
+		estimateSatPerVByte    int64
+		perDepositMaxFee       uint64
+		expectedSatPerVByteFee int64
+		expectedTotalFee       int64
+		expectErrorContains    string
+	}{
+		"low estimate is raised to the minimum floor": {
+			depositsCount:          1,
+			estimateSatPerVByte:    1,
+			perDepositMaxFee:       100000,
+			expectedSatPerVByteFee: 5, // max(5, ceil(1*1.25)=2) = 5
+			expectedTotalFee:       5 * size1,
+		},
+		"estimate above the floor is buffered by 25%": {
+			depositsCount:          1,
+			estimateSatPerVByte:    20,
+			perDepositMaxFee:       100000,
+			expectedSatPerVByteFee: 25, // ceil(20*1.25) = 25
+			expectedTotalFee:       25 * size1,
+		},
+		"multi-deposit estimate is buffered by 25%": {
+			depositsCount:          3,
+			estimateSatPerVByte:    20,
+			perDepositMaxFee:       100000,
+			expectedSatPerVByteFee: 25, // ceil(20*1.25) = 25
+			expectedTotalFee:       25 * size3,
+		},
+		"buffered estimate above the cap is bounded to the cap": {
+			depositsCount:       1,
+			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 * size1),
+			expectedSatPerVByteFee: 22,
+			expectedTotalFee:       22 * size1, // the cap itself, not 25*size1
+		},
+		"raw estimate above the cap returns an error": {
+			depositsCount:       1,
+			estimateSatPerVByte: 30,
+			// The raw 30*size fee already exceeds the 10*size cap, so the sweep
+			// is uneconomical and the raw-fee check must error before the
+			// minimum-floor logic runs. The substring pins this to the
+			// raw-exceeds-cap branch, distinguishing it from the floor branch.
+			perDepositMaxFee:    uint64(10 * size1),
+			expectErrorContains: "estimated fee exceeds the maximum fee",
+		},
+		"minimum floor above the cap returns an error": {
+			depositsCount:       1,
+			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 * size1),
+			expectErrorContains: "minimum safe sweep fee",
+		},
+		"multi-deposit minimum floor above the cap returns an error": {
+			depositsCount:       3,
+			estimateSatPerVByte: 1,
+			// At N=3 the total cap is 3*size3 (linear in N) while the floor is
+			// 5*size3 (scales with the sub-linear tx size), so the floor exceeds
+			// the cap and the sweep must error. The raw 1*size3 fee stays under
+			// the cap, so the raw-fee check passes and the floor branch is the
+			// one exercised.
+			perDepositMaxFee:    uint64(size3),
+			expectErrorContains: "minimum safe sweep 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, test.depositsCount,
+			)
+
+			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)
+			}
+
+			fee := fees[test.depositsCount]
+			if fee.SatPerVByteFee != test.expectedSatPerVByteFee {
+				t.Errorf(
+					"unexpected sweep fee rate\nexpected: [%d] sat/vByte\nactual:   [%d] sat/vByte",
+					test.expectedSatPerVByteFee, fee.SatPerVByteFee,
+				)
+			}
+			// TotalFee is the value actually broadcast on-chain; assert it
+			// directly. SatPerVByteFee alone is lossy: math.Round collapses a
+			// range of TotalFee values onto the same rate (e.g. in the cap case
+			// both 22*size and 22*size+1 round to 22), so a TotalFee bug would be
+			// invisible if only the rate were checked.
+			if fee.TotalFee != test.expectedTotalFee {
+				t.Errorf(
+					"unexpected sweep total fee\nexpected: [%d] sat\nactual:   [%d] sat",
+					test.expectedTotalFee, fee.TotalFee,
+				)
+			}
+		})
+	}
+}

pkg/tbtcpg/internal/test/testdata/propose_sweep_scenario_0.json

@@ -37,7 +37,7 @@
         "FundingOutputIndex": 3
       }
     ],
-    "SweepTxFee": 10634,
+    "SweepTxFee": 13497,
     "DepositsRevealBlocks": [11, 31, 32]
   }
 }