Sweeper: support Inputs with required locktimes and txOuts by halseth · Pull Request #4750 · lightningnetwork/lnd

halseth · 2020-11-06T19:14:30Z

This PR adds support for inputs that put extra restrictions on the sweep tx, namely a locktime and accompanied output.

Two methods are added to the Input interface that for most normal inputs will do nothing:

	// RequiredTxOut returns a non-nil TxOut if input commits to a certain
	// transaction output. This is used in the SINGLE|ANYONECANPAY case to
	// make sure any presigned input is still valid by including the
	// output.
	RequiredTxOut() *wire.TxOut

	// RequiredLockTime returns whether this input commits to a tx locktime
	// that must be used in the transaction including it.
	RequiredLockTime() (uint32, bool)

For certain inputs though, this is needed. Most notably we plan to use this for sweeping and aggregation of second level HTLC transactions for anchor channels. Since these transactions are pre-signed by the channel counterparty using sighash SINGLE|ANYONECANPAY, they can be altered at will as long as the locktime and input-ouput pair stays intact.

By allowing the sweeper to add such input-output pairs to the sweep transaction, we get BYOF (bring-your-own-fees) and HTLC aggregation for free.

Roasbeef

First pass, solid diff. Still need to fully grok the process of zipping/merging the valid input sets. Also I don't see any changes to the resolver to send these outputs directly to the sweeper, I take it that'll come in another PR?

sweep/sweeper.go

input/input.go

sweep/sweeper.go

sweep/sweeper_test.go

input/size.go

sweep/tx_input_set.go

sweep/txgenerator.go

breacharbiter.go

sweep/txgenerator.go

halseth · 2020-11-09T19:32:43Z

Still need to fully grok the process of zipping/merging the valid input sets.

Tried to keep it simple. Should perhaps make it even simpler/dumber for this first iteration? (it is not really where review time should be spent I feel).

Also I don't see any changes to the resolver to send these outputs directly to the sweeper, I take it that'll come in another PR?

Yep, will come in a follow up soon (tm)

sweep/sweeper.go

sweep/tx_input_set.go

sweep/txgenerator.go

Roasbeef

Updated my mental model of the current state of the sweeper by re-reading this package thoroughly since it's shifted a good bit since I had a major review in this area. Didn't generate any additional major comments on my end, though the existing more minor comments still stand. If we're to get what we need in to activate anchors by default in a timely manner, then we'll need to ensure we have quick review cycles on this as well as the planned follow up PRs.

LGTM 🥗

sweep/sweeper.go

Roasbeef · 2020-11-17T02:44:18Z

sweep/tx_input_set.go

As mentioned above this shouldn't happen since the 2 of 2's must be above the dust limit in order to be considered, but good defensive clause to have nevertheless.

Roasbeef · 2020-11-17T02:47:17Z

sweep/txgenerator.go

Same logic is duplicated above from lines 169-175, non-blocking comment.

wpaulino · 2020-11-18T22:22:42Z

sweep/sweeper.go

+			// If the fee rate is less than a's, we break. All the
+			// remining clusters from bs have lower fee rate.
+			if b.sweepFeeRate < a.sweepFeeRate {
+				break


Not sure if I'm missing something, but don't we need to include the remaining input clusters from as before the break? Otherwise they won't be returned as part of finalClusters.

Confirmed with this diff:

diff --git a/sweep/sweeper_test.go b/sweep/sweeper_test.go index 0104903cc..f6123837e 100644 --- a/sweep/sweeper_test.go +++ b/sweep/sweeper_test.go @@ -1513,10 +1513,10 @@ func TestZipClusters(t *testing.T) { createCluster(testInputsA, 5000), }, bs: []inputCluster{ - createCluster(testInputsB, 7000), + createCluster(testInputsB, 3000), }, res: []inputCluster{ - createCluster(testInputsC, 7000), + createCluster(testInputsC, 5000), }, }, { @@ -1558,8 +1558,9 @@ func TestZipClusters(t *testing.T) { for _, test := range testCases { zipped := zipClusters(test.as, test.bs) if !reflect.DeepEqual(zipped, test.res) { - t.Fatalf("[%s] unexpected result: %v", - test.name, spew.Sdump(zipped)) + t.Fatalf("[%s] unexpected result: got %v, want %v", + test.name, spew.Sdump(zipped), + spew.Sdump(test.res)) } } }

Tests fail as only bs is added and items from as are omitted all together. The diff just makes the fee rate of bs lower than all of as so the break gets hit immediately.

An even simpler scenario to consider if when bs is empty (we cluster by lock time and all the inputs have a lock time). With this logic, they'll all be dropped.

Another way of stating is that at each step for as, it either should be:

merged in with bs

or added to the final cluster (non-merged)

The break here means that the last statement can be skipped all together at times.

Good catch! Yeah, this was wrong. I think it is a left over from when I initially had a double loop here.

Went with @Roasbeef's suggestion, and added added the failing test cases: b28bc4d

wpaulino

LGTM after squash.

This is needed to sweep second level HTLC transactions that are signed using SINGLE|ANYONECANPAY, since the input and ouput must be aligned.

If inputs require outputs to be added at the same time, this will change the weight and amount calculations, so we must account for that. We wait to get the weight estimator for the sweep tx until needed, such that we can easily choose whether to include a change output or not in the estimate. This is needed for the case where the second level transactions can pay for their own fee, so no change output is needed.

We also increase the value of the wallet UTXO, as it is needed when we want to afford fees for larger transactions.

Now that inputs might have accompanied outputs to be added to the sweep tx, we add them to the sweep transaction first, and account for it when calculating the change amount.

Add tests that inputs with required TXOuts gets aggregated and swept, while keeping locktimes satisfied.

halseth requested review from Roasbeef and cfromknecht as code owners November 6, 2020 19:14

halseth added anchors HTLC utxo sweeping labels Nov 6, 2020

halseth added this to the 0.12.0 milestone Nov 6, 2020

Roasbeef requested changes Nov 6, 2020

View reviewed changes

halseth force-pushed the sweeper-required-locktime-txout branch 2 times, most recently from 1eaae13 to b1ab6c2 Compare November 9, 2020 19:29

halseth requested a review from Roasbeef November 9, 2020 19:32

Roasbeef reviewed Nov 12, 2020

View reviewed changes

sweep/sweeper.go Outdated Show resolved Hide resolved

sweep/sweeper.go Outdated Show resolved Hide resolved

sweep/tx_input_set.go Outdated Show resolved Hide resolved

sweep/txgenerator.go Outdated Show resolved Hide resolved

Roasbeef approved these changes Nov 17, 2020

View reviewed changes

halseth force-pushed the sweeper-required-locktime-txout branch 3 times, most recently from d971f1a to 151b2db Compare November 17, 2020 11:42

Roasbeef requested review from wpaulino and removed request for cfromknecht November 17, 2020 19:23

halseth mentioned this pull request Nov 18, 2020

[anchors] HTLC second level aggregation in the sweeper #4779

Merged

wpaulino reviewed Nov 18, 2020

View reviewed changes

halseth force-pushed the sweeper-required-locktime-txout branch from 151b2db to 0a4036a Compare November 19, 2020 09:14

halseth requested a review from wpaulino November 19, 2020 09:17

wpaulino approved these changes Nov 20, 2020

View reviewed changes

halseth added 7 commits November 20, 2020 13:06

sweeper: define method createInputClusters

1280870

sweep: add mergeClusters

fa4fd02

sweep+input: add RequiredLockTime to inputs

efd6bc9

input+sweep: let weightestimator take known TxOut

3d20905

sweep: pass dustLimit to CreateSweepTx

aa8fa9d

sweep+input: add RequiredTxOut to inputs

985b783

This is needed to sweep second level HTLC transactions that are signed using SINGLE|ANYONECANPAY, since the input and ouput must be aligned.

halseth added 2 commits November 20, 2020 13:06

sweeper_test: return unique wallet utxos

7f9df26

We also increase the value of the wallet UTXO, as it is needed when we want to afford fees for larger transactions.

sweep: add required TxOuts to sweep tx

616503d

Now that inputs might have accompanied outputs to be added to the sweep tx, we add them to the sweep transaction first, and account for it when calculating the change amount.

halseth force-pushed the sweeper-required-locktime-txout branch from 0a4036a to beaf8f6 Compare November 20, 2020 12:07

sweeper_test: add txOut sweep test

8d2e6de

Add tests that inputs with required TXOuts gets aggregated and swept, while keeping locktimes satisfied.

halseth force-pushed the sweeper-required-locktime-txout branch from beaf8f6 to 8d2e6de Compare November 21, 2020 09:17

halseth merged commit ca91d50 into lightningnetwork:master Nov 21, 2020

Conversation

halseth commented Nov 6, 2020

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halseth commented Nov 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Roasbeef Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

Roasbeef Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

wpaulino Nov 18, 2020

Choose a reason for hiding this comment

Uh oh!

Roasbeef Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roasbeef Nov 19, 2020

Choose a reason for hiding this comment

Uh oh!

halseth Nov 19, 2020

Choose a reason for hiding this comment

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Roasbeef Nov 19, 2020 •

edited

Loading