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

pkg/tbtc/deposit_sweep.go

@@ -49,6 +49,20 @@ 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. It is exported so the external
+	// tbtc_test package can compare it against the canonical tbtcpg value
+	// (guarded by TestSweepFeeConstantsMirrorTbtcpg). 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.
+	// Exported alongside MinSweepTxSatPerVByteFee for the same cross-package
+	// drift guard.
+	DepositScriptByteSize = 126
 )
 
 // DepositSweepProposal represents a deposit sweep proposal issued by a
@@ -461,6 +475,61 @@ 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 {
+		minSweepTxFee := big.NewInt(int64(MinSweepTxSatPerVByteFee) * sweepTxSize)
+
+		switch {
+		// This branch is defense-in-depth for test/mock chain implementations
+		// and is not expected to be reachable on the real production path: by
+		// the time control reaches this point, chain.ValidateDepositSweepProposal
+		// above has already ABI-packed proposal.SweepTxFee to call the on-chain
+		// WalletProposalValidator, which panics on a nil *big.Int before this
+		// code ever runs. Likewise, a proposal decoded off the wire
+		// (DepositSweepProposal.Unmarshal in marshaling.go) always constructs
+		// SweepTxFee via new(big.Int).SetBytes(...), which never yields nil.
+		case proposal.SweepTxFee == nil:
+			validateProposalLogger.Warnf(
+				"proposal has no sweep tx fee set; expected at least the safe "+
+					"minimum [%d] ([%d] sat/vByte * [%d] vByte)",
+				minSweepTxFee,
+				MinSweepTxSatPerVByteFee,
+				sweepTxSize,
+			)
+		case proposal.SweepTxFee.Cmp(minSweepTxFee) < 0:
+			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/deposit_sweep_test.go

@@ -318,3 +318,141 @@ func TestAssembleDepositSweepTransaction(t *testing.T) {
 		})
 	}
 }
+
+// capturingLogger wraps testutils.MockLogger and records Warnf calls for
+// assertions.
+type capturingLogger struct {
+	testutils.MockLogger
+	warnings []string
+}
+
+func (cl *capturingLogger) Warnf(format string, args ...interface{}) {
+	cl.warnings = append(cl.warnings, fmt.Sprintf(format, args...))
+}
+
+// depositSweepFeeCheckChain is a minimal stub satisfying the chain interface
+// ValidateDepositSweepProposal requires. Its ValidateDepositSweepProposal
+// unconditionally reports the proposal as valid, and its other two methods
+// are never invoked for a proposal with no deposits. This isolates the
+// follower-side sweep-fee soft check (deposit_sweep.go, below the
+// "calling chain for proposal validation" log line) from on-chain proposal
+// validation and deposit-lookup concerns that the soft check does not
+// depend on.
+type depositSweepFeeCheckChain struct{}
+
+func (depositSweepFeeCheckChain) PastDepositRevealedEvents(
+	*DepositRevealedEventFilter,
+) ([]*DepositRevealedEvent, error) {
+	return nil, nil
+}
+
+func (depositSweepFeeCheckChain) ValidateDepositSweepProposal(
+	[20]byte,
+	*DepositSweepProposal,
+	[]struct {
+		*Deposit
+		FundingTx *bitcoin.Transaction
+	},
+) error {
+	return nil
+}
+
+func (depositSweepFeeCheckChain) GetDepositRequest(
+	bitcoin.Hash,
+	uint32,
+) (*DepositChainRequest, bool, error) {
+	return nil, false, nil
+}
+
+// TestValidateDepositSweepProposal_SweepFeeSoftCheck exercises the
+// follower-side soft check on the leader-proposed sweep fee. The check is
+// log-only by design (see threshold-network/keep-core#4171): it must warn
+// about an unsafe fee but must never fail proposal validation because of it.
+func TestValidateDepositSweepProposal_SweepFeeSoftCheck(t *testing.T) {
+	var walletPublicKeyHash [20]byte
+	stubChain := depositSweepFeeCheckChain{}
+	btcChain := newLocalBitcoinChain()
+
+	// Compute the exact safe-minimum fee for a proposal with no deposits
+	// using the same estimator call the soft check itself performs
+	// (deposit_sweep.go), so the boundary between "below" and "at/above" the
+	// floor is derived rather than hardcoded.
+	sweepTxSize, err := bitcoin.NewTransactionSizeEstimator().
+		AddPublicKeyHashInputs(1, true).
+		AddScriptHashInputs(0, DepositScriptByteSize, true).
+		AddPublicKeyHashOutputs(1, true).
+		VirtualSize()
+	if err != nil {
+		t.Fatal(err)
+	}
+	minSweepTxFee := big.NewInt(int64(MinSweepTxSatPerVByteFee) * sweepTxSize)
+
+	scenarios := map[string]struct {
+		fee        *big.Int
+		expectWarn bool
+	}{
+		"fee below the safe minimum": {
+			fee:        new(big.Int).Sub(minSweepTxFee, big.NewInt(1)),
+			expectWarn: true,
+		},
+		"fee at the safe minimum": {
+			fee:        minSweepTxFee,
+			expectWarn: false,
+		},
+		"fee above the safe minimum": {
+			fee:        new(big.Int).Add(minSweepTxFee, big.NewInt(1000)),
+			expectWarn: false,
+		},
+		// A nil SweepTxFee cannot occur on the real production path (see the
+		// comment on the nil case in deposit_sweep.go): the on-chain
+		// WalletProposalValidator call a few lines above the soft check
+		// already ABI-packs the fee and panics first, and wire
+		// deserialization always constructs a non-nil value. This scenario
+		// exists to lock in the defense-in-depth behavior for callers, like
+		// this test's stub chain, that can hand the soft check a nil fee
+		// directly.
+		"nil fee from a test/mock caller": {
+			fee:        nil,
+			expectWarn: true,
+		},
+	}
+
+	for name, scenario := range scenarios {
+		t.Run(name, func(t *testing.T) {
+			proposal := &DepositSweepProposal{
+				SweepTxFee: scenario.fee,
+			}
+
+			logger := &capturingLogger{}
+
+			_, err := ValidateDepositSweepProposal(
+				logger,
+				walletPublicKeyHash,
+				proposal,
+				0,
+				stubChain,
+				btcChain,
+			)
+			if err != nil {
+				t.Fatalf(
+					"expected the log-only soft check to never fail "+
+						"validation; got error: [%v]",
+					err,
+				)
+			}
+
+			gotWarn := len(logger.warnings) > 0
+			if gotWarn != scenario.expectWarn {
+				t.Errorf(
+					"unexpected warning presence for fee [%v]\n"+
+						"expected warning: %v\nactual warning:   %v\n"+
+						"captured warnings: %v",
+					scenario.fee,
+					scenario.expectWarn,
+					gotWarn,
+					logger.warnings,
+				)
+			}
+		})
+	}
+}

pkg/tbtc/sweep_fee_sync_test.go

@@ -0,0 +1,43 @@
+package tbtc_test
+
+import (
+	"testing"
+
+	"github.com/keep-network/keep-core/pkg/tbtc"
+	"github.com/keep-network/keep-core/pkg/tbtcpg"
+)
+
+// TestSweepFeeConstantsMirrorTbtcpg guards the sweep-fee constants that pkg/tbtc
+// 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 both pkg/tbtc and pkg/tbtcpg without forming the cycle. It
+// compares the two actual constants directly - not against hand-copied literals
+// - so it fails whenever the pkg/tbtc mirror and the canonical tbtcpg value
+// drift apart, regardless of which side was changed. A literal-based guard
+// could be defeated by updating tbtcpg and the literal together while forgetting
+// the pkg/tbtc mirror; comparing the live values closes that gap.
+func TestSweepFeeConstantsMirrorTbtcpg(t *testing.T) {
+	if tbtc.MinSweepTxSatPerVByteFee != tbtcpg.MinWalletTxSatPerVByteFee {
+		t.Errorf(
+			"tbtc.MinSweepTxSatPerVByteFee [%d] has drifted from the canonical "+
+				"tbtcpg.MinWalletTxSatPerVByteFee [%d]; the follower soft check "+
+				"would warn at the wrong threshold",
+			tbtc.MinSweepTxSatPerVByteFee,
+			tbtcpg.MinWalletTxSatPerVByteFee,
+		)
+	}
+
+	if tbtc.DepositScriptByteSize != tbtcpg.DepositScriptByteSize {
+		t.Errorf(
+			"tbtc.DepositScriptByteSize [%d] has drifted from the canonical "+
+				"tbtcpg.DepositScriptByteSize [%d]; the follower soft check would "+
+				"estimate the sweep tx size incorrectly",
+			tbtc.DepositScriptByteSize,
+			tbtcpg.DepositScriptByteSize,
+		)
+	}
+}

pkg/tbtcpg/deposit_sweep.go

@@ -17,29 +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
-
-// 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
+// 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
@@ -510,7 +491,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)
@@ -581,7 +562,7 @@ func (dst *DepositSweepTask) ProposeDepositsSweep(
 //
 // 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
+// MinWalletTxSatPerVByteFee) required to avoid a stuck, unbumpable sweep would
 // itself exceed that Bridge maximum.
 func EstimateDepositsSweepFee(
 	chain Chain,
@@ -653,7 +634,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()
@@ -677,45 +658,19 @@ 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
 	// 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)
+	totalFee, err = applyWalletTxFeeFloor(totalFee, transactionSize, totalMaxFee)
+	if err != nil {
+		return 0, 0, err
 	}
 
 	// Compute the actual sat/vbyte fee for informational purposes.