lnd lnwallet/btcwallet: use CheckTransactionStandard to validate txns before broadcast

In this commit, we start to use the CheckTransactionStandard function from the mempool package to validate transactions further before broadcast. This'll help catch instances where we make a massive transaction, and therefor can't actually propagate it across the network.

Fixes https://github.com/lightningnetwork/lnd/issues/4760 Fixes https://github.com/lightningnetwork/lnd/issues/6556

May 26 '22 22:05 Roasbeef

Thanks to @ellemouton for making the change to the btcd's mempool package to export the func used here.

May 26 '22 22:05 Roasbeef

This is a very simple approach that just ensures we won't broadcast these transactions. Ideally we should add this check before this step even, in areas like the funding manager so we can just bail out if we know coin selection is weird or w/e. This is covered in the more encompassing PR here: https://github.com/lightningnetwork/lnd/pull/6400

May 26 '22 22:05 Roasbeef

lready handles removing the failed tx from TxStore, what's the effect of adding CheckTransactionStandard from the caller's view?

That's a good question: AFAICT, something is happening where even though we remove the transaction there, it is kept around somewhere and been rebroadcasted again. See this issue as an example: https://github.com/lightningnetwork/lnd/issues/6556.

May 30 '22 18:05 Roasbeef

I think we need to do this instead. From caller's view the behavior of the new btcwallet.PublishTransaction is not changed.

Thought about this a bit more, and it's potentially a bit more error prone: any new PublishTransaction call sites need to be aware of this required check, vs just being able to call PublishTransaction and know you'll get an error if something is up (which is effectively the same behavior as we have w/ and w/o this PR).

Jun 01 '22 00:06 Roasbeef

Here's a crude diff I used to verify the question above:

diff --git a/lnwallet/btcwallet/btcwallet.go b/lnwallet/btcwallet/btcwallet.go
index ff0161d36..455af3126 100644
--- a/lnwallet/btcwallet/btcwallet.go
+++ b/lnwallet/btcwallet/btcwallet.go
@@ -14,7 +14,6 @@ import (
 	"github.com/btcsuite/btcd/btcutil/hdkeychain"
 	"github.com/btcsuite/btcd/chaincfg"
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
-	"github.com/btcsuite/btcd/mempool"
 	"github.com/btcsuite/btcd/txscript"
 	"github.com/btcsuite/btcd/wire"
 	"github.com/btcsuite/btcwallet/chain"
@@ -1059,7 +1058,7 @@ func (b *BtcWallet) PublishTransaction(tx *wire.MsgTx, label string) error {
 	// Note that we just use the time of the last block header here vs
 	// calculating the full median time past (BIP 113). We do this as a
 	// block's timestamp MUST be greater than the MTP of the prior block.
-	bestHash, bestHeight, err := b.chain.GetBestBlock()
+	/*bestHash, bestHeight, err := b.chain.GetBestBlock()
 	if err != nil {
 		return err
 	}
@@ -1074,7 +1073,7 @@ func (b *BtcWallet) PublishTransaction(tx *wire.MsgTx, label string) error {
 	)
 	if err != nil {
 		return fmt.Errorf("tx failed policy checks: %w", err)
-	}
+	}*/
 
 	// TODO(roasbeef): can also use testmempoolaccept here
 
diff --git a/lnwallet/test/test_interface.go b/lnwallet/test/test_interface.go
index 158f0dd99..19612f730 100644
--- a/lnwallet/test/test_interface.go
+++ b/lnwallet/test/test_interface.go
@@ -2055,9 +2055,14 @@ func testPublishTransaction(r *rpctest.Harness,
 		// TODO(roasbeef): we can't use Unwrap() here as TxRuleError
 		// doesn't define it
 		err := alice.PublishTransaction(testTx, labels.External)
-		require.True(
-			t, strings.Contains(err.Error(), "max allowed weight"),
-		)
+		fmt.Println(err)
+		//require.True(
+		//	t, strings.Contains(err.Error(), "max allowed weight"),
+		//	)
+		txHash := testTx.TxHash()
+		tx, err := alice.FetchTx(txHash)
+		require.NoError(t, err)
+		fmt.Println("tx value: ", tx)
 	})
 }

In this case, after I look up the transaction it isn't present, which confirms the suspicion that we are properly removing the transaction as expected if policy is violated. So I don't think we really need this branch as is, other than the funding manger, which is already covered elsewhere.

Jun 01 '22 17:06 Roasbeef

In this case, after I look up the transaction it isn't present, which confirms the suspicion that we are properly removing the transaction as expected if policy is violated.

Cool so it is working as intended, thanks for confirming!

My thought is more about the overall architecture, like who is responsible for what. The layering is quite clear here. lnd defines all types of transactions, btcwallet treats them as raw tx and handles the validation and broadcast. We could abstract the validation logic into the CheckTransactionStandard or using testmempoolaccept, but is it lnd's job to perform validation at the policy level? Maybe not since PublishTransaction is already handling that part.

So my thought on this is to refactor PublishTransaction to return a list of explicitly defined errors, and we handle the errors one by one in lnd. This way we can keep the abstractions clear.