Add follower-side soft check for below-floor sweep fees

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

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,
+		)
+	}
+}

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))
 

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,
+				)
+			}
+		})
+	}
+}

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
+}