neutrino remove sweeptx

[ziggie1984]

Fixes #7790

Change Description

Up until now we were only removing pending sweep inputs when a transaction was confirmed spending an input of the same exclusive group. For neutrino backends this is not enough, we need to make sure that the whole unconfirmed transaction is purged as well. This refers especially to anchor force closes where up to 3 sweep inputs are broadcasted within the same exclusive group. In the worst case scenario 2 additional inputs will be blocked because they are used to sweep anchor outputs which will never confirm because their parent will never make it into the blockchain as soon as one of them is confirmed.
This change does not restrict the removing soley for neutrino backends because it is most likely a NOOP for other backends. Nonetheless I think it does no harm and prevents us from surfacing the backend-structure into the Sweeper package. Open for suggestions here if there is a better way.

Restarting the node will not fix this behaviour because the transaction is still in the unminded bucket which means it will be rebroadcasted and bitcoind nodes will not fail this transaction because they will consider it as orphan.

Enhanced an itest to easily verify this:

make itest icase=multi_hop_local_force_close_on-chain_htlc_timeout/zeroconf=false/committype=ANCHORS backend=neutrino

Steps to Test

Steps for reviewers to follow to test the change.

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.
• There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

[ziggie1984]

A switch from neutrino to normal backends could solve the problem too, so maybe that's the reason why it comes up so late?

While analysing the bug I realised that we also do not save the "ForceClosing" transactions (lncli listchaintxns) when they are confirmed. Maybe the reason for it is that all of the outputs are not part of the Default Wallet ? Do we want fix this too?

[guggero]

Thanks a lot for catching this and providing a fix!
I think this makes sense, but would like to get at least another review from someone who is a bit more familiar with the internals of the sweeper.

[Roasbeef]

Isn't this equivalent to just removing the !isOurTx in this logic block: https://github.com/lightningnetwork/lnd/blob/9ff2d4005f9ec24d3fe56f6e762a6c2a082e2a01/sweep/sweeper.go#L706-L728

[ziggie1984]

Isn't this equivalent to just removing the !isOurTx in this logic block:

Good idea but our use case is a bit different here. So when we launch our CPFP anchor sweeps we do that in the worst case for 3 different commitment txids, (local,remote,remote_dangling) in your function only the inputs are checked which are spent in the transaction, however we must trigger the removal also when sweeps of the same exclusive group are confirmed, because only one commitment tx can make it into the blockchain.

Maybe there is a way to include this removal in this code, instead of introducing another function.

Ok so the interesting part happens here, where all pendingsweeps with the same exclusive group are evaluated and exactly there we have to hook in for the removal of those lingering unminded transaction:
https://github.com/lightningnetwork/lnd/blob/9ff2d4005f9ec24d3fe56f6e762a6c2a082e2a01/sweep/sweeper.go#L756C1-L763.

So I think it's the safest way to include the call in the s.removeExclusiveGroup so that we can be sure the relevant tx is removed?

[ziggie1984]

Regarding our solution in #7879 we should also make sure that we remove invalid transactions when our sweeper exhausts for specific outputs. This would make sure in a case of the anchorsweeps or any other sweep also gets removed from the unminded bucket of the wallet and the Rebroadcaster as well. Basically calling the new RemoveConflict function when we signalAndRemove from the sweeper store.

The removal of transactions in the same exclusive group could also be fixed in another way. So we could add an ancestor field to the ChainNotifier and I think we would also need to add it into btcwallet which also needs to be aware in case an ancestor (in our case the funding output) is mined. Would be a bigger change but maybe more elegant because we actually move things more closely to the software stack where they belong? Wdyt ?

[ziggie1984]

The question here which is still open: How do we remove transactions resulting from this behaviour prior to this fix? Where the channel is already resolved and no more sweeps are registered but the transaction is still stuck in the UnmindedTx bucket ? Offering an Endpoint where noderunner could manually purge UnminedTransactions ? Open for suggestions

[ziggie1984]

The question here which is still open: How do we remove transactions resulting from this behaviour prior to this fix? Where the channel is already resolved and no more sweeps are registered but the transaction is still stuck in the UnmindedTx bucket ? Offering an Endpoint where noderunner could manually purge UnminedTransactions ? Open for suggestions

Rescanning the wallet works here 🙌 user will need to trigger a rescan. However we can maybe add an rpc endpoint nonetheless, facilitating the process in case the user knows what he is missing ?

[Roasbeef]

However we can maybe add an rpc endpoint nonetheless, facilitating the process in case the user knows what he is missing ?

@ziggie1984 I think that's a good idea. Having to add that flag, then also resume with a special unlock flag is kinda annoying. Should be a small change.

[lightninglabs-deploy]

@yyforyongyu: review reminder
@ziggie1984, remember to re-request review from reviewers when ready

[ziggie1984]

!lightninglabs-deploy mute

[ziggie1984]

Added a new walletrpc endpoint RemoveTransaction to easily remove an unconfirmed transaction from the default wallet.

Only relevant for neutrino backends imo, prior to that people could just use lncli wallet publishtx which would have removed the transaction if it was not successfully registered with the mempool.

[yyforyongyu]

Nice addition of the RemoveTransaction! My main question is whether we should combine removeLastSweepDescendants and removeConflictingSweepTx.

[yyforyongyu]

Looking good🏖 Just a few nits. Meanwhile do you think the following case is fixed here?

lnd/itest/lnd_onchain_test.go

Lines 523 to 551 in d9b88fb

	// Send another UTXO if this is a neutrino backend. When sweeping
	// anchors, there are two transactions created, `local_sweep_tx` for
	// sweeping Alice's anchor on the local commitment, `remote_sweep_tx`
	// for sweeping her anchor on the remote commitment. Whenever the force
	// close transaction is published, Alice will always create these two
	// transactions to sweep her anchor.
	// On the other hand, when creating the sweep txes, the anchor itself
	// is not able to cover the fee, so another wallet UTXO is needed. In
	// our test case, there's a change output that can be used from the
	// above funding process. And it's used by both sweep txes - when `lnd`
	// happens to create the `remote_sweep_tx` first, it will receive an
	// error since its parent tx, the remote commitment, is not known,
	// hence freeing the change output to be used by `local_sweep_tx`.
	// For neutrino client, however, it will consider the transaction which
	// sweeps the remote anchor as an orphan tx, and it will neither send
	// it to the mempool nor return an error to free the change output.
	// Thus, if the change output is already used in `remote_sweep_tx`, we
	// won't have UTXO to create `local_sweep_tx`.
	//
	// NOTE: the order of the sweep requests for the two anchors cannot be
	// guaranteed. If the sweeper happens to sweep the remote anchor first,
	// then the test won't pass without the extra UTXO, which is the source
	// of the flakeness.
	//
	// TODO(yy): make a RPC server for sweeper so we can explicitly check
	// and control its state.
	if ht.IsNeutrinoBackend() {
	ht.FundCoins(anchorFeeBuffer, alice)
	}

Was thinking how we can test this new behavior for neutrino backend, and this the above case won't be fixed becase these conflicting transactions are only removed AFTER the sweeping tx is confirmed?

[ziggie1984]

Was thinking how we can test this new behavior for neutrino backend, and this the above case won't be fixed becase these conflicting transactions are only removed AFTER the sweeping tx is confirmed?

I agree it's not fixed because this only removes them if the commitment is confirmed. But the flakiness is fixed when adding this additional input correct ?

[yyforyongyu]

LGTM🙏

But the flakiness is fixed when adding this additional input correct ?

Correct.

[guggero]

Did a first pass. Very nice, definitely very useful to have!
Main ask is to change the RPC/CLI a bit to allow only specifying the TXID. Other than that it's mostly nits.

[guggero]

Great work, we're almost there.
Just one more ask (simplification because of new wallet method we have) and a couple of nits.

[guggero]

We can now fetch a transaction directly from the wallet, see #7654.

[ziggie1984]

I wonder if we also should allow to remove confirmed transactions to shortcircuit the wallet rescan, but I guess for this we would also need to introduce a addTx cmd, so that that we can manually add some utxos if we know for sure that they are ours?

[ziggie1984]

see for example issue #8251, where they kind of know what they are missing ?

[guggero]

Yeah, not sure if that's a good idea... You could really mess up your wallet state with that. At the very least it shouldn't be part of this PR anyway so we can properly discuss the feature (and its consequences) in a standalone PR.

[ziggie1984]

We might think of a --i_know_what_i_am_doing flag and also allow the removal of confirmed txids ?

[guggero]

See my comment above about doing it in a separate PR (if even...). Also, when it comes to naming, I think we'd want something more along the lines of --danger-allow-confirmed since apparently the "I know what I'm doing" approach doesn't seem to work for some users. Psychologically it seems people are more inclined to think they know what they're doing while if something is labelled as "dangerous" they might be more careful (even if they happen to actually know what they are doing).

[ziggie1984]

well said, I observed the same behavior :)

[guggero]

LGTM 🎉