From 6da0f2a1579da9f732c5865a13bc4886bec2daa2 Mon Sep 17 00:00:00 2001 From: ziggie Date: Sat, 3 Jun 2023 14:10:38 +0200 Subject: [PATCH 1/6] mod: bump version of neutrino and btcwallet. --- go.mod | 4 ++-- go.sum | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index b72c58ed3..2e696353b 100644 --- a/go.mod +++ b/go.mod @@ -9,7 +9,7 @@ require ( github.com/btcsuite/btcd/btcutil/psbt v1.1.8 github.com/btcsuite/btcd/chaincfg/chainhash v1.0.2 github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f - github.com/btcsuite/btcwallet v0.16.10-0.20230621165747-9c21f464ce13 + github.com/btcsuite/btcwallet v0.16.10-0.20230706223227-037580c66b74 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 @@ -29,7 +29,7 @@ require ( github.com/jessevdk/go-flags v1.4.0 github.com/jrick/logrotate v1.0.0 github.com/kkdai/bstream v1.0.0 - github.com/lightninglabs/neutrino v0.15.0 + github.com/lightninglabs/neutrino v0.15.1-0.20230710222839-9fd0fc551366 github.com/lightninglabs/neutrino/cache v1.1.1 github.com/lightningnetwork/lightning-onion v1.2.1-0.20221202012345-ca23184850a1 github.com/lightningnetwork/lnd/cert v1.2.1 diff --git a/go.sum b/go.sum index 50e166e2d..9419edfc1 100644 --- a/go.sum +++ b/go.sum @@ -91,8 +91,8 @@ github.com/btcsuite/btcd/chaincfg/chainhash v1.0.2/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.20230621165747-9c21f464ce13 h1:7i0CzK+PP4+Dth9ia/eIBFRw8+K6MT8MfoFqBH43Xts= -github.com/btcsuite/btcwallet v0.16.10-0.20230621165747-9c21f464ce13/go.mod h1:Hl4PP/tSNcgN6himfx/020mYSa19a1qkqTuqQBUU97w= +github.com/btcsuite/btcwallet v0.16.10-0.20230706223227-037580c66b74 h1:RanL5kPm5Gi9YPf+3aPfdd6gJuohoJMxcDv8R033RIo= +github.com/btcsuite/btcwallet v0.16.10-0.20230706223227-037580c66b74/go.mod h1:Hl4PP/tSNcgN6himfx/020mYSa19a1qkqTuqQBUU97w= 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= @@ -390,8 +390,8 @@ github.com/lib/pq v1.10.3 h1:v9QZf2Sn6AmjXtQeFpdoq/eaNtYP6IN+7lcrygsIAtg= github.com/lib/pq v1.10.3/go.mod h1:AlVN5x4E4T544tWzH6hKfbfQvm3HdbOxrmggDNAPY9o= github.com/lightninglabs/gozmq v0.0.0-20191113021534-d20a764486bf h1:HZKvJUHlcXI/f/O0Avg7t8sqkPo78HFzjmeYFl6DPnc= github.com/lightninglabs/gozmq v0.0.0-20191113021534-d20a764486bf/go.mod h1:vxmQPeIQxPf6Jf9rM8R+B4rKBqLA2AjttNxkFBL2Plk= -github.com/lightninglabs/neutrino v0.15.0 h1:yr3uz36fLAq8hyM0TRUVlef1TRNoWAqpmmNlVtKUDtI= -github.com/lightninglabs/neutrino v0.15.0/go.mod h1:pmjwElN/091TErtSE9Vd5W4hpxoG2/+xlb+HoPm9Gug= +github.com/lightninglabs/neutrino v0.15.1-0.20230710222839-9fd0fc551366 h1:++HuI+fNJ61HWobNkj0BvFs477R2Ar3TJABI0gendI8= +github.com/lightninglabs/neutrino v0.15.1-0.20230710222839-9fd0fc551366/go.mod h1:pmjwElN/091TErtSE9Vd5W4hpxoG2/+xlb+HoPm9Gug= github.com/lightninglabs/neutrino/cache v1.1.1 h1:TllWOSlkABhpgbWJfzsrdUaDH2fBy/54VSIB4vVqV8M= github.com/lightninglabs/neutrino/cache v1.1.1/go.mod h1:XJNcgdOw1LQnanGjw8Vj44CvguYA25IMKjWFZczwZuo= github.com/lightninglabs/protobuf-go-hex-display v1.30.0-hex-display h1:pRdza2wleRN1L2fJXd6ZoQ9ZegVFTAb2bOQfruJPKcY= From 54bacec4223ef160aeee8566ffca02e6007b7b59 Mon Sep 17 00:00:00 2001 From: ziggie Date: Sat, 3 Jun 2023 13:58:31 +0200 Subject: [PATCH 2/6] lnwallet: add new ErrMempoolFee error. --- lnwallet/btcwallet/btcwallet.go | 12 ++++++++++-- lnwallet/interface.go | 6 ++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/lnwallet/btcwallet/btcwallet.go b/lnwallet/btcwallet/btcwallet.go index 3050be03a..1535d434b 100644 --- a/lnwallet/btcwallet/btcwallet.go +++ b/lnwallet/btcwallet/btcwallet.go @@ -1192,8 +1192,9 @@ func (b *BtcWallet) ListUnspentWitness(minConfs, maxConfs int32, // 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 -// (currently ErrDoubleSpend). If the transaction is already published to the -// network (either in the mempool or chain) no error will be returned. +// and mapped to the wallet's internal error types. If the transaction is +// 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 { // If we failed to publish the transaction, check whether we @@ -1210,6 +1211,13 @@ func (b *BtcWallet) PublishTransaction(tx *wire.MsgTx, label string) error { case *base.ErrReplacement: 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 *base.ErrMempoolFee: + return fmt.Errorf("%w: %v", lnwallet.ErrMempoolFee, + err.Error()) + default: return err } diff --git a/lnwallet/interface.go b/lnwallet/interface.go index f27e6345d..d8dc9d5b8 100644 --- a/lnwallet/interface.go +++ b/lnwallet/interface.go @@ -69,6 +69,12 @@ var ( // ErrNotMine is an error denoting that a WalletController instance is // unable to spend a specified output. ErrNotMine = errors.New("the passed output doesn't belong to the wallet") + + // ErrMempoolFee is returned from PublishTransaction in case the tx + // being published is not accepted into mempool because the fee + // requirements of the mempool backend are not met. + ErrMempoolFee = errors.New("transaction rejected by the mempool " + + "because of low fees") ) // ErrNoOutputs is returned if we try to create a transaction with no outputs From 8314f0a879a5bff2974f5181e9eedd3f19f4820c Mon Sep 17 00:00:00 2001 From: ziggie Date: Sat, 3 Jun 2023 14:05:16 +0200 Subject: [PATCH 3/6] contractcourt: handle mempool min fee error. In case the mempool backend signals that our transaction does not meet fee requirements when publishing it we will continue to start up now. The transaction will be rebroadcasted in the background and a specific log message will be printed to let the user know that he could increase his mempool size to at least have this transaction in his own mempool. --- contractcourt/channel_arbitrator.go | 13 ++++++++++--- contractcourt/utxonursery.go | 9 ++++++++- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index eeb8850f7..b19bae7fe 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -553,7 +553,7 @@ func (c *ChannelArbitrator) Start(state *chanArbStartState) error { // StateWaitingFullResolution after we've transitioned from // StateContractClosed which can only be triggered by the local // or remote close trigger. This trigger is only fired when we - // receive a chain event from the chain watcher than the + // receive a chain event from the chain watcher that the // commitment has been confirmed on chain, and before we // advance our state step, we call InsertConfirmedCommitSet. err := c.relaunchResolvers(state.commitSet, triggerHeight) @@ -990,11 +990,18 @@ func (c *ChannelArbitrator) stateStep( label := labels.MakeLabel( labels.LabelTypeChannelClose, &c.cfg.ShortChanID, ) - if err := c.cfg.PublishTx(closeTx, label); err != nil { log.Errorf("ChannelArbitrator(%v): unable to broadcast "+ "close tx: %v", c.cfg.ChanPoint, err) - if err != lnwallet.ErrDoubleSpend { + + // This makes sure we don't fail at startup if the + // commitment transaction has too low fees to make it + // into mempool. The rebroadcaster makes sure this + // transaction is republished regularly until confirmed + // or replaced. + if !errors.Is(err, lnwallet.ErrDoubleSpend) && + !errors.Is(err, lnwallet.ErrMempoolFee) { + return StateError, closeTx, err } } diff --git a/contractcourt/utxonursery.go b/contractcourt/utxonursery.go index 3776a3546..a763679ed 100644 --- a/contractcourt/utxonursery.go +++ b/contractcourt/utxonursery.go @@ -3,6 +3,7 @@ package contractcourt import ( "bytes" "encoding/binary" + "errors" "fmt" "io" "sync" @@ -872,7 +873,13 @@ func (u *UtxoNursery) sweepCribOutput(classHeight uint32, baby *babyOutput) erro // confirmed before transitioning it to kindergarten. label := labels.MakeLabel(labels.LabelTypeSweepTransaction, nil) err := u.cfg.PublishTransaction(baby.timeoutTx, label) - if err != nil && err != lnwallet.ErrDoubleSpend { + + // In case the tx does not meet mempool fee requirements we continue + // because the tx is rebroadcasted in the background and there is + // nothing we can do to bump this transaction anyways. + if err != nil && !errors.Is(err, lnwallet.ErrDoubleSpend) && + !errors.Is(err, lnwallet.ErrMempoolFee) { + utxnLog.Errorf("Unable to broadcast baby tx: "+ "%v, %v", err, spew.Sdump(baby.timeoutTx)) return err From c88ff144773fcfa7624d4620e161aa754ce88429 Mon Sep 17 00:00:00 2001 From: ziggie Date: Wed, 7 Jun 2023 10:00:13 +0200 Subject: [PATCH 4/6] contractcourt: add tests for mempool rejection. Add a test where the channel arbitrator starts up correctly when a prior unilateral close of a channel did not broadcast for specific reasons. Also add a test which ensures that when a crib output is rejected by the bitcoin backend the startup works correctly for specific errors. --- contractcourt/channel_arbitrator_test.go | 85 ++++++++++++++++++ contractcourt/utxonursery_test.go | 106 ++++++++++++++++++++++- 2 files changed, 189 insertions(+), 2 deletions(-) diff --git a/contractcourt/channel_arbitrator_test.go b/contractcourt/channel_arbitrator_test.go index 3cda7a666..48aea1a12 100644 --- a/contractcourt/channel_arbitrator_test.go +++ b/contractcourt/channel_arbitrator_test.go @@ -2686,6 +2686,91 @@ func TestChannelArbitratorAnchors(t *testing.T) { ) } +// TestChannelArbitratorStartAfterCommitmentRejected tests that when we run into +// the case where our commitment tx is rejected by our bitcoin backend we still +// continue to startup the arbitrator for a specific set of errors. +func TestChannelArbitratorStartAfterCommitmentRejected(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + + // The specific error during broadcasting the transaction. + broadcastErr error + + // expected state when the startup of the arbitrator succeeds. + expectedState ArbitratorState + + expectedStartup bool + }{ + { + name: "Commitment is rejected because of low mempool " + + "fees", + broadcastErr: lnwallet.ErrMempoolFee, + expectedState: StateCommitmentBroadcasted, + expectedStartup: true, + }, + { + // We map a rejected rbf transaction to ErrDoubleSpend + // in lnd. + name: "Commitment is rejected because of a " + + "rbf transaction not succeeding", + broadcastErr: lnwallet.ErrDoubleSpend, + expectedState: StateCommitmentBroadcasted, + expectedStartup: true, + }, + { + name: "Commitment is rejected with an " + + "unmatched error", + broadcastErr: fmt.Errorf("Reject Commitment Tx"), + expectedState: StateBroadcastCommit, + expectedStartup: false, + }, + } + + for _, test := range tests { + test := test + + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + // We'll create the arbitrator and its backing log + // to signal that it's already in the process of being + // force closed. + log := &mockArbitratorLog{ + newStates: make(chan ArbitratorState, 5), + state: StateBroadcastCommit, + } + chanArbCtx, err := createTestChannelArbitrator(t, log) + require.NoError(t, err, "unable to create "+ + "ChannelArbitrator") + + chanArb := chanArbCtx.chanArb + + // Customize the PublishTx function of the arbitrator. + chanArb.cfg.PublishTx = func(*wire.MsgTx, + string) error { + + return test.broadcastErr + } + err = chanArb.Start(nil) + if !test.expectedStartup { + require.ErrorIs(t, err, test.broadcastErr) + return + } + require.NoError(t, err) + + t.Cleanup(func() { + require.NoError(t, chanArb.Stop()) + }) + + // In case the startup succeeds we check that the state + // is as expected. + chanArbCtx.AssertStateTransitions(test.expectedState) + }) + } +} + // putResolverReportInChannel returns a put report function which will pipe // reports into the channel provided. func putResolverReportInChannel(reports chan *channeldb.ResolverReport) func( diff --git a/contractcourt/utxonursery_test.go b/contractcourt/utxonursery_test.go index 3f5026ddc..05cda32ca 100644 --- a/contractcourt/utxonursery_test.go +++ b/contractcourt/utxonursery_test.go @@ -504,8 +504,7 @@ func createNurseryTestContext(t *testing.T, /// Restart nursery. nurseryCfg.SweepInput = ctx.sweeper.sweepInput ctx.nursery = NewUtxoNursery(&nurseryCfg) - ctx.nursery.Start() - + require.NoError(t, ctx.nursery.Start()) }) } @@ -646,6 +645,109 @@ func incubateTestOutput(t *testing.T, nursery *UtxoNursery, return outgoingRes } +// TestRejectedCribTransaction makes sure that our nursery does not fail to +// start up in case a Crib transaction (htlc-timeout) is rejected by the +// bitcoin backend for some excepted reasons. +func TestRejectedCribTransaction(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + + // The specific error during broadcasting the transaction. + broadcastErr error + + // expectErr specifies whether the rejection of the transaction + // fails the nursery engine. + expectErr bool + }{ + { + name: "Crib tx is rejected because of low mempool " + + "fees", + broadcastErr: lnwallet.ErrMempoolFee, + }, + { + // We map a rejected rbf transaction to ErrDoubleSpend + // in lnd. + name: "Crib tx is rejected because of a " + + "rbf transaction not succeeding", + broadcastErr: lnwallet.ErrDoubleSpend, + }, + { + name: "Crib tx is rejected with an " + + "unmatched error", + broadcastErr: fmt.Errorf("Reject Commitment Tx"), + expectErr: true, + }, + } + + for _, test := range tests { + test := test + + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + // The checkStartStop function just calls the callback + // here to make sure the restart routine works + // correctly. + ctx := createNurseryTestContext(t, + func(callback func()) bool { + callback() + return true + }) + + outgoingRes := createOutgoingRes(true) + + ctx.nursery.cfg.PublishTransaction = + func(tx *wire.MsgTx, source string) error { + log.Tracef("Publishing tx %v "+ + "by %v", tx.TxHash(), source) + return test.broadcastErr + } + ctx.notifyEpoch(125) + + // Hand off to nursery. + err := ctx.nursery.IncubateOutputs( + testChanPoint, + []lnwallet.OutgoingHtlcResolution{*outgoingRes}, + nil, 0, + ) + if test.expectErr { + require.ErrorIs(t, err, test.broadcastErr) + return + } + require.NoError(t, err) + + // Make sure that a restart is not affected by the + // rejected Crib transaction. + ctx.restart() + + // Confirm the timeout tx. This should promote the + // HTLC to KNDR state. + timeoutTxHash := outgoingRes.SignedTimeoutTx.TxHash() + err = ctx.notifier.ConfirmTx(&timeoutTxHash, 126) + require.NoError(t, err) + + // Wait for output to be promoted in store to KNDR. + select { + case <-ctx.store.cribToKinderChan: + case <-time.After(defaultTestTimeout): + t.Fatalf("output not promoted to KNDR") + } + + // Notify arrival of block where second level HTLC + // unlocks. + ctx.notifyEpoch(128) + + // Check final sweep into wallet. + testSweepHtlc(t, ctx) + + // Cleanup utxonursery. + ctx.finish() + }) + } +} + func assertNurseryReport(t *testing.T, nursery *UtxoNursery, expectedNofHtlcs int, expectedStage uint32, expectedLimboBalance btcutil.Amount) { From 521a67c79598d4603922f6c04d0cc8f2dd54944e Mon Sep 17 00:00:00 2001 From: ziggie Date: Sat, 24 Jun 2023 00:00:37 +0200 Subject: [PATCH 5/6] lnd: define additional rebroadcaster function. The MapCustomBroadcastError of the rebroadcaster is defined. This let's us use the Rebroadcaster of the neutrino package (pushtx) with other backends and guarantee a consistent behaviour. Prior to this change the rebroadcaster would stop rebroadcasting transactions because they did not meet the neutrino specific broadcastErr type. --- config_builder.go | 52 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/config_builder.go b/config_builder.go index 1cbf17348..1827587e8 100644 --- a/config_builder.go +++ b/config_builder.go @@ -3,6 +3,7 @@ package lnd import ( "bytes" "context" + "errors" "fmt" "io/ioutil" "net" @@ -719,6 +720,10 @@ func (d *DefaultWalletImpl) BuildChainControl( partialChainControl.ChainNotifier, ), RebroadcastInterval: pushtx.DefaultRebroadcastInterval, + // In case the backend is different from neutrino we + // make sure that broadcast backend errors are mapped + // to the neutrino broadcastErr. + MapCustomBroadcastError: broadcastErrorMapper, } lnWalletConfig.Rebroadcaster = newWalletReBroadcaster( @@ -1420,3 +1425,50 @@ func parseHeaderStateAssertion(state string) (*headerfs.FilterHeader, error) { FilterHash: *hash, }, nil } + +// broadcastErrorMapper maps errors from bitcoin backends other than neutrino to +// 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) + + // 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): + returnErr = &pushtx.BroadcastError{ + Code: pushtx.Confirmed, + Reason: returnErr.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): + returnErr = &pushtx.BroadcastError{ + Code: pushtx.Mempool, + Reason: returnErr.Error(), + } + + // Transactions which are not accepted into mempool because of low fees + // in the first place are rebroadcasted despite of their backend 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): + ltndLog.Warnf("Error while broadcasting transaction: %v", + returnErr) + + returnErr = &pushtx.BroadcastError{ + Code: pushtx.Mempool, + Reason: returnErr.Error(), + } + } + + return returnErr +} From a9ad0e520805dac5a1eb4cb71b1841db6043d1d7 Mon Sep 17 00:00:00 2001 From: ziggie Date: Sun, 4 Jun 2023 22:12:40 +0200 Subject: [PATCH 6/6] docs: add release-notes for 0.17.0. --- docs/release-notes/release-notes-0.17.0.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/release-notes/release-notes-0.17.0.md b/docs/release-notes/release-notes-0.17.0.md index ff4711e7a..3d7903a96 100644 --- a/docs/release-notes/release-notes-0.17.0.md +++ b/docs/release-notes/release-notes-0.17.0.md @@ -163,6 +163,11 @@ unlock or create. * Fixed a memory leak found in mempool management handled by [`btcwallet`](https://github.com/lightningnetwork/lnd/pull/7767). + +* Make sure lnd starts up as normal in case a transaction does not meet min + mempool fee requirements. [Handle min mempool fee backend error when a + transaction fails to be broadcasted by the + bitcoind backend](https://github.com/lightningnetwork/lnd/pull/7746) * [Updated bbolt to v1.3.7](https://github.com/lightningnetwork/lnd/pull/7796) in order to address mmap issues affecting certain older iPhone devices.