From e4482421c9dfd97aa2c27d222da21993f22fba68 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 5 Jan 2024 04:18:50 +0800 Subject: [PATCH 01/10] gomod+chainreg: update btcwallet to use `TestMempoolAccept` --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 0054d179e..1b0a9de89 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 2e818f776..ea87b0c02 100644 --- a/go.sum +++ b/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= From 2686ca324a15f964b5f86e14c0da789036438074 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 5 Jan 2024 04:19:19 +0800 Subject: [PATCH 02/10] lnwallet: check mempool acceptance before publishing This commit adds a mempool acceptance check before broadcasting a given transaction. To maintain the current behavior from `BtcWallet.PublishTransaction`, the two errors, `ErrInMempool` and `ErrAlreadyConfirmed` returned from `TestMempoolAccept` are ignored. --- lnutils/errors.go | 21 +++++++++ lnwallet/btcwallet/btcwallet.go | 81 +++++++++++++++++++++++++++++---- 2 files changed, 94 insertions(+), 8 deletions(-) create mode 100644 lnutils/errors.go diff --git a/lnutils/errors.go b/lnutils/errors.go new file mode 100644 index 000000000..f6a9c57c9 --- /dev/null +++ b/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) +} diff --git a/lnwallet/btcwallet/btcwallet.go b/lnwallet/btcwallet/btcwallet.go index a5fd7534b..1e55a0502 100644 --- a/lnwallet/btcwallet/btcwallet.go +++ b/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,96 @@ 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()) + //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. + case lnutils.ErrorAs[*base.ErrInMempool](err), + lnutils.ErrorAs[*base.ErrAlreadyConfirmed](err): + + return nil + default: 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)) + + // If the check failed, there's no need to publish it. We'll + // handle the error and return. + if !result.Allowed { + 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), + ) + + return handleErr(err) + } + + // Once mempool check passed, we can publish the transaction. + err = b.wallet.PublishTransaction(tx, label) + return handleErr(err) } // LabelTransaction adds a label to a transaction. If the tx already From 1435ba5636adb701828fdc94544b675e95d13825 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 5 Jan 2024 17:02:12 +0800 Subject: [PATCH 03/10] docs: add release note for `testmempoolaccept` --- docs/release-notes/release-notes-0.18.0.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/release-notes/release-notes-0.18.0.md b/docs/release-notes/release-notes-0.18.0.md index 785d49eb6..13e52baa7 100644 --- a/docs/release-notes/release-notes-0.18.0.md +++ b/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) From fb1c6ea6a78ec577d390b0cc51f359fcc6c60f58 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Sun, 7 Jan 2024 04:28:54 +0800 Subject: [PATCH 04/10] btcwallet: proceed to call `PublishTransaction` on mempool errors Previously, `PublishTransaction` in `btcwallet` will first mark the tx label in db first before broadcasting it to the network. Even though the broadcast may fail with the error already in mempool or already confirmed, this tx label updating is still performed. To maintain the old behavior, we now catch the errors from `TestMempoolAccept`, and make sure to call the `PublishTransaction` to mark the tx label even there are errors from mempool acceptance check. --- lnwallet/btcwallet/btcwallet.go | 72 ++++++++++++++++++--------------- 1 file changed, 39 insertions(+), 33 deletions(-) diff --git a/lnwallet/btcwallet/btcwallet.go b/lnwallet/btcwallet/btcwallet.go index 1e55a0502..93f909072 100644 --- a/lnwallet/btcwallet/btcwallet.go +++ b/lnwallet/btcwallet/btcwallet.go @@ -1224,33 +1224,16 @@ func (b *BtcWallet) PublishTransaction(tx *wire.MsgTx, label string) error { case lnutils.ErrorAs[*base.ErrMempoolFee](err): return fmt.Errorf("%w: %v", lnwallet.ErrMempoolFee, err.Error()) - - //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. - case lnutils.ErrorAs[*base.ErrInMempool](err), - lnutils.ErrorAs[*base.ErrAlreadyConfirmed](err): - - return nil - - default: - return err } + + return err } // 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) } @@ -1273,23 +1256,46 @@ func (b *BtcWallet) PublishTransaction(tx *wire.MsgTx, label string) error { result := results[0] log.Debugf("TestMempoolAccept result: %s", spew.Sdump(result)) - // If the check failed, there's no need to publish it. We'll - // handle the error and return. - if !result.Allowed { - 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), - ) + // 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) } - // Once mempool check passed, we can publish the transaction. - err = b.wallet.PublishTransaction(tx, label) return handleErr(err) } From a6a8415a266b7dba2933bd2145ebb213098407d9 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Sat, 6 Jan 2024 05:45:33 +0800 Subject: [PATCH 05/10] contractcourt: make sure `ChainArbitrator` is started properly We should not fail to start the node if a republish attempt failed for a channel's closing tx. Instead, we log an error to continue the startup and let other channels continue their operations. --- contractcourt/chain_arbitrator.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/contractcourt/chain_arbitrator.go b/contractcourt/chain_arbitrator.go index acae5b678..acf3512af 100644 --- a/contractcourt/chain_arbitrator.go +++ b/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, From a616fa3fb713755ec600059bb7deaf4b45b526fb Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 26 May 2022 15:49:13 -0700 Subject: [PATCH 06/10] lnwallet/test: convert the tx publish tests to sub-tests --- lnwallet/test/test_interface.go | 306 +++++++++++++++----------------- 1 file changed, 145 insertions(+), 161 deletions(-) diff --git a/lnwallet/test/test_interface.go b/lnwallet/test/test_interface.go index b33349119..ad01a9ebb 100644 --- a/lnwallet/test/test_interface.go +++ b/lnwallet/test/test_interface.go @@ -1721,187 +1721,171 @@ func testPublishTransaction(r *rpctest.Harness, keyDesc, err := alice.DeriveNextKey(keychain.KeyFamilyMultiSig) require.NoError(t, err, "unable to obtain public key") - // We will first check that publishing a transaction already in the - // mempool does NOT return an error. Create the tx. - tx1 := newTx(t, r, keyDesc.PubKey, alice, false) + t.Run("no_error_on_duplicate_tx", func(t *testing.T) { + // We will first check that publishing a transaction already in + // the mempool does NOT return an error. Create the tx. + tx1 := newTx(t, r, keyDesc.PubKey, alice, false) - // Publish the transaction. - err = alice.PublishTransaction(tx1, labels.External) - require.NoError(t, err, "unable to publish") + // Publish the transaction. + err = alice.PublishTransaction(tx1, labels.External) + require.NoError(t, err) - txid1 := tx1.TxHash() - err = waitForMempoolTx(r, &txid1) - require.NoError(t, err, "tx not relayed to miner") + txid1 := tx1.TxHash() + err = waitForMempoolTx(r, &txid1) + require.NoError(t, err, "tx not relayed to miner") - // Publish the exact same transaction again. This should not return an - // error, even though the transaction is already in the mempool. - err = alice.PublishTransaction(tx1, labels.External) - require.NoError(t, err, "unable to publish") - - // Mine the transaction. - if _, err := r.Client.Generate(1); err != nil { - t.Fatalf("unable to generate block: %v", err) - } - - // We'll now test that we don't get an error if we try to publish a - // transaction that is already mined. - // - // Create a new transaction. We must do this to properly test the - // reject messages from our peers. They might only send us a reject - // message for a given tx once, so we create a new to make sure it is - // not just immediately rejected. - tx2 := newTx(t, r, keyDesc.PubKey, alice, false) - - // Publish this tx. - err = alice.PublishTransaction(tx2, labels.External) - require.NoError(t, err, "unable to publish") - - // Mine the transaction. - if err := mineAndAssert(r, tx2); err != nil { - t.Fatalf("unable to mine tx: %v", err) - } - - // Publish the transaction again. It is already mined, and we don't - // expect this to return an error. - err = alice.PublishTransaction(tx2, labels.External) - require.NoError(t, err, "unable to publish") - - // We'll do the next mempool check on both RBF and non-RBF enabled - // transactions. - var ( - txFee = btcutil.Amount(0.005 * btcutil.SatoshiPerBitcoin) - tx3, tx3Spend *wire.MsgTx - ) - - for _, rbf := range []bool{false, true} { - // Now we'll try to double spend an output with a different - // transaction. Create a new tx and publish it. This is the - // output we'll try to double spend. - tx3 = newTx(t, r, keyDesc.PubKey, alice, false) - err := alice.PublishTransaction(tx3, labels.External) - if err != nil { - t.Fatalf("unable to publish: %v", err) - } + // Publish the exact same transaction again. This should not + // return an error, even though the transaction is already in + // the mempool. + err = alice.PublishTransaction(tx1, labels.External) + require.NoError(t, err) // Mine the transaction. - if err := mineAndAssert(r, tx3); err != nil { - t.Fatalf("unable to mine tx: %v", err) - } + _, err := r.Client.Generate(1) + require.NoError(t, err) + }) - // Now we create a transaction that spends the output from the - // tx just mined. - tx4, err := txFromOutput( - tx3, alice.Cfg.Signer, keyDesc.PubKey, - keyDesc.PubKey, txFee, rbf, + t.Run("no_error_on_minged_tx", func(t *testing.T) { + // We'll now test that we don't get an error if we try to + // publish a transaction that is already mined. + // + // Create a new transaction. We must do this to properly test + // the reject messages from our peers. They might only send us + // a reject message for a given tx once, so we create a new to + // make sure it is not just immediately rejected. + tx2 := newTx(t, r, keyDesc.PubKey, alice, false) + + // Publish this tx. + err = alice.PublishTransaction(tx2, labels.External) + require.NoError(t, err) + + // Mine the transaction. + err := mineAndAssert(r, tx2) + require.NoError(t, err) + + // Publish the transaction again. It is already mined, and we + // don't expect this to return an error. + err = alice.PublishTransaction(tx2, labels.External) + require.NoError(t, err) + }) + + // We'll do the next mempool check on both RBF and non-RBF + // enabled transactions. + var ( + txFee = btcutil.Amount( + 0.005 * btcutil.SatoshiPerBitcoin, ) - if err != nil { - t.Fatal(err) - } + tx3, tx3Spend *wire.MsgTx + ) + t.Run("rbf_tests", func(t *testing.T) { + for _, rbf := range []bool{false, true} { + // Now we'll try to double spend an output with a + // different transaction. Create a new tx and publish + // it. This is the output we'll try to double spend. + tx3 = newTx(t, r, keyDesc.PubKey, alice, false) + err := alice.PublishTransaction(tx3, labels.External) + require.NoError(t, err) - // This should be accepted into the mempool. - err = alice.PublishTransaction(tx4, labels.External) - if err != nil { - t.Fatalf("unable to publish: %v", err) - } + // Mine the transaction. + err = mineAndAssert(r, tx3) + require.NoError(t, err) - // Keep track of the last successfully published tx to spend - // tx3. - tx3Spend = tx4 + // Now we create a transaction that spends the output + // from the tx just mined. + tx4, err := txFromOutput( + tx3, alice.Cfg.Signer, keyDesc.PubKey, + keyDesc.PubKey, txFee, rbf, + ) + require.NoError(t, err) - txid4 := tx4.TxHash() - err = waitForMempoolTx(r, &txid4) - if err != nil { - t.Fatalf("tx not relayed to miner: %v", err) - } + // This should be accepted into the mempool. + err = alice.PublishTransaction(tx4, labels.External) + require.NoError(t, err) - // Create a new key we'll pay to, to ensure we create a unique - // transaction. - keyDesc2, err := alice.DeriveNextKey( - keychain.KeyFamilyMultiSig, - ) - if err != nil { - t.Fatalf("unable to obtain public key: %v", err) - } + // Keep track of the last successfully published tx to + // spend tx3. + tx3Spend = tx4 - // Create a new transaction that spends the output from tx3, - // and that pays to a different address. We expect this to be - // rejected because it is a double spend. - tx5, err := txFromOutput( - tx3, alice.Cfg.Signer, keyDesc.PubKey, - keyDesc2.PubKey, txFee, rbf, - ) - if err != nil { - t.Fatal(err) - } + txid4 := tx4.TxHash() + err = waitForMempoolTx(r, &txid4) + require.NoError(t, err, "tx not relayed to miner") - err = alice.PublishTransaction(tx5, labels.External) - if err != lnwallet.ErrDoubleSpend { - t.Fatalf("expected ErrDoubleSpend, got: %v", err) - } + // Create a new key we'll pay to, to ensure we create a + // unique transaction. + keyDesc2, err := alice.DeriveNextKey( + keychain.KeyFamilyMultiSig, + ) + require.NoError(t, err, "unable to obtain public key") - // Create another transaction that spends the same output, but - // has a higher fee. We expect also this tx to be rejected for - // non-RBF enabled transactions, while it should succeed - // otherwise. - pubKey3, err := alice.DeriveNextKey(keychain.KeyFamilyMultiSig) - if err != nil { - t.Fatalf("unable to obtain public key: %v", err) - } - tx6, err := txFromOutput( - tx3, alice.Cfg.Signer, keyDesc.PubKey, - pubKey3.PubKey, 2*txFee, rbf, - ) - if err != nil { - t.Fatal(err) - } + // Create a new transaction that spends the output from + // tx3, and that pays to a different address. We expect + // this to be rejected because it is a double spend. + tx5, err := txFromOutput( + tx3, alice.Cfg.Signer, keyDesc.PubKey, + keyDesc2.PubKey, txFee, rbf, + ) + require.NoError(t, err) - // Expect rejection in non-RBF case. - expErr := lnwallet.ErrDoubleSpend - if rbf { - // Expect success in rbf case. - expErr = nil - tx3Spend = tx6 - } - err = alice.PublishTransaction(tx6, labels.External) - if err != expErr { - t.Fatalf("expected ErrDoubleSpend, got: %v", err) - } + err = alice.PublishTransaction(tx5, labels.External) + require.ErrorIs(t, err, lnwallet.ErrDoubleSpend) - // Mine the tx spending tx3. - if err := mineAndAssert(r, tx3Spend); err != nil { - t.Fatalf("unable to mine tx: %v", err) - } - } + // Create another transaction that spends the same + // output, but has a higher fee. We expect also this tx + // to be rejected for non-RBF enabled transactions, + // while it should succeed otherwise. + pubKey3, err := alice.DeriveNextKey( + keychain.KeyFamilyMultiSig, + ) + require.NoError(t, err, "unable to obtain public key") - // At last we try to spend an output already spent by a confirmed - // transaction. - // TODO(halseth): we currently skip this test for neutrino, as the - // backing btcd node will consider the tx being an orphan, and will - // accept it. Should look into if this is the behavior also for - // bitcoind, and update test accordingly. - if alice.BackEnd() != "neutrino" { - // Create another tx spending tx3. - pubKey4, err := alice.DeriveNextKey( - keychain.KeyFamilyMultiSig, - ) - if err != nil { - t.Fatalf("unable to obtain public key: %v", err) - } - tx7, err := txFromOutput( - tx3, alice.Cfg.Signer, keyDesc.PubKey, - pubKey4.PubKey, txFee, false, - ) + tx6, err := txFromOutput( + tx3, alice.Cfg.Signer, keyDesc.PubKey, + pubKey3.PubKey, 2*txFee, rbf, + ) + require.NoError(t, err) - if err != nil { - t.Fatal(err) - } + // Expect rejection in non-RBF case. + expErr := lnwallet.ErrDoubleSpend + if rbf { + // Expect success in rbf case. + expErr = nil + tx3Spend = tx6 + } + err = alice.PublishTransaction(tx6, labels.External) + require.ErrorIs(t, err, expErr) - // Expect rejection. - err = alice.PublishTransaction(tx7, labels.External) - if err != lnwallet.ErrDoubleSpend { - t.Fatalf("expected ErrDoubleSpend, got: %v", err) + // Mine the tx spending tx3. + err = mineAndAssert(r, tx3Spend) + require.NoError(t, err) } - } + }) + + t.Run("tx_double_spend", func(t *testing.T) { + // At last we try to spend an output already spent by a + // confirmed transaction. + // + // TODO(halseth): we currently skip this test for neutrino, as + // the backing btcd node will consider the tx being an orphan, + // and will accept it. Should look into if this is the behavior + // also for bitcoind, and update test accordingly. + if alice.BackEnd() != "neutrino" { + // Create another tx spending tx3. + pubKey4, err := alice.DeriveNextKey( + keychain.KeyFamilyMultiSig, + ) + require.NoError(t, err, "unable to obtain public key") + + tx7, err := txFromOutput( + tx3, alice.Cfg.Signer, keyDesc.PubKey, + pubKey4.PubKey, txFee, false, + ) + require.NoError(t, err) + + // Expect rejection. + err = alice.PublishTransaction(tx7, labels.External) + require.ErrorIs(t, err, lnwallet.ErrDoubleSpend) + } + }) } func testSignOutputUsingTweaks(r *rpctest.Harness, From 11bafe84ff624e0d7a2d56ef00be89567a7d4cbd Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 26 May 2022 15:49:36 -0700 Subject: [PATCH 07/10] lnwallet/test: add new tx publish sub-test for policy weight limits --- lnwallet/test/test_interface.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/lnwallet/test/test_interface.go b/lnwallet/test/test_interface.go index ad01a9ebb..bc2dbcf1b 100644 --- a/lnwallet/test/test_interface.go +++ b/lnwallet/test/test_interface.go @@ -15,6 +15,7 @@ import ( "testing" "time" + "github.com/btcsuite/btcd/blockchain" "github.com/btcsuite/btcd/btcec/v2" "github.com/btcsuite/btcd/btcjson" "github.com/btcsuite/btcd/btcutil" @@ -1886,6 +1887,31 @@ func testPublishTransaction(r *rpctest.Harness, require.ErrorIs(t, err, lnwallet.ErrDoubleSpend) } }) + + t.Run("test_tx_size_limit", func(t *testing.T) { + // In this test, we'll try to create a massive transaction that + // can't be mined as dictacted by widely deployed transaction + // policy. + // + // To do this, we'll take out of the prior transactions, and + // add a bunch of outputs, putting it over the max weight + // limit. + testTx := tx3.Copy() + for i := 0; i < blockchain.MaxOutputsPerBlock; i++ { + testTx.AddTxOut(&wire.TxOut{ + Value: tx3.TxOut[0].Value, + PkScript: tx3.TxOut[0].PkScript, + }) + } + + // Now broadcast the transaction, we should get an error that + // the weight is too large. + // + // TODO(roasbeef): we can't use Unwrap() here as TxRuleError + // doesn't define it + err := alice.PublishTransaction(testTx, labels.External) + require.Contains(t, err.Error(), "bad-txns-oversize") + }) } func testSignOutputUsingTweaks(r *rpctest.Harness, From 783e9140275c2bae355cb0696248c2f6dc743f5a Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 16 Jan 2024 16:31:18 +0800 Subject: [PATCH 08/10] lnwallet/test: stop creating burning tx in tests bitcoind v25.0 updated the `sendrawtransaction` RPC to have an optional argument `maxburnamount` with a default value of 0. This means our existing test that uses burning output cannot be published, hence, we remove the usage of it in our tests and replace it with a normal tx. --- lnwallet/test/test_interface.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/lnwallet/test/test_interface.go b/lnwallet/test/test_interface.go index bc2dbcf1b..75926872a 100644 --- a/lnwallet/test/test_interface.go +++ b/lnwallet/test/test_interface.go @@ -1472,15 +1472,20 @@ func testTransactionSubscriptions(miner *rpctest.Harness, // We'll also ensure that the client is able to send our new // notifications when we _create_ transactions ourselves that spend our // own outputs. - b := txscript.NewScriptBuilder() - b.AddOp(txscript.OP_RETURN) - outputScript, err := b.Script() - require.NoError(t, err, "unable to make output script") + addr, err := alice.NewAddress( + lnwallet.WitnessPubKey, false, + lnwallet.DefaultAccountName, + ) + require.NoError(t, err) + + outputScript, err := txscript.PayToAddrScript(addr) + require.NoError(t, err) + burnOutput := wire.NewTxOut(outputAmt, outputScript) tx, err := alice.SendOutputs( []*wire.TxOut{burnOutput}, 2500, 1, labels.External, ) - require.NoError(t, err, "unable to create burn tx") + require.NoError(t, err, "unable to create tx") txid := tx.TxHash() err = waitForMempoolTx(miner, &txid) require.NoError(t, err, "tx not relayed to miner") From d2ab35629a9f88f602982fce726016dd24fde419 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 25 Jan 2024 23:33:32 +0800 Subject: [PATCH 09/10] lnwallet/test: fix ZMQ format --- lnwallet/test/test_interface.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lnwallet/test/test_interface.go b/lnwallet/test/test_interface.go index 75926872a..0770e8822 100644 --- a/lnwallet/test/test_interface.go +++ b/lnwallet/test/test_interface.go @@ -3259,9 +3259,15 @@ func runTests(t *testing.T, walletDriver *lnwallet.WalletDriver, case "bitcoind": // Start a bitcoind instance. tempBitcoindDir := t.TempDir() - zmqBlockHost := "ipc:///" + tempBitcoindDir + "/blocks.socket" - zmqTxHost := "ipc:///" + tempBitcoindDir + "/tx.socket" + rpcPort := getFreePort() + zmqBlockPort := getFreePort() + zmqTxPort := getFreePort() + zmqBlockHost := fmt.Sprintf("tcp://127.0.0.1:%d", + zmqBlockPort) + zmqTxHost := fmt.Sprintf("tcp://127.0.0.1:%d", + zmqTxPort) + bitcoind := exec.Command( "bitcoind", "-datadir="+tempBitcoindDir, From a6148887972ef5945311479016c2087369cbade0 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 26 Jan 2024 02:54:36 +0800 Subject: [PATCH 10/10] lnwallet/test: fix `testPublishTransaction` for neutrino --- lnwallet/test/test_interface.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lnwallet/test/test_interface.go b/lnwallet/test/test_interface.go index 0770e8822..aa3a6625e 100644 --- a/lnwallet/test/test_interface.go +++ b/lnwallet/test/test_interface.go @@ -1915,7 +1915,17 @@ 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.Contains(t, err.Error(), "bad-txns-oversize") + + errStr := "bad-txns-oversize" + + // For neutrino backend, the error string in not mapped. + // + // TODO(yy): unify error matching in neutrino too. + if alice.BackEnd() == "neutrino" { + errStr = "serialized transaction is too big" + } + + require.Contains(t, err.Error(), errStr) }) }