From 19265ac8ed2cfa3090c89a6ef791b43d5687c0da Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Wed, 27 Mar 2024 03:45:07 +0800 Subject: [PATCH] sweep: make sure non-fee related errors are notified So these inputs can be retried by the sweeper. --- sweep/fee_bumper.go | 79 ++++++++++++++++++++++++---------------- sweep/fee_bumper_test.go | 32 ++++++++++------ 2 files changed, 69 insertions(+), 42 deletions(-) diff --git a/sweep/fee_bumper.go b/sweep/fee_bumper.go index 46e932359..58a7f8b45 100644 --- a/sweep/fee_bumper.go +++ b/sweep/fee_bumper.go @@ -13,6 +13,7 @@ import ( "github.com/btcsuite/btcwallet/chain" "github.com/davecgh/go-spew/spew" "github.com/lightningnetwork/lnd/chainntnfs" + "github.com/lightningnetwork/lnd/fn" "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/labels" "github.com/lightningnetwork/lnd/lnutils" @@ -815,22 +816,20 @@ func (t *TxPublisher) handleFeeBumpTx(requestID uint64, r *monitorRecord, // The fee function now has a new fee rate, we will use it to bump the // fee of the tx. - result, err := t.createAndPublishTx(requestID, r) - if err != nil { - log.Errorf("Failed to bump tx %v: %v", oldTxid, err) + resultOpt := t.createAndPublishTx(requestID, r) - return - } - - // Notify the new result. - t.handleResult(result) + // If there's a result, we will notify the caller about the result. + resultOpt.WhenSome(func(result BumpResult) { + // Notify the new result. + t.handleResult(&result) + }) } // createAndPublishTx creates a new tx with a higher fee rate and publishes it // to the network. It will update the record with the new tx and fee rate if // successfully created, and return the result when published successfully. func (t *TxPublisher) createAndPublishTx(requestID uint64, - r *monitorRecord) (*BumpResult, error) { + r *monitorRecord) fn.Option[BumpResult] { // Fetch the old tx. oldTx := r.tx @@ -842,21 +841,8 @@ func (t *TxPublisher) createAndPublishTx(requestID uint64, // directly here. tx, fee, err := t.createAndCheckTx(r.req, r.feeFunction) - // If the tx doesn't not have enought budget, we will return a result - // so the sweeper can handle it by re-clustering the utxos. - if errors.Is(err, ErrNotEnoughBudget) { - log.Warnf("Fail to fee bump tx %v: %v", oldTx.TxHash(), err) - - return &BumpResult{ - Event: TxFailed, - Tx: oldTx, - Err: err, - requestID: requestID, - }, nil - } - - // If the error is not budget related, we will return an error and let - // the fee bumper retry it at next block. + // If the error is fee related, we will return an error and let the fee + // bumper retry it at next block. // // NOTE: we can check the RBF error here and ask the fee function to // recalculate the fee rate. However, this would defeat the purpose of @@ -865,12 +851,40 @@ func (t *TxPublisher) createAndPublishTx(requestID uint64, // - if the deadline is close, we expect the fee function to give us a // higher fee rate. If the fee rate cannot satisfy the RBF rules, it // means the budget is not enough. - if err != nil { - log.Infof("Failed to bump tx %v: %v", oldTx.TxHash(), err) - return nil, err + if errors.Is(err, rpcclient.ErrInsufficientFee) || + errors.Is(err, lnwallet.ErrMempoolFee) { + + log.Debugf("Failed to bump tx %v: %v", oldTx.TxHash(), err) + return fn.None[BumpResult]() } - // Register a new record by overwriting the same requestID. + // If the error is not fee related, we will return a `TxFailed` event + // so this input can be retried. + if err != nil { + // If the tx doesn't not have enought budget, we will return a + // result so the sweeper can handle it by re-clustering the + // utxos. + if errors.Is(err, ErrNotEnoughBudget) { + log.Warnf("Fail to fee bump tx %v: %v", oldTx.TxHash(), + err) + } else { + // Otherwise, an unexpected error occurred, we will + // fail the tx and let the sweeper retry the whole + // process. + log.Errorf("Failed to bump tx %v: %v", oldTx.TxHash(), + err) + } + + return fn.Some(BumpResult{ + Event: TxFailed, + Tx: oldTx, + Err: err, + requestID: requestID, + }) + } + + // The tx has been created without any errors, we now register a new + // record by overwriting the same requestID. t.records.Store(requestID, &monitorRecord{ tx: tx, req: r.req, @@ -881,7 +895,10 @@ func (t *TxPublisher) createAndPublishTx(requestID uint64, // Attempt to broadcast this new tx. result, err := t.broadcast(requestID) if err != nil { - return nil, err + log.Infof("Failed to broadcast replacement tx %v: %v", + tx.TxHash(), err) + + return fn.None[BumpResult]() } // A successful replacement tx is created, attach the old tx. @@ -890,7 +907,7 @@ func (t *TxPublisher) createAndPublishTx(requestID uint64, // If the new tx failed to be published, we will return the result so // the caller can handle it. if result.Event == TxFailed { - return result, nil + return fn.Some(*result) } log.Infof("Replaced tx=%v with new tx=%v", oldTx.TxHash(), tx.TxHash()) @@ -898,7 +915,7 @@ func (t *TxPublisher) createAndPublishTx(requestID uint64, // Otherwise, it's a successful RBF, set the event and return. result.Event = TxReplaced - return result, nil + return fn.Some(*result) } // isConfirmed checks the btcwallet to see whether the tx is confirmed. diff --git a/sweep/fee_bumper_test.go b/sweep/fee_bumper_test.go index f3b67f3bd..5f031a9bf 100644 --- a/sweep/fee_bumper_test.go +++ b/sweep/fee_bumper_test.go @@ -1027,8 +1027,8 @@ func TestCreateAnPublishFail(t *testing.T) { mock.Anything).Return(script, nil) // Call the createAndPublish method. - result, err := tp.createAndPublishTx(requestID, record) - require.NoError(t, err) + resultOpt := tp.createAndPublishTx(requestID, record) + result := resultOpt.UnwrapOrFail(t) // We expect the result to be TxFailed and the error is set in the // result. @@ -1040,14 +1040,23 @@ func TestCreateAnPublishFail(t *testing.T) { // error to be returned from CheckMempoolAcceptance. req.Budget = 1000 - // Mock the testmempoolaccept to return an error. + // Mock the testmempoolaccept to return a fee related error that should + // be ignored. m.wallet.On("CheckMempoolAcceptance", mock.Anything).Return(lnwallet.ErrMempoolFee).Once() - // Call the createAndPublish method and expect an error. - result, err = tp.createAndPublishTx(requestID, record) - require.ErrorIs(t, err, lnwallet.ErrMempoolFee) - require.Nil(t, result) + // Call the createAndPublish method and expect a none option. + resultOpt = tp.createAndPublishTx(requestID, record) + require.True(t, resultOpt.IsNone()) + + // Mock the testmempoolaccept to return a fee related error that should + // be ignored. + m.wallet.On("CheckMempoolAcceptance", + mock.Anything).Return(rpcclient.ErrInsufficientFee).Once() + + // Call the createAndPublish method and expect a none option. + resultOpt = tp.createAndPublishTx(requestID, record) + require.True(t, resultOpt.IsNone()) } // TestCreateAnPublishSuccess checks the expected result is returned from the @@ -1090,8 +1099,8 @@ func TestCreateAnPublishSuccess(t *testing.T) { mock.Anything, mock.Anything).Return(errDummy).Once() // Call the createAndPublish method and expect a failure result. - result, err := tp.createAndPublishTx(requestID, record) - require.NoError(t, err) + resultOpt := tp.createAndPublishTx(requestID, record) + result := resultOpt.UnwrapOrFail(t) // We expect the result to be TxFailed and the error is set. require.Equal(t, TxFailed, result.Event) @@ -1111,8 +1120,9 @@ func TestCreateAnPublishSuccess(t *testing.T) { mock.Anything, mock.Anything).Return(nil).Once() // Call the createAndPublish method and expect a success result. - result, err = tp.createAndPublishTx(requestID, record) - require.NoError(t, err) + resultOpt = tp.createAndPublishTx(requestID, record) + result = resultOpt.UnwrapOrFail(t) + require.True(t, resultOpt.IsSome()) // We expect the result to be TxReplaced and the error is nil. require.Equal(t, TxReplaced, result.Event)