fix: address multi-agent review findings from testnet4 deployment PR (#3932)

cmd/maintainer.go

@@ -44,7 +44,7 @@ func init() {
 func maintainers(cmd *cobra.Command, args []string) error {
 	ctx := context.Background()
 
-	btcChain, err := electrum.Connect(ctx, clientConfig.Bitcoin.Electrum)
+	btcChain, err := electrum.Connect(ctx, clientConfig.Bitcoin.Network, clientConfig.Bitcoin.Electrum)
 	if err != nil {
 		return fmt.Errorf("could not connect to Electrum chain: [%v]", err)
 	}

cmd/maintainercli.go

@@ -86,7 +86,7 @@ var listDepositsCommand = cobra.Command{
 			)
 		}
 
-		btcChain, err := electrum.Connect(ctx, clientConfig.Bitcoin.Electrum)
+		btcChain, err := electrum.Connect(ctx, clientConfig.Bitcoin.Network, clientConfig.Bitcoin.Electrum)
 		if err != nil {
 			return fmt.Errorf("could not connect to Electrum chain: [%v]", err)
 		}
@@ -179,7 +179,7 @@ var estimateDepositsSweepFeeCommand = cobra.Command{
 			)
 		}
 
-		btcChain, err := electrum.Connect(ctx, clientConfig.Bitcoin.Electrum)
+		btcChain, err := electrum.Connect(ctx, clientConfig.Bitcoin.Network, clientConfig.Bitcoin.Electrum)
 		if err != nil {
 			return fmt.Errorf("could not connect to Electrum chain: [%v]", err)
 		}
@@ -296,7 +296,7 @@ var submitDepositSweepProofCommand = cobra.Command{
 			)
 		}
 
-		btcChain, err := electrum.Connect(ctx, clientConfig.Bitcoin.Electrum)
+		btcChain, err := electrum.Connect(ctx, clientConfig.Bitcoin.Network, clientConfig.Bitcoin.Electrum)
 		if err != nil {
 			return fmt.Errorf("could not connect to Electrum chain: [%v]", err)
 		}
@@ -386,7 +386,7 @@ var submitRedemptionProofCommand = cobra.Command{
 			)
 		}
 
-		btcChain, err := electrum.Connect(ctx, clientConfig.Bitcoin.Electrum)
+		btcChain, err := electrum.Connect(ctx, clientConfig.Bitcoin.Network, clientConfig.Bitcoin.Electrum)
 		if err != nil {
 			return fmt.Errorf("could not connect to Electrum chain: [%v]", err)
 		}

cmd/start.go

@@ -110,7 +110,7 @@ func start(cmd *cobra.Command) error {
 	// Skip initialization for bootstrap nodes as they are only used for network
 	// discovery.
 	if !isBootstrap() {
-		btcChain, err := electrum.Connect(ctx, clientConfig.Bitcoin.Electrum)
+		btcChain, err := electrum.Connect(ctx, clientConfig.Bitcoin.Network, clientConfig.Bitcoin.Electrum)
 		if err != nil {
 			return fmt.Errorf("could not connect to Electrum chain: [%v]", err)
 		}

pkg/bitcoin/electrum/electrum.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"crypto/sha256"
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"math"
 	"sort"
@@ -32,10 +33,11 @@ type Connection struct {
 	client      *electrum.Client
 	clientMutex *sync.Mutex
 	config      Config
+	network     bitcoin.Network
 }
 
 // Connect initializes handle with provided Config.
-func Connect(parentCtx context.Context, config Config) (bitcoin.Chain, error) {
+func Connect(parentCtx context.Context, network bitcoin.Network, config Config) (bitcoin.Chain, error) {
 	if config.ConnectTimeout == 0 {
 		config.ConnectTimeout = DefaultConnectTimeout
 	}
@@ -56,6 +58,7 @@ func Connect(parentCtx context.Context, config Config) (bitcoin.Chain, error) {
 		parentCtx:   parentCtx,
 		config:      config,
 		clientMutex: &sync.Mutex{},
+		network:     network,
 	}
 
 	if err := c.electrumConnect(); err != nil {
@@ -988,21 +991,28 @@ func feeEstimateWithFallbackTargets(primary uint32) []uint32 {
 // deposit sweep max-fee checks on the Bridge bound the total fee.
 const defaultFallbackSatPerVByteWhenEstimateFails int64 = 2
 
-// isElectrumFeeOracleFailure reports whether the error is the usual
-// "no fee data" / JSON-RPC -32603 from estimatefee, as opposed to transport
-// or auth failures where we should not invent a feerate.
+// feeOracleFailure wraps errors that indicate the Electrum server could not
+// produce a fee estimate (as opposed to transport or auth failures). Using a
+// typed sentinel allows EstimateSatPerVByteFee to distinguish oracle failures
+// from general connectivity failures without string matching at the call site.
+type feeOracleFailure struct{ cause error }
+
+func (f feeOracleFailure) Error() string { return f.cause.Error() }
+func (f feeOracleFailure) Unwrap() error { return f.cause }
+
+// isElectrumFeeOracleFailure reports whether err originated from the fee
+// estimation oracle (i.e. the server responded but had no fee data), as
+// opposed to a transport or auth failure where inventing a feerate is wrong.
 func isElectrumFeeOracleFailure(err error) bool {
-	if err == nil {
-		return false
-	}
-	s := err.Error()
-	return strings.Contains(s, "cannot estimate fee") ||
-		strings.Contains(s, "-32603")
+	var f feeOracleFailure
+	return errors.As(err, &f)
 }
 
 // getFeeBtcPerKbOnce issues a single blockchain.estimatefee call (no multi-minute
 // retry loop). Persistent RPC errors for one confirmation target should not
 // exhaust RequestRetryTimeout; EstimateSatPerVByteFee tries looser targets next.
+// Errors that indicate the server processed the request but has no fee data are
+// wrapped in feeOracleFailure so callers can distinguish them from transport errors.
 func (c *Connection) getFeeBtcPerKbOnce(blocks uint32) (float32, error) {
 	if err := c.reconnectIfShutdown(); err != nil {
 		return 0, err
@@ -1016,7 +1026,12 @@ func (c *Connection) getFeeBtcPerKbOnce(blocks uint32) (float32, error) {
 	fee, err := c.client.GetFee(requestCtx, blocks)
 	c.clientMutex.Unlock()
 	if err != nil {
-		return 0, fmt.Errorf("request failed: [%w]", err)
+		wrapped := fmt.Errorf("request failed: [%w]", err)
+		s := wrapped.Error()
+		if strings.Contains(s, "cannot estimate fee") || strings.Contains(s, "-32603") {
+			return 0, feeOracleFailure{wrapped}
+		}
+		return 0, wrapped
 	}
 	return fee, nil
 }
@@ -1062,14 +1077,7 @@ func (c *Connection) EstimateSatPerVByteFee(blocks uint32) (int64, error) {
 	}
 
 	if sawFeeOracleFailure {
-		logger.Warnf(
-			"Electrum returned no fee estimate for any target %v; using "+
-				"fallback [%d] sat/vbyte (last error: [%v])",
-			targets,
-			defaultFallbackSatPerVByteWhenEstimateFails,
-			lastErr,
-		)
-		return defaultFallbackSatPerVByteWhenEstimateFails, nil
+		return feeOracleFallback(c.network, targets, lastErr)
 	}
 
 	if lastErr != nil {
@@ -1081,6 +1089,32 @@ func (c *Connection) EstimateSatPerVByteFee(blocks uint32) (int64, error) {
 	)
 }
 
+// feeOracleFallback decides what to return when all Electrum fee oracle calls
+// failed. On mainnet it refuses to invent a feerate; on other networks it
+// returns the static fallback so testnet4 deposits can still be swept.
+func feeOracleFallback(
+	network bitcoin.Network,
+	targets []uint32,
+	lastErr error,
+) (int64, error) {
+	if network == bitcoin.Mainnet {
+		return 0, fmt.Errorf(
+			"Electrum fee oracle returned no estimate for any target %v "+
+				"(last error: [%v]); refusing static fallback on mainnet",
+			targets,
+			lastErr,
+		)
+	}
+	logger.Warnf(
+		"Electrum returned no fee estimate for any target %v; using "+
+			"fallback [%d] sat/vbyte (last error: [%v])",
+		targets,
+		defaultFallbackSatPerVByteWhenEstimateFails,
+		lastErr,
+	)
+	return defaultFallbackSatPerVByteWhenEstimateFails, nil
+}
+
 func convertBtcKbToSatVByte(btcPerKbFee float32) int64 {
 	// To convert from BTC/KB to sat/vbyte, we need to multiply by 1e8/1e3.
 	satPerVByte := (1e8 / 1e3) * float64(btcPerKbFee)
@@ -1170,6 +1204,9 @@ func (c *Connection) keepAlive() {
 				)
 			} else {
 				// Adjust ticker starting at the time of the latest successful ping.
+				// Stop the current ticker before replacing it to avoid leaking the
+				// internal goroutine and timer resource.
+				ticker.Stop()
 				ticker = time.NewTicker(c.config.KeepAliveInterval)
 			}
 		case <-c.parentCtx.Done():

pkg/bitcoin/electrum/electrum_integration_test.go

@@ -147,14 +147,14 @@ func init() {
 
 func TestConnect_Integration(t *testing.T) {
 	runParallel(t, func(t *testing.T, testConfig testConfig) {
-		_, cancelCtx := newTestConnection(t, testConfig.clientConfig)
+		_, cancelCtx := newTestConnection(t, testConfig)
 		defer cancelCtx()
 	})
 }
 
 func TestGetTransaction_Integration(t *testing.T) {
 	runParallel(t, func(t *testing.T, testConfig testConfig) {
-		electrum, cancelCtx := newTestConnection(t, testConfig.clientConfig)
+		electrum, cancelCtx := newTestConnection(t, testConfig)
 		defer cancelCtx()
 
 		for txName, tx := range testData.Transactions[testConfig.network] {
@@ -180,7 +180,7 @@ func TestGetTransaction_Integration(t *testing.T) {
 
 func TestGetTransaction_Negative_Integration(t *testing.T) {
 	runParallel(t, func(t *testing.T, testConfig testConfig) {
-		electrum, cancelCtx := newTestConnection(t, testConfig.clientConfig)
+		electrum, cancelCtx := newTestConnection(t, testConfig)
 		defer cancelCtx()
 
 		_, err := electrum.GetTransaction(invalidTxID)
@@ -206,7 +206,7 @@ func TestGetTransaction_Negative_Integration(t *testing.T) {
 
 func TestGetTransactionConfirmations_Integration(t *testing.T) {
 	runParallel(t, func(t *testing.T, testConfig testConfig) {
-		electrum, cancelCtx := newTestConnection(t, testConfig.clientConfig)
+		electrum, cancelCtx := newTestConnection(t, testConfig)
 		defer cancelCtx()
 
 		for txName, tx := range testData.Transactions[testConfig.network] {
@@ -233,7 +233,7 @@ func TestGetTransactionConfirmations_Integration(t *testing.T) {
 
 func TestGetTransactionConfirmations_Negative_Integration(t *testing.T) {
 	runParallel(t, func(t *testing.T, testConfig testConfig) {
-		electrum, cancelCtx := newTestConnection(t, testConfig.clientConfig)
+		electrum, cancelCtx := newTestConnection(t, testConfig)
 		defer cancelCtx()
 
 		_, err := electrum.GetTransactionConfirmations(invalidTxID)
@@ -268,7 +268,7 @@ func TestGetTransactionConfirmations_Negative_Integration(t *testing.T) {
 
 // 	for testName, testConfig := range testConfigs {
 // 		t.Run(testName+"_get", func(t *testing.T) {
-// 			electrum, cancelCtx := newTestConnection(t, testConfig.clientConfig)
+// 			electrum, cancelCtx := newTestConnection(t, testConfig)
 // 			defer cancelCtx()
 
 // 			var wg sync.WaitGroup
@@ -306,7 +306,7 @@ func TestGetLatestBlockHeight_Integration(t *testing.T) {
 
 	for testName, testConfig := range testConfigs {
 		t.Run(testName+"_get", func(t *testing.T) {
-			electrum, cancelCtx := newTestConnection(t, testConfig.clientConfig)
+			electrum, cancelCtx := newTestConnection(t, testConfig)
 			defer cancelCtx()
 
 			result, err := electrum.GetLatestBlockHeight()
@@ -357,7 +357,7 @@ func TestGetLatestBlockHeight_Integration(t *testing.T) {
 
 func TestGetBlockHeader_Integration(t *testing.T) {
 	runParallel(t, func(t *testing.T, testConfig testConfig) {
-		electrum, cancelCtx := newTestConnection(t, testConfig.clientConfig)
+		electrum, cancelCtx := newTestConnection(t, testConfig)
 		defer cancelCtx()
 
 		blockData, ok := testData.Blocks[testConfig.network]
@@ -380,7 +380,7 @@ func TestGetBlockHeader_Negative_Integration(t *testing.T) {
 	blockHeight := uint(math.MaxUint32)
 
 	runParallel(t, func(t *testing.T, testConfig testConfig) {
-		electrum, cancelCtx := newTestConnection(t, testConfig.clientConfig)
+		electrum, cancelCtx := newTestConnection(t, testConfig)
 		defer cancelCtx()
 
 		_, err := electrum.GetBlockHeader(blockHeight)
@@ -396,7 +396,7 @@ func TestGetBlockHeader_Negative_Integration(t *testing.T) {
 
 func TestGetTransactionMerkleProof_Integration(t *testing.T) {
 	runParallel(t, func(t *testing.T, testConfig testConfig) {
-		electrum, cancelCtx := newTestConnection(t, testConfig.clientConfig)
+		electrum, cancelCtx := newTestConnection(t, testConfig)
 		defer cancelCtx()
 
 		txMerkleProofData, ok := testData.TxMerkleProofs[testConfig.network]
@@ -427,7 +427,7 @@ func TestGetTransactionMerkleProof_Negative_Integration(t *testing.T) {
 	blockHeight := uint(123456)
 
 	runParallel(t, func(t *testing.T, testConfig testConfig) {
-		electrum, cancelCtx := newTestConnection(t, testConfig.clientConfig)
+		electrum, cancelCtx := newTestConnection(t, testConfig)
 		defer cancelCtx()
 
 		_, err := electrum.GetTransactionMerkleProof(
@@ -446,7 +446,7 @@ func TestGetTransactionMerkleProof_Negative_Integration(t *testing.T) {
 
 func TestGetTransactionsForPublicKeyHash_Integration(t *testing.T) {
 	runParallel(t, func(t *testing.T, testConfig testConfig) {
-		electrum, cancelCtx := newTestConnection(t, testConfig.clientConfig)
+		electrum, cancelCtx := newTestConnection(t, testConfig)
 		defer cancelCtx()
 
 		txMerkleProofData, ok := testData.TransactionsForPublicKeyHash[testConfig.network]
@@ -475,7 +475,7 @@ func TestGetTransactionsForPublicKeyHash_Integration(t *testing.T) {
 
 func TestGetTxHashesForPublicKeyHash_Integration(t *testing.T) {
 	runParallel(t, func(t *testing.T, testConfig testConfig) {
-		electrum, cancelCtx := newTestConnection(t, testConfig.clientConfig)
+		electrum, cancelCtx := newTestConnection(t, testConfig)
 		defer cancelCtx()
 
 		data, ok := testData.TransactionsForPublicKeyHash[testConfig.network]
@@ -505,7 +505,7 @@ func TestGetTxHashesForPublicKeyHash_Integration(t *testing.T) {
 
 func TestGetUtxosForPublicKeyHash_Integration(t *testing.T) {
 	runParallel(t, func(t *testing.T, testConfig testConfig) {
-		electrum, cancelCtx := newTestConnection(t, testConfig.clientConfig)
+		electrum, cancelCtx := newTestConnection(t, testConfig)
 		defer cancelCtx()
 
 		data, ok := testData.TransactionsForPublicKeyHash[testConfig.network]
@@ -549,7 +549,7 @@ func TestGetUtxosForPublicKeyHash_Integration(t *testing.T) {
 
 func TestEstimateSatPerVByteFee_Integration(t *testing.T) {
 	runParallel(t, func(t *testing.T, testConfig testConfig) {
-		electrum, cancelCtx := newTestConnection(t, testConfig.clientConfig)
+		electrum, cancelCtx := newTestConnection(t, testConfig)
 		defer cancelCtx()
 
 		// A 1-block target often returns no estimate on public testnets; 25 is
@@ -588,7 +588,7 @@ func isFeeEstimateUnavailable(err error) bool {
 
 func TestGetCoinbaseTxHash_Integration(t *testing.T) {
 	runParallel(t, func(t *testing.T, testConfig testConfig) {
-		electrum, cancelCtx := newTestConnection(t, testConfig.clientConfig)
+		electrum, cancelCtx := newTestConnection(t, testConfig)
 		defer cancelCtx()
 
 		blockData, ok := testData.Blocks[testConfig.network]
@@ -628,9 +628,9 @@ func runParallel(t *testing.T, runFunc func(t *testing.T, testConfig testConfig)
 	}
 }
 
-func newTestConnection(t *testing.T, config electrum.Config) (bitcoin.Chain, context.CancelFunc) {
+func newTestConnection(t *testing.T, testConfig testConfig) (bitcoin.Chain, context.CancelFunc) {
 	ctx, cancelCtx := context.WithCancel(context.Background())
-	electrum, err := electrum.Connect(ctx, config)
+	electrum, err := electrum.Connect(ctx, testConfig.network, testConfig.clientConfig)
 	if err != nil {
 		t.Fatal(err)
 	}