From 837c7f761c892bec81859059786b3b7cbb1926bd Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 15 Mar 2024 05:19:43 +0800 Subject: [PATCH 1/4] lnwallet+lnd: make use of the new errors from `btcd/rpcclient` This commit updates `btcd` and `btcwallet` packages and make use of the new RPC error mappings. --- config_builder.go | 15 +++---- go.mod | 8 ++-- go.sum | 16 +++---- lnwallet/btcwallet/btcwallet.go | 79 ++++++++++++++++----------------- 4 files changed, 57 insertions(+), 61 deletions(-) diff --git a/config_builder.go b/config_builder.go index 5195777e1..703c2fdb1 100644 --- a/config_builder.go +++ b/config_builder.go @@ -18,6 +18,7 @@ import ( "github.com/btcsuite/btcd/chaincfg" "github.com/btcsuite/btcd/chaincfg/chainhash" + "github.com/btcsuite/btcd/rpcclient" "github.com/btcsuite/btcd/wire" "github.com/btcsuite/btclog" "github.com/btcsuite/btcwallet/waddrmgr" @@ -1442,18 +1443,16 @@ func parseHeaderStateAssertion(state string) (*headerfs.FilterHeader, error) { // the neutrino BroadcastError which allows the Rebroadcaster which currently // resides in the neutrino package to use all of its functionalities. func broadcastErrorMapper(err error) error { - returnErr := wallet.MapBroadcastBackendError(err) + returnErr := rpcclient.MapRPCErr(err) // We only filter for specific backend errors which are relevant for the // Rebroadcaster. - var errAlreadyConfirmed *wallet.ErrAlreadyConfirmed - var errInMempool *wallet.ErrInMempool - var errMempoolFee *wallet.ErrMempoolFee - switch { // This makes sure the tx is removed from the rebroadcaster once it is // confirmed. - case errors.As(returnErr, &errAlreadyConfirmed): + case errors.Is(returnErr, rpcclient.ErrTxAlreadyKnown), + errors.Is(err, rpcclient.ErrTxAlreadyConfirmed): + returnErr = &pushtx.BroadcastError{ Code: pushtx.Confirmed, Reason: returnErr.Error(), @@ -1461,7 +1460,7 @@ func broadcastErrorMapper(err error) error { // Transactions which are still in mempool but might fall out because // of low fees are rebroadcasted despite of their backend error. - case errors.As(returnErr, &errInMempool): + case errors.Is(returnErr, rpcclient.ErrTxAlreadyInMempool): returnErr = &pushtx.BroadcastError{ Code: pushtx.Mempool, Reason: returnErr.Error(), @@ -1472,7 +1471,7 @@ func broadcastErrorMapper(err error) error { // Mempool conditions change over time so it makes sense to retry // publishing the transaction. Moreover we log the detailed error so the // user can intervene and increase the size of his mempool. - case errors.As(returnErr, &errMempoolFee): + case errors.Is(err, rpcclient.ErrMempoolMinFeeNotMet): ltndLog.Warnf("Error while broadcasting transaction: %v", returnErr) diff --git a/go.mod b/go.mod index 9297fc1d6..54548e663 100644 --- a/go.mod +++ b/go.mod @@ -4,17 +4,17 @@ require ( github.com/NebulousLabs/go-upnp v0.0.0-20180202185039-29b680b06c82 github.com/Yawning/aez v0.0.0-20211027044916-e49e68abd344 github.com/andybalholm/brotli v1.0.3 - github.com/btcsuite/btcd v0.24.1-0.20240301210420-1a2b599bf1af + github.com/btcsuite/btcd v0.24.1-0.20240318151728-2fc99e0496d2 github.com/btcsuite/btcd/btcec/v2 v2.3.2 github.com/btcsuite/btcd/btcutil v1.1.5 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.20240305014015-f7c216e78ee8 + github.com/btcsuite/btcwallet v0.16.10-0.20240318155207-9a7dd2416f4d github.com/btcsuite/btcwallet/wallet/txauthor v1.3.4 github.com/btcsuite/btcwallet/wallet/txrules v1.2.1 - github.com/btcsuite/btcwallet/walletdb v1.4.1 - github.com/btcsuite/btcwallet/wtxmgr v1.5.1 + github.com/btcsuite/btcwallet/walletdb v1.4.2 + github.com/btcsuite/btcwallet/wtxmgr v1.5.3 github.com/coreos/go-systemd v0.0.0-20190719114852-fd7a80b32e1f github.com/davecgh/go-spew v1.1.1 github.com/decred/dcrd/dcrec/secp256k1/v4 v4.0.1 diff --git a/go.sum b/go.sum index 370c8716d..9c1aa38e4 100644 --- a/go.sum +++ b/go.sum @@ -71,8 +71,8 @@ github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6r github.com/btcsuite/btcd v0.20.1-beta/go.mod h1:wVuoA8VJLEcwgqHBwHmzLRazpKxTv13Px/pDuV7OomQ= github.com/btcsuite/btcd v0.22.0-beta.0.20220111032746-97732e52810c/go.mod h1:tjmYdS6MLJ5/s0Fj4DbLgSbDHbEqLJrtnHecBFkdz5M= github.com/btcsuite/btcd v0.23.5-0.20231215221805-96c9fd8078fd/go.mod h1:nm3Bko6zh6bWP60UxwoT5LzdGJsQJaPo6HjduXq9p6A= -github.com/btcsuite/btcd v0.24.1-0.20240301210420-1a2b599bf1af h1:F60A3wst4/fy9Yr1Vn8MYmFlfn7DNLxp8o8UTvhqgBE= -github.com/btcsuite/btcd v0.24.1-0.20240301210420-1a2b599bf1af/go.mod h1:5C8ChTkl5ejr3WHj8tkQSCmydiMEPB0ZhQhehpq7Dgg= +github.com/btcsuite/btcd v0.24.1-0.20240318151728-2fc99e0496d2 h1:b7EiiYEZypI2id3516ptqjzhUfFAgNfF4YVtxikAg6Y= +github.com/btcsuite/btcd v0.24.1-0.20240318151728-2fc99e0496d2/go.mod h1:5C8ChTkl5ejr3WHj8tkQSCmydiMEPB0ZhQhehpq7Dgg= github.com/btcsuite/btcd/btcec/v2 v2.1.0/go.mod h1:2VzYrv4Gm4apmbVVsSq5bqf1Ec8v56E48Vt0Y/umPgA= github.com/btcsuite/btcd/btcec/v2 v2.1.3/go.mod h1:ctjw4H1kknNJmRN4iP1R7bTQ+v3GJkZBd6mui8ZsAZE= github.com/btcsuite/btcd/btcec/v2 v2.3.2 h1:5n0X6hX0Zk+6omWcihdYvdAlGf2DfasC0GMf7DClJ3U= @@ -90,18 +90,18 @@ 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.20240305014015-f7c216e78ee8 h1:fkPZ7LfOCm3Nn0Y/56LOqwooYaw9cV/4jZkdHw7XGik= -github.com/btcsuite/btcwallet v0.16.10-0.20240305014015-f7c216e78ee8/go.mod h1:q+J5AzV+ymzUaBXkP099uuVh18yzCyHHXPc4rxi55mk= +github.com/btcsuite/btcwallet v0.16.10-0.20240318155207-9a7dd2416f4d h1:ikEYkMZEEOwOEpp3DR3EwV6DbZRDPshNw0uJn7W8v7I= +github.com/btcsuite/btcwallet v0.16.10-0.20240318155207-9a7dd2416f4d/go.mod h1:2C3Q/MhYAKmk7F+Tey6LfKtKRTdQsrCf8AAAzzDPmH4= github.com/btcsuite/btcwallet/wallet/txauthor v1.3.4 h1:poyHFf7+5+RdxNp5r2T6IBRD7RyraUsYARYbp/7t4D8= github.com/btcsuite/btcwallet/wallet/txauthor v1.3.4/go.mod h1:GETGDQuyq+VFfH1S/+/7slLM/9aNa4l7P4ejX6dJfb0= github.com/btcsuite/btcwallet/wallet/txrules v1.2.1 h1:UZo7YRzdHbwhK7Rhv3PO9bXgTxiOH45edK5qdsdiatk= github.com/btcsuite/btcwallet/wallet/txrules v1.2.1/go.mod h1:MVSqRkju/IGxImXYPfBkG65FgEZYA4fXchheILMVl8g= github.com/btcsuite/btcwallet/wallet/txsizes v1.2.4 h1:nmcKAVTv/cmYrs0A4hbiC6Qw+WTLYy/14SmTt3mLnCo= github.com/btcsuite/btcwallet/wallet/txsizes v1.2.4/go.mod h1:YqJR8WAAHiKIPesZTr9Cx9Az4fRhRLcJ6GcxzRUZCAc= -github.com/btcsuite/btcwallet/walletdb v1.4.1 h1:NGIGoxx3trpaWqmdOeuhju7KJKp5UM96mQL21idF6RY= -github.com/btcsuite/btcwallet/walletdb v1.4.1/go.mod h1:7ZQ+BvOEre90YT7eSq8bLoxTsgXidUzA/mqbRS114CQ= -github.com/btcsuite/btcwallet/wtxmgr v1.5.1 h1:2yXhMGa4DNz16Mi0e8dVoiFXKOznXlxiGLhB3hKj2uA= -github.com/btcsuite/btcwallet/wtxmgr v1.5.1/go.mod h1:tO4FBSdann0xg/Jtm0grV7t1DzpQMK8nThYVtvSJo/8= +github.com/btcsuite/btcwallet/walletdb v1.4.2 h1:zwZZ+zaHo4mK+FAN6KeK85S3oOm+92x2avsHvFAhVBE= +github.com/btcsuite/btcwallet/walletdb v1.4.2/go.mod h1:7ZQ+BvOEre90YT7eSq8bLoxTsgXidUzA/mqbRS114CQ= +github.com/btcsuite/btcwallet/wtxmgr v1.5.3 h1:QrWCio9Leh3DwkWfp+A1SURj8pYn3JuTLv3waP5uEro= +github.com/btcsuite/btcwallet/wtxmgr v1.5.3/go.mod h1:M4nQpxGTXiDlSOODKXboXX7NFthmiBNjzAKKNS7Fhjg= github.com/btcsuite/go-socks v0.0.0-20170105172521-4720035b7bfd h1:R/opQEbFEy9JGkIguV40SvRY1uliPX8ifOvi6ICsFCw= github.com/btcsuite/go-socks v0.0.0-20170105172521-4720035b7bfd/go.mod h1:HHNXQzUsZCxOoE+CPiyCTO6x34Zs86zZUiwtpXoGdtg= github.com/btcsuite/golangcrypto v0.0.0-20150304025918-53f62d9b43e8/go.mod h1:tYvUd8KLhm/oXvUeSEs2VlLghFjQt9+ZaF9ghH0JNjc= diff --git a/lnwallet/btcwallet/btcwallet.go b/lnwallet/btcwallet/btcwallet.go index 3790af507..c4c2c52fc 100644 --- a/lnwallet/btcwallet/btcwallet.go +++ b/lnwallet/btcwallet/btcwallet.go @@ -29,7 +29,6 @@ import ( "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" ) @@ -1191,6 +1190,32 @@ func (b *BtcWallet) ListUnspentWitness(minConfs, maxConfs int32, return witnessOutputs, nil } +// mapRpcclientError maps an error from the rpcclient package to defined error +// in this package. +// +// NOTE: we are mapping the errors returned from `sendrawtransaction` RPC or +// the reject reason from `testmempoolaccept` RPC. +func mapRpcclientError(err error) error { + // If we failed to publish the transaction, check whether we got an + // error of known type. + switch { + // If the wallet reports a double spend, convert it to our internal + // ErrDoubleSpend and return. + case errors.Is(err, rpcclient.ErrMempoolConflict), + errors.Is(err, rpcclient.ErrMissingInputs): + + return lnwallet.ErrDoubleSpend + + // 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 errors.Is(err, rpcclient.ErrMempoolMinFeeNotMet): + return fmt.Errorf("%w: %v", lnwallet.ErrMempoolFee, err.Error()) + } + + return err +} + // PublishTransaction performs cursory validation (dust checks, etc), then // finally broadcasts the passed transaction to the Bitcoin network. If // publishing the transaction fails, an error describing the reason is returned @@ -1198,40 +1223,12 @@ 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 { - // 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 { - // If the wallet reports a double spend, convert it to our - // internal ErrDoubleSpend and return. - 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 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 - // ErrMempoolFee and return. - case lnutils.ErrorAs[*base.ErrMempoolFee](err): - return fmt.Errorf("%w: %v", lnwallet.ErrMempoolFee, - err.Error()) - } - - 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) + return mapRpcclientError(err) } // For non-neutrino nodes, we will first check whether the transaction @@ -1250,7 +1247,7 @@ func (b *BtcWallet) PublishTransaction(tx *wire.MsgTx, label string) error { err := b.wallet.PublishTransaction(tx, label) - return handleErr(err) + return mapRpcclientError(err) } return err @@ -1269,7 +1266,7 @@ func (b *BtcWallet) PublishTransaction(tx *wire.MsgTx, label string) error { if result.Allowed { err = b.wallet.PublishTransaction(tx, label) - return handleErr(err) + return mapRpcclientError(err) } // If the check failed, there's no need to publish it. We'll handle the @@ -1279,7 +1276,7 @@ func (b *BtcWallet) PublishTransaction(tx *wire.MsgTx, label string) error { // 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)) + err = rpcclient.MapRPCErr(errors.New(result.RejectReason)) //nolint:lll // These two errors are ignored inside `PublishTransaction`: @@ -1288,24 +1285,24 @@ func (b *BtcWallet) PublishTransaction(tx *wire.MsgTx, label string) error { // 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 + // the same tx twice, we'd always get ErrTxAlreadyInMempool. 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): + case errors.Is(err, rpcclient.ErrTxAlreadyInMempool), + errors.Is(err, rpcclient.ErrTxAlreadyKnown), + errors.Is(err, rpcclient.ErrTxAlreadyConfirmed): err := b.wallet.PublishTransaction(tx, label) - - return handleErr(err) + return mapRpcclientError(err) } - return handleErr(err) + return mapRpcclientError(err) } // LabelTransaction adds a label to a transaction. If the tx already From c00b7101d2e4cf6a2745e5cd53532ab62722d033 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 15 Mar 2024 22:12:04 +0800 Subject: [PATCH 2/4] lnwallet: update the unit tests to check the new errors --- lnwallet/test/test_interface.go | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/lnwallet/test/test_interface.go b/lnwallet/test/test_interface.go index caa222cf4..472a0d179 100644 --- a/lnwallet/test/test_interface.go +++ b/lnwallet/test/test_interface.go @@ -1819,8 +1819,7 @@ func testPublishTransaction(r *rpctest.Harness, require.NoError(t, err, "unable to obtain public key") // 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. + // tx3, and that pays to a different address. tx5, err := txFromOutput( tx3, alice.Cfg.Signer, keyDesc.PubKey, keyDesc2.PubKey, txFee, rbf, @@ -1828,7 +1827,19 @@ func testPublishTransaction(r *rpctest.Harness, require.NoError(t, err) err = alice.PublishTransaction(tx5, labels.External) - require.ErrorIs(t, err, lnwallet.ErrDoubleSpend) + + // If RBF is not enabled, we expect this to be rejected + // because it is a double spend. + expectedErr := lnwallet.ErrDoubleSpend + + // If RBF is enabled, we expect it to be rejected + // because it doesn't pay enough fees. + if rbf { + expectedErr = rpcclient.ErrInsufficientFee + } + + // Assert the expected error. + require.ErrorIsf(t, err, expectedErr, "has rbf=%v", rbf) // Create another transaction that spends the same // output, but has a higher fee. We expect also this tx @@ -1906,21 +1917,8 @@ func testPublishTransaction(r *rpctest.Harness, // 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) - - 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) + require.ErrorIs(t, err, rpcclient.ErrOversizeTx) }) } From 03afb72918b095560c4bdaf0d3bea03a74bb0bda Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 15 Mar 2024 22:12:23 +0800 Subject: [PATCH 3/4] workflows: remove loggings from unit test These loggings are not helpful. --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 4a87854bf..ec7554555 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -218,7 +218,7 @@ jobs: run: ./scripts/install_bitcoind.sh - name: run ${{ matrix.unit_type }} - run: make log="stdlog debug" ${{ matrix.unit_type }} + run: make ${{ matrix.unit_type }} - name: Send coverage uses: shogo82148/actions-goveralls@v1 From 9dace4377af373ee3029c7888e082353d2e0d209 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 15 Mar 2024 08:17:40 +0800 Subject: [PATCH 4/4] docs: update release notes --- docs/release-notes/release-notes-0.18.0.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/release-notes/release-notes-0.18.0.md b/docs/release-notes/release-notes-0.18.0.md index adbc887e9..738cc955b 100644 --- a/docs/release-notes/release-notes-0.18.0.md +++ b/docs/release-notes/release-notes-0.18.0.md @@ -220,6 +220,10 @@ * Allow `healthcheck` package users to provide [custom callbacks](https://github.com/lightningnetwork/lnd/pull/8504) which will execute whenever a healthcheck succeeds/fails. +* `PublishTransaction` now [returns the error + types](https://github.com/lightningnetwork/lnd/pull/8554) defined in + `btcd/rpcclient`. + ### Logging * [Add the htlc amount](https://github.com/lightningnetwork/lnd/pull/8156) to contract court logs in case of timed-out htlcs in order to easily spot dust