diff --git a/server.go b/server.go index 4ad5c09e34f..e9f922b4590 100644 --- a/server.go +++ b/server.go @@ -1304,6 +1304,7 @@ func newServer(ctx context.Context, cfg *Config, listenAddrs []net.Addr, Estimator: cc.FeeEstimator, Notifier: cc.ChainNotifier, AuxSweeper: s.implCfg.AuxSweeper, + ChainIO: cc.ChainIO, }) s.sweeper = sweep.New(&sweep.UtxoSweeperConfig{ diff --git a/sweep/fee_bumper.go b/sweep/fee_bumper.go index 1602165ea56..263cf441173 100644 --- a/sweep/fee_bumper.go +++ b/sweep/fee_bumper.go @@ -5,6 +5,7 @@ import ( "fmt" "sync" "sync/atomic" + "time" "github.com/btcsuite/btcd/btcutil" "github.com/btcsuite/btcd/chaincfg/chainhash" @@ -20,10 +21,15 @@ import ( "github.com/lightningnetwork/lnd/lntypes" "github.com/lightningnetwork/lnd/lnutils" "github.com/lightningnetwork/lnd/lnwallet" + "github.com/lightningnetwork/lnd/lnwallet/btcwallet" "github.com/lightningnetwork/lnd/lnwallet/chainfee" "github.com/lightningnetwork/lnd/tlv" ) +// spentNotificationTimeout defines the time to wait for a spending event from +// `RegisterSpendNtfn` when an immediate response is expected. +const spentNotificationTimeout = 1 * time.Second + var ( // ErrInvalidBumpResult is returned when the bump result is invalid. ErrInvalidBumpResult = errors.New("invalid bump result") @@ -111,8 +117,8 @@ const ( // error, which means they cannot be retried with increased budget. TxFatal - // sentinalEvent is used to check if an event is unknown. - sentinalEvent + // sentinelEvent is used to check if an event is unknown. + sentinelEvent ) // String returns a human-readable string for the event. @@ -137,13 +143,13 @@ func (e BumpEvent) String() string { // Unknown returns true if the event is unknown. func (e BumpEvent) Unknown() bool { - return e >= sentinalEvent + return e >= sentinelEvent } // BumpRequest is used by the caller to give the Bumper the necessary info to // create and manage potential fee bumps for a set of inputs. type BumpRequest struct { - // Budget givens the total amount that can be used as fees by these + // Budget gives the total amount that can be used as fees by these // inputs. Budget btcutil.Amount @@ -344,6 +350,10 @@ type TxPublisherConfig struct { // Notifier is used to monitor the confirmation status of the tx. Notifier chainntnfs.ChainNotifier + // ChainIO represents an abstraction over a source that can query the + // blockchain. + ChainIO lnwallet.BlockChainIO + // AuxSweeper is an optional interface that can be used to modify the // way sweep transaction are generated. AuxSweeper fn.Option[AuxSweeper] @@ -589,7 +599,7 @@ func (t *TxPublisher) createRBFCompliantTx( // used up the budget, we will return an error // indicating this tx cannot be made. The // sweeper should handle this error and try to - // cluster these inputs differetly. + // cluster these inputs differently. increased, err = f.Increment() if err != nil { return nil, err @@ -1332,7 +1342,7 @@ func (t *TxPublisher) createAndPublishTx( // the fee bumper retry it at next block. // // NOTE: we may get this error if we've bypassed the mempool check, - // which means we are suing neutrino backend. + // which means we are using neutrino backend. if errors.Is(result.Err, chain.ErrInsufficientFee) || errors.Is(result.Err, lnwallet.ErrMempoolFee) { @@ -1415,6 +1425,38 @@ func (t *TxPublisher) getSpentInputs( "%v", op, heightHint) } + // Check whether the input has been spent or not. + utxo, err := t.cfg.ChainIO.GetUtxo( + &op, inp.SignDesc().Output.PkScript, heightHint, t.quit, + ) + if err != nil { + // GetUtxo will return `ErrOutputSpent` when the input + // has already been spent. In that case, the returned + // `utxo` must be nil, which will move us to subscribe + // its spending event below. + if !errors.Is(err, btcwallet.ErrOutputSpent) { + log.Errorf("Failed to get utxo for input=%v: "+ + "%v", op, err) + + // If this is an unexpected error, move to check + // the next input. + continue + } + + log.Tracef("GetUtxo for input=%v, err: %v", op, err) + } + + // If a non-nil utxo is returned it means this input is still + // unspent. Thus we can continue to the next input as there's no + // need to register spend notification for it. + if utxo != nil { + log.Tracef("Input=%v not spent yet", op) + continue + } + + log.Debugf("Input=%v already spent, fetching its spending "+ + "tx...", op) + // If the input has already been spent after the height hint, a // spend event is sent back immediately. spendEvent, err := t.cfg.Notifier.RegisterSpendNtfn( @@ -1424,13 +1466,13 @@ func (t *TxPublisher) getSpentInputs( log.Criticalf("Failed to register spend ntfn for "+ "input=%v: %v", op, err) - return nil + return spentInputs } // Remove the subscription when exit. defer spendEvent.Cancel() - // Do a non-blocking read to see if the output has been spent. + // Do a blocking read to receive the spent event. select { case spend, ok := <-spendEvent.Spend: if !ok { @@ -1446,9 +1488,19 @@ func (t *TxPublisher) getSpentInputs( spentInputs[op] = spendingTx - // Move to the next input. - default: - log.Tracef("Input %v not spent yet", op) + // The above spent event should be returned immediately, yet we + // still perform a timeout check here in case it blocks forever. + // + // TODO(yy): The proper way to fix this is to redesign the area + // so we use the async flow for checking whether a given input + // is spent or not. A better approach is to implement a new + // synchronous method to check for spending, which should be + // attempted when implementing SQL into btcwallet. + case <-time.After(spentNotificationTimeout): + log.Warnf("Input is reported as spent by GetUtxo, "+ + "but spending notification is not returned "+ + "immediately: input=%v, heightHint=%v", op, + heightHint) } } diff --git a/sweep/fee_bumper_test.go b/sweep/fee_bumper_test.go index aa5561b5ac1..d432337f3c4 100644 --- a/sweep/fee_bumper_test.go +++ b/sweep/fee_bumper_test.go @@ -14,6 +14,7 @@ import ( "github.com/lightningnetwork/lnd/fn/v2" "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/keychain" + "github.com/lightningnetwork/lnd/lnmock" "github.com/lightningnetwork/lnd/lnwallet" "github.com/lightningnetwork/lnd/lnwallet/chainfee" "github.com/stretchr/testify/mock" @@ -73,7 +74,7 @@ func TestBumpResultValidate(t *testing.T) { // Unknown event type will give an error. b = BumpResult{ Tx: &wire.MsgTx{}, - Event: sentinalEvent, + Event: sentinelEvent, } require.ErrorIs(t, b.Validate(), ErrInvalidBumpResult) @@ -384,6 +385,7 @@ type mockers struct { wallet *MockWallet estimator *chainfee.MockEstimator notifier *chainntnfs.MockChainNotifier + chainIO *lnmock.MockChain feeFunc *MockFeeFunction } @@ -405,12 +407,16 @@ func createTestPublisher(t *testing.T) (*TxPublisher, *mockers) { // Create a mock chain notifier. notifier := &chainntnfs.MockChainNotifier{} + // Create a mock chain IO. + chainIO := &lnmock.MockChain{} + t.Cleanup(func() { estimator.AssertExpectations(t) feeFunc.AssertExpectations(t) signer.AssertExpectations(t) wallet.AssertExpectations(t) notifier.AssertExpectations(t) + chainIO.AssertExpectations(t) }) m := &mockers{ @@ -418,6 +424,7 @@ func createTestPublisher(t *testing.T) (*TxPublisher, *mockers) { wallet: wallet, estimator: estimator, notifier: notifier, + chainIO: chainIO, feeFunc: feeFunc, } @@ -427,6 +434,7 @@ func createTestPublisher(t *testing.T) (*TxPublisher, *mockers) { Signer: m.signer, Wallet: m.wallet, Notifier: m.notifier, + ChainIO: m.chainIO, AuxSweeper: fn.Some[AuxSweeper](&MockAuxSweeper{}), }) @@ -457,7 +465,7 @@ func TestCreateAndCheckTx(t *testing.T) { // // NOTE: we are not testing the utility of creating valid txes here, so // this is fine to be mocked. This behaves essentially as skipping the - // Signer check and alaways assume the tx has a valid sig. + // Signer check and always assume the tx has a valid sig. script := &input.Script{} m.signer.On("ComputeInputScript", mock.Anything, mock.Anything).Return(script, nil) @@ -550,7 +558,7 @@ func TestCreateRBFCompliantTx(t *testing.T) { // // NOTE: we are not testing the utility of creating valid txes here, so // this is fine to be mocked. This behaves essentially as skipping the - // Signer check and alaways assume the tx has a valid sig. + // Signer check and always assume the tx has a valid sig. script := &input.Script{} m.signer.On("ComputeInputScript", mock.Anything, mock.Anything).Return(script, nil) @@ -1120,9 +1128,9 @@ func TestBroadcastImmediate(t *testing.T) { require.Empty(t, tp.subscriberChans.Len()) } -// TestCreateAnPublishFail checks all the error cases are handled properly in -// the method createAndPublish. -func TestCreateAnPublishFail(t *testing.T) { +// TestCreateAndPublishFail checks all the error cases are handled properly in +// the method createAndPublishTx. +func TestCreateAndPublishFail(t *testing.T) { t.Parallel() // Create a publisher using the mocks. @@ -1152,7 +1160,7 @@ func TestCreateAnPublishFail(t *testing.T) { // // NOTE: we are not testing the utility of creating valid txes here, so // this is fine to be mocked. This behaves essentially as skipping the - // Signer check and alaways assume the tx has a valid sig. + // Signer check and always assume the tx has a valid sig. script := &input.Script{} m.signer.On("ComputeInputScript", mock.Anything, mock.Anything).Return(script, nil) @@ -1190,9 +1198,9 @@ func TestCreateAnPublishFail(t *testing.T) { require.True(t, resultOpt.IsNone()) } -// TestCreateAnPublishSuccess checks the expected result is returned from the -// method createAndPublish. -func TestCreateAnPublishSuccess(t *testing.T) { +// TestCreateAndPublishSuccess checks the expected result is returned from the +// method createAndPublishTx. +func TestCreateAndPublishSuccess(t *testing.T) { t.Parallel() // Create a publisher using the mocks. @@ -1218,7 +1226,7 @@ func TestCreateAnPublishSuccess(t *testing.T) { // // NOTE: we are not testing the utility of creating valid txes here, so // this is fine to be mocked. This behaves essentially as skipping the - // Signer check and alaways assume the tx has a valid sig. + // Signer check and always assume the tx has a valid sig. script := &input.Script{} m.signer.On("ComputeInputScript", mock.Anything, mock.Anything).Return(script, nil) @@ -1445,7 +1453,7 @@ func TestHandleFeeBumpTx(t *testing.T) { // // NOTE: we are not testing the utility of creating valid txes here, so // this is fine to be mocked. This behaves essentially as skipping the - // Signer check and alaways assume the tx has a valid sig. + // Signer check and always assume the tx has a valid sig. script := &input.Script{} m.signer.On("ComputeInputScript", mock.Anything, mock.Anything).Return(script, nil) @@ -1500,14 +1508,11 @@ func TestProcessRecordsInitial(t *testing.T) { req := createTestBumpRequest() op := req.Inputs[0].OutPoint() - // Mock RegisterSpendNtfn. - // - // Create the spending event that doesn't send an event. - se := &chainntnfs.SpendEvent{ - Cancel: func() {}, - } - m.notifier.On("RegisterSpendNtfn", - &op, mock.Anything, mock.Anything).Return(se, nil).Once() + // Mock GetUtxo to return a utxo, indicating the input is not spent. + inp := req.Inputs[0] + m.chainIO.On("GetUtxo", &op, inp.SignDesc().Output.PkScript, + inp.HeightHint(), mock.Anything, + ).Return(&wire.TxOut{}, nil).Once() // Create a monitor record that's broadcast the first time. record := &monitorRecord{ @@ -1570,6 +1575,13 @@ func TestProcessRecordsInitialSpent(t *testing.T) { tx := &wire.MsgTx{LockTime: 1} op := req.Inputs[0].OutPoint() + // Mock GetUtxo to return nil, indicating the input is spent. + inp := req.Inputs[0] + m.chainIO.On("GetUtxo", + &op, inp.SignDesc().Output.PkScript, inp.HeightHint(), + mock.Anything, + ).Return(nil, nil).Once() + // Mock RegisterSpendNtfn. se := createTestSpendEvent(tx) m.notifier.On("RegisterSpendNtfn", @@ -1622,14 +1634,12 @@ func TestProcessRecordsFeeBump(t *testing.T) { tx := &wire.MsgTx{LockTime: 1} op := req.Inputs[0].OutPoint() - // Mock RegisterSpendNtfn. - // - // Create the spending event that doesn't send an event. - se := &chainntnfs.SpendEvent{ - Cancel: func() {}, - } - m.notifier.On("RegisterSpendNtfn", - &op, mock.Anything, mock.Anything).Return(se, nil).Once() + // Mock GetUtxo to return a utxo, indicating the input is not spent. + inp := req.Inputs[0] + m.chainIO.On("GetUtxo", + &op, inp.SignDesc().Output.PkScript, inp.HeightHint(), + mock.Anything, + ).Return(&wire.TxOut{}, nil).Once() // Create a monitor record that's not confirmed. We know it's not // confirmed because the `SpendEvent` is empty. @@ -1702,6 +1712,13 @@ func TestProcessRecordsConfirmed(t *testing.T) { tx := &wire.MsgTx{LockTime: 1} op := req.Inputs[0].OutPoint() + // Mock GetUtxo to return nil, indicating the input is spent. + inp := req.Inputs[0] + m.chainIO.On("GetUtxo", + &op, inp.SignDesc().Output.PkScript, inp.HeightHint(), + mock.Anything, + ).Return(nil, nil).Once() + // Mock RegisterSpendNtfn. se := createTestSpendEvent(tx) m.notifier.On("RegisterSpendNtfn", @@ -1760,6 +1777,13 @@ func TestProcessRecordsSpent(t *testing.T) { // Create a unknown tx. txUnknown := &wire.MsgTx{LockTime: 2} + // Mock GetUtxo to return nil, indicating the input is spent. + inp := req.Inputs[0] + m.chainIO.On("GetUtxo", + &op, inp.SignDesc().Output.PkScript, inp.HeightHint(), + mock.Anything, + ).Return(nil, nil).Once() + // Mock RegisterSpendNtfn. se := createTestSpendEvent(txUnknown) m.notifier.On("RegisterSpendNtfn", @@ -1830,7 +1854,7 @@ func TestHandleInitialBroadcastSuccess(t *testing.T) { // // NOTE: we are not testing the utility of creating valid txes here, so // this is fine to be mocked. This behaves essentially as skipping the - // Signer check and alaways assume the tx has a valid sig. + // Signer check and always assume the tx has a valid sig. script := &input.Script{} m.signer.On("ComputeInputScript", mock.Anything, mock.Anything).Return(script, nil) @@ -1916,7 +1940,7 @@ func TestHandleInitialBroadcastFail(t *testing.T) { // // NOTE: we are not testing the utility of creating valid txes here, so // this is fine to be mocked. This behaves essentially as skipping the - // Signer check and alaways assume the tx has a valid sig. + // Signer check and always assume the tx has a valid sig. script := &input.Script{} m.signer.On("ComputeInputScript", mock.Anything, mock.Anything).Return(script, nil) @@ -2034,7 +2058,7 @@ func TestHasInputsSpent(t *testing.T) { PkScript: pkScript1, }, } - inp1.On("SignDesc").Return(sd1).Once() + inp1.On("SignDesc").Return(sd1).Twice() pkScript2 := []byte{1} sd2 := &input.SignDescriptor{ @@ -2052,6 +2076,13 @@ func TestHasInputsSpent(t *testing.T) { } walletInp.On("SignDesc").Return(sd3).Once() + // Mock GetUtxo. + // + // Mock GetUtxo to return nil for the first input. + m.chainIO.On("GetUtxo", + &op1, pkScript1, heightHint1, mock.Anything, + ).Return(nil, nil).Once() + // Mock RegisterSpendNtfn. // // spendingTx1 is the tx spending op1. @@ -2060,18 +2091,15 @@ func TestHasInputsSpent(t *testing.T) { m.notifier.On("RegisterSpendNtfn", &op1, pkScript1, heightHint1).Return(se1, nil).Once() - // Create the spending event that doesn't send an event. - se2 := &chainntnfs.SpendEvent{ - Cancel: func() {}, - } - m.notifier.On("RegisterSpendNtfn", - &op2, pkScript2, heightHint2).Return(se2, nil).Once() + // Mock GetUtxo to return a result for the second input. + m.chainIO.On("GetUtxo", + &op2, pkScript2, heightHint2, mock.Anything, + ).Return(&wire.TxOut{}, nil).Once() - se3 := &chainntnfs.SpendEvent{ - Cancel: func() {}, - } - m.notifier.On("RegisterSpendNtfn", - &op3, pkScript3, heightHint3).Return(se3, nil).Once() + // Mock GetUtxo to return a result for the wallet input. + m.chainIO.On("GetUtxo", + &op3, pkScript3, heightHint3, mock.Anything, + ).Return(&wire.TxOut{}, nil).Once() // Prepare the test inputs. inputs := []input.Input{inp1, inp2, walletInp}