Skip to content

sweep: fix expected spending events being missed #10060

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
yyforyongyu wants to merge 5 commits into from
Closed

sweep: fix expected spending events being missed #10060

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
940d317
sweep: check utxo set before registering for spending notification
yyforyongyu Jul 8, 2025
1a26723
sweep: wait for spending notification for spent inputs
yyforyongyu Jul 8, 2025
96a6857
sweep: fix unit tests
yyforyongyu Jul 8, 2025
5502d91
sweep: fix typos
yyforyongyu Jul 8, 2025
ea6c132
sweep: add timeout when waiting for spending event
yyforyongyu Jul 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
39 changes: 38 additions & 1 deletion sweep/fee_bumper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ 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"
)
Expand Down Expand Up @@ -344,6 +345,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]
Expand Down Expand Up @@ -1415,6 +1420,38 @@ func (t *TxPublisher) getSpentInputs(
"%v", op, heightHint)
}

// Check whether the input has been spent or not.

@Abdulkbk Abdulkbk Jul 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the GetUtxo call is probably just added here to save us time? because I noticed RegisterSpendNtfn also checks this internally.

@yyforyongyu yyforyongyu Jul 15, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah correct, it creates a shortcut here so we don't need to make unnecessary subscriptions. We only attempt to subscribe for spending when we know it's not in the utxo set, which means either the input has been spent or it's an orphan.

utxo, err := t.cfg.ChainIO.GetUtxo(

@Roasbeef Roasbeef Jul 25, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, will this also populate the spend cache for neutrino backends? Otherwise, this can be a very expensive filter rescan depending on how far back they are.

In other words, this'll block for neutrno backends. Would need to check for behavior with backends that have the txindex off.

@Roasbeef Roasbeef Jul 25, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about just moving back to the spend channel/goroutine? That way it's always active, always watching, and we can handle the notification async when needed.

It would allow us to remove all these other default select cases for spend ntfns. I recall I pointed out a possibility of missed events when this change was originally added.

@yyforyongyu yyforyongyu Jul 31, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, will this also populate the spend cache for neutrino backends? Otherwise, this can be a very expensive filter rescan depending on how far back they are.

FWIW this is already used in RegisterSpendNtfn implemented in neutrino,

lnd/chainntnfs/neutrinonotify/neutrino.go

Line 864 in 4389067

spendReport, err := n.p2pNode.GetUtxo(

What about just moving back to the spend channel/goroutine? That way it's always active, always watching, and we can handle the notification async when needed.

Can try that route, meanwhile there's #10117 that fixes this issue using an alternative approach. I will see if it's possible to make a new sync method when implementing SQL into btcwallet.

&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(
Expand All @@ -1424,7 +1461,7 @@ func (t *TxPublisher) getSpentInputs(
log.Criticalf("Failed to register spend ntfn for "+
"input=%v: %v", op, err)
Comment thread
yyforyongyu marked this conversation as resolved.

return nil
return spentInputs

@Abdulkbk Abdulkbk Jul 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So initially we return nil, and looking at the 2 instances this method is used, there is a check for the length of what was returned if len(spends) == 0 {. That would have caused LND to panic, right?.

A follow-up question is: what happens when we have multiple inputs (I guess that's a possibility), and one fails? Does that affect where we call the method since no error will be returned, and the only check I see is for the length of the returned result?

@yyforyongyu yyforyongyu Jul 15, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning nil here actually returns an empty map, so the nil is actually a zero-value map, thus calling len won't panic.

what happens when we have multiple inputs (I guess that's a possibility), and one fails?

What do you mean one fails? If there's a failure here, then we'd shut down lnd due to Criticalf.

@Abdulkbk Abdulkbk Jul 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean one fails? If there's a failure here, then we'd shut down lnd due to Criticalf.

Ah, I now understand that Criticalf sends a shutdown request after logging the error.

}

// Remove the subscription when exit.
Expand Down