lnwallet: perform mempool acceptance check before publishing

contractcourt/chain_arbitrator.go

@@ -548,9 +548,10 @@ func (c *ChainArbitrator) Start() error {
 		c.activeChannels[chanPoint] = channelArb
 
 		// Republish any closing transactions for this channel.
-		err = c.publishClosingTxs(channel)
+		err = c.republishClosingTxs(channel)
 		if err != nil {
-			return err
+			log.Errorf("Failed to republish closing txs for "+
+				"channel %v", chanPoint)
 		}
 	}
 
@@ -825,10 +826,10 @@ func (c *ChainArbitrator) dispatchBlocks(
 	}
 }
 
-// publishClosingTxs will load any stored cooperative or unilater closing
+// republishClosingTxs will load any stored cooperative or unilateral closing
 // transactions and republish them. This helps ensure propagation of the
 // transactions in the event that prior publications failed.
-func (c *ChainArbitrator) publishClosingTxs(
+func (c *ChainArbitrator) republishClosingTxs(
 	channel *channeldb.OpenChannel) error {
 
 	// If the channel has had its unilateral close broadcasted already,

docs/release-notes/release-notes-0.18.0.md

@@ -115,6 +115,12 @@
   without the user needing to publicly give away a lot of privacy-sensitive
   data.
 
+* When publishing transactions in `lnd`, all the transactions will now go
+  through [mempool acceptance
+  check](https://github.com/lightningnetwork/lnd/pull/8345) before being
+  broadcast. This means when a transaction has failed the `testmempoolaccept`
+  check by bitcoind or btcd, the broadcast won't be attempted.
+
 ## RPC Additions
 
 * [Deprecated](https://github.com/lightningnetwork/lnd/pull/7175)

go.mod

@@ -10,7 +10,7 @@ require (
 	github.com/btcsuite/btcd/btcutil/psbt v1.1.8
 	github.com/btcsuite/btcd/chaincfg/chainhash v1.1.0
 	github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f
-	github.com/btcsuite/btcwallet v0.16.10-0.20231129183218-5df09dd43358
+	github.com/btcsuite/btcwallet v0.16.10-0.20240122184004-f1648e92097f
 	github.com/btcsuite/btcwallet/wallet/txauthor v1.3.2
 	github.com/btcsuite/btcwallet/wallet/txrules v1.2.0
 	github.com/btcsuite/btcwallet/walletdb v1.4.0

go.sum

@@ -95,8 +95,8 @@ github.com/btcsuite/btcd/chaincfg/chainhash v1.1.0/go.mod h1:7SFka0XMvUgj3hfZtyd
 github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f h1:bAs4lUbRJpnnkd9VhRV3jjAVU7DJVjMaK+IsvSeZvFo=
 github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f/go.mod h1:TdznJufoqS23FtqVCzL0ZqgP5MqXbb4fg/WgDys70nA=
 github.com/btcsuite/btcutil v0.0.0-20190425235716-9e5f4b9a998d/go.mod h1:+5NJ2+qvTyV9exUAL/rxXi3DcLg2Ts+ymUAY5y4NvMg=
-github.com/btcsuite/btcwallet v0.16.10-0.20231129183218-5df09dd43358 h1:lZUSo6TISHUJQxpn/AniW5gqaN1iRNS87SDWvV3AHfg=
-github.com/btcsuite/btcwallet v0.16.10-0.20231129183218-5df09dd43358/go.mod h1:WSKhOJWUmUOHKCKEzdt+jWAHFAE/t4RqVbCwL2pEdiU=
+github.com/btcsuite/btcwallet v0.16.10-0.20240122184004-f1648e92097f h1:wtA3/5W+SAIQaWEuvnjMJqWEPjf2mWupXBB6KHbFKYA=
+github.com/btcsuite/btcwallet v0.16.10-0.20240122184004-f1648e92097f/go.mod h1:LzcW/LYkQLgDufv6Ouw4cOIW0YsY+A60MTtc61/OZTU=
 github.com/btcsuite/btcwallet/wallet/txauthor v1.3.2 h1:etuLgGEojecsDOYTII8rYiGHjGyV5xTqsXi+ZQ715UU=
 github.com/btcsuite/btcwallet/wallet/txauthor v1.3.2/go.mod h1:Zpk/LOb2sKqwP2lmHjaZT9AdaKsHPSbNLm2Uql5IQ/0=
 github.com/btcsuite/btcwallet/wallet/txrules v1.2.0 h1:BtEN5Empw62/RVnZ0VcJaVtVlBijnLlJY+dwjAye2Bg=

lnutils/errors.go

@@ -0,0 +1,21 @@
+package lnutils
+
+import "errors"
+
+// ErrorAs behaves the same as `errors.As` except there's no need to declare
+// the target error as a variable first.
+// Instead of writing:
+//
+//	var targetErr *TargetErr
+//	errors.As(err, &targetErr)
+//
+// We can write:
+//
+//	lnutils.ErrorAs[*TargetErr](err)
+//
+// To save us from declaring the target error variable.
+func ErrorAs[Target error](err error) bool {
+	var targetErr Target
+
+	return errors.As(err, &targetErr)
+}

lnwallet/btcwallet/btcwallet.go

@@ -23,10 +23,12 @@ import (
 	"github.com/btcsuite/btcwallet/wallet/txrules"
 	"github.com/btcsuite/btcwallet/walletdb"
 	"github.com/btcsuite/btcwallet/wtxmgr"
+	"github.com/davecgh/go-spew/spew"
 	"github.com/lightningnetwork/lnd/blockcache"
 	"github.com/lightningnetwork/lnd/input"
 	"github.com/lightningnetwork/lnd/keychain"
 	"github.com/lightningnetwork/lnd/kvdb"
+	"github.com/lightningnetwork/lnd/lnutils"
 	"github.com/lightningnetwork/lnd/lnwallet"
 	"github.com/lightningnetwork/lnd/lnwallet/chainfee"
 )
@@ -1199,33 +1201,102 @@ func (b *BtcWallet) ListUnspentWitness(minConfs, maxConfs int32,
 // already published to the network (either in the mempool or chain) no error
 // will be returned.
 func (b *BtcWallet) PublishTransaction(tx *wire.MsgTx, label string) error {
-	if err := b.wallet.PublishTransaction(tx, label); err != nil {
+	// handleErr is a helper closure that handles the error from
+	// PublishTransaction and TestMempoolAccept.
+	handleErr := func(err error) error {
 		// If we failed to publish the transaction, check whether we
 		// got an error of known type.
-		switch err.(type) {
+		switch {
 		// If the wallet reports a double spend, convert it to our
 		// internal ErrDoubleSpend and return.
-		case *base.ErrDoubleSpend:
+		case lnutils.ErrorAs[*base.ErrDoubleSpend](err):
 			return lnwallet.ErrDoubleSpend
 
 		// If the wallet reports a replacement error, return
 		// ErrDoubleSpend, as we currently are never attempting to
 		// replace transactions.
-		case *base.ErrReplacement:
+		case lnutils.ErrorAs[*base.ErrReplacement](err):
 			return lnwallet.ErrDoubleSpend
 
-		// If the wallet reports that fee requirements for accepting the
-		// tx into mempool are not met, convert it to our internal
+		// If the wallet reports that fee requirements for accepting
+		// the tx into mempool are not met, convert it to our internal
 		// ErrMempoolFee and return.
-		case *base.ErrMempoolFee:
+		case lnutils.ErrorAs[*base.ErrMempoolFee](err):
 			return fmt.Errorf("%w: %v", lnwallet.ErrMempoolFee,
 				err.Error())
-
-		default:
-			return err
 		}
+
+		return err
 	}
-	return nil
+
+	// For neutrino backend there's no mempool, so we return early by
+	// publishing the transaction.
+	if b.chain.BackEnd() == "neutrino" {
+		err := b.wallet.PublishTransaction(tx, label)
+
+		return handleErr(err)
+	}
+
+	// For non-neutrino nodes, we will first check whether the transaction
+	// can be accepted by the mempool.
+	// Use a max feerate of 0 means the default value will be used when
+	// testing mempool acceptance. The default max feerate is 0.10 BTC/kvb,
+	// or 10,000 sat/vb.
+	results, err := b.chain.TestMempoolAccept([]*wire.MsgTx{tx}, 0)
+	if err != nil {
+		return err
+	}
+
+	// Sanity check that the expected single result is returned.
+	if len(results) != 1 {
+		return fmt.Errorf("expected 1 result from TestMempoolAccept, "+
+			"instead got %v", len(results))
+	}
+
+	result := results[0]
+	log.Debugf("TestMempoolAccept result: %s", spew.Sdump(result))
+
+	// Once mempool check passed, we can publish the transaction.
+	if result.Allowed {
+		err = b.wallet.PublishTransaction(tx, label)
+
+		return handleErr(err)
+	}
+
+	// If the check failed, there's no need to publish it. We'll handle the
+	// error and return.
+	log.Warnf("Transaction %v not accepted by mempool: %v",
+		tx.TxHash(), result.RejectReason)
+
+	// We need to use the string to create an error type and map it to a
+	// btcwallet error.
+	err = base.MapBroadcastBackendError(errors.New(result.RejectReason))
+
+	//nolint:lll
+	// These two errors are ignored inside `PublishTransaction`:
+	// https://github.com/btcsuite/btcwallet/blob/master/wallet/wallet.go#L3763
+	// To keep our current behavior, we need to ignore the same errors
+	// returned from TestMempoolAccept.
+	//
+	// TODO(yy): since `LightningWallet.PublishTransaction` always publish
+	// the same tx twice, we'd always get ErrInMempool. We should instead
+	// create a new rebroadcaster that monitors the mempool, and only
+	// rebroadcast when the tx is evicted. This way we don't need to
+	// broadcast twice, and can instead return these errors here.
+	switch {
+	// NOTE: In addition to ignoring these errors, we need to call
+	// `PublishTransaction` again because we need to mark the label in the
+	// wallet. We can remove this exception once we have the above TODO
+	// fixed.
+	case lnutils.ErrorAs[*base.ErrInMempool](err),
+		lnutils.ErrorAs[*base.ErrAlreadyConfirmed](err):
+
+		err := b.wallet.PublishTransaction(tx, label)
+
+		return handleErr(err)
+	}
+
+	return handleErr(err)
 }
 
 // LabelTransaction adds a label to a transaction. If the tx already