lnwallet/btcwallet: use CheckTransactionStandard to validate txns before broadcast

[Roasbeef]

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 #4760
Fixes #6556

[Roasbeef]

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

[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: #6400

[yyforyongyu]

Thanks for picking this up! My main question is since the wallet.PublishTransaction already handles removing the failed tx from TxStore, what's the effect of adding CheckTransactionStandard from the caller's view?
https://github.com/btcsuite/btcwallet/blob/af5562928b707e0b56c1e44052ece3205bb66552/wallet/wallet.go#L3743-L3754

By adding CheckTransactionStandard here, it does save us from the process of first writing the tx to TxStore and later removing it. re #4760, it seems the dangling tx would still be there?

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.

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

[yyforyongyu]

nit: could remove this title as well.

[yyforyongyu]

I think we need to do bestHeight + 1 here?

[yyforyongyu]

typo: minged -> mined

[yyforyongyu]

👍

[yyforyongyu]

nit: could make it a public constant and put it in the header?

[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: #6556.

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

[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.

[yyforyongyu]

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.

[Crypt-iQ]

I don't want this one to get lost since it's security-related so reopening it

[lightninglabs-deploy]

@Roasbeef, remember to re-request review from reviewers when ready

[Roasbeef]

Superseded by #8345