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

halseth · 2020-11-18T18:41:17Z

This PR finishes the changes needed to make the sweeper bundle HTLC second level transaction in a sweep tx, and resign the inputs.

To achieve this we extend the resolvers to include SignDetails containing signatures and information needed for when the inputs are signed.

Depends on #4750
Depends on #4838

Roasbeef · 2020-11-19T00:58:08Z

input/size.go

@@ -328,6 +328,12 @@ const (
 	AcceptedHtlcScriptSize = 3*1 + 20 + 5*1 + 33 + 8*1 + 20 + 4*1 +
 		33 + 5*1 + 4 + 8*1

+	// AcceptedHtlcScriptSizeConfirmed 143 bytes
+	//
+	// TODO(halseth): the non-confirmed version currently includes the


In that it's currently being over estimated?

Yes, that's why the overhead is commented out below. Aiming to fix this in #4775

iirc we decided having the extra constant wasn't worth it bc we were only overestimating by 3 bytes. what changed?

found the comment: https://github.com/lightningnetwork/lnd/pull/3821/files#r387578921

Yeah, that's right. I will look into in the other PR whether it is worth keeping it this way, but I felt as we now need a new witnesstype for these inputs, it was better to define a separate weight constants for them.

Roasbeef · 2020-11-19T00:59:08Z

input/size.go

+	// AcceptedHtlcSuccessWitnessSizeConfirmed 327 bytes
+	//
+	// Input to second level success tx, spending 1 CSV delayed HTLC output.
+	AcceptedHtlcSuccessWitnessSizeConfirmed = 1 + 1 + 1 + 73 + 1 + 73 + 1 + 32 + 1 +


All the defined constants should use the existing "unrolled annotated" comment structure to be more self documenting, and make it easier to catch any errors in the future.

Hm, not entirely sure what you mean, but probably a discussion for #4775.

I mean a comment annotating the set of integers being summed up here, can mostly be copy-pasted from prior versions.

Roasbeef · 2020-11-19T02:12:29Z

contractcourt/contract_resolvers.go

+
+	// secondLevelConfTarget is the confirmation target we'll use when
+	// adding fees to our second-level HTLC transactions.
+	secondLevelConfTarget = 6


Good enough for now, but eventually this should start to be a function of the current height and the deadline to expiry (if the HTLC was contested).

contractcourt/htlc_success_resolver.go

contractcourt/htlc_timeout_resolver.go

cfromknecht

👍 initial pass to familiarize myself with the changes

cfromknecht · 2020-11-20T01:59:16Z

sweep/sweeper.go

@@ -833,6 +834,103 @@ func (s *UtxoSweeper) clusterBySweepFeeRate(inputs pendingInputs) []inputCluster
 	return inputClusters
 }

+// zipClusters merges pairwise clusters from as and bs such that cluster a from


cfromknecht · 2020-11-20T02:01:47Z

sweep/sweeper.go

+
+	// Go through each cluster in as, and merge with the next one from bs
+	// if it has at least the fee rate needed.
+	for i := range as {


for a, i := range as?

cfromknecht · 2020-11-20T02:07:29Z

sweep/sweeper.go

+
+			// We can merge.
+			merged := mergeClusters(a, bs[j])
+			finalClusters = append(finalClusters, merged...)


what if there are multiple clusters in b that an be merged with this a?

Then it will be merged with the first one compatible. This is definitely not an optimized version, there are certainly "smarter" ways of finding clusters to merge with, but wanted to keep it simple for now.

cfromknecht · 2020-11-20T02:20:38Z

sweep/sweeper.go

+			sweepFeeRate += inputFeeRates[op]
+		}
+
+		sweepFeeRate /= chainfee.SatPerKWeight(len(inputs))


doesn’t this need to be a weighted average based on the inputs’ actual weight? this method assumes all inputs are of equal weight

I think this is correct (if we want to average the feerates), since we taking the average fee rate here, not the fee. This is pre-existing though, I'm just using the same logic as below.

hmm i'm not so sure, say you have two inputs A (100w, 1000sat/kw) and B (200w, 3000sat/kw). individually, input A would pay 100w * 1000sat/kw = 100 sat and B would pay 200w * 3000sat/kw = 600sat. combined they pay 700sat total for 300w, or 2.333sat/w.

the unweighted average of the fee rates gives you 2000sat/kw, so total of 300w * 2000sat/kw = 600sat.

however, a weighted average produces (1000sat/kw + 2 * 3000sat/kw)/3 = 2333sat/kw which gives us the expected fee rate. then 100w * 2333sat/kw + 200w * 2333sat/kw = 700sat as expected.

This is pre-existing though, I'm just using the same logic as below.

This may be, but I still think it's incorrect :P

which gives us the expected fee rate

Sure, but I don't think it is defined what is the "expected" fee rate here. Here we cluster some inputs with different fee rates togheter, and need to decide which fee rate to use for all of them. Even though one is heavier than the other, I'm not sure it is "more correct" to choose a fee rate that is closer to that one.

I do agree though, that it might be a better strategy. Since many tiny outputs could bring the fee rate down (and since they are tiny they don't cost much anyway) it could be more correct to take the weighted average. You okay with me doing this as a follow up (must change it in the pre-existing case as well)?

okay, maybe it helps to clarify the objective. say you have the weights and fee rate for each input, (w_i, r_i), i in [1, n]. my assumption was that this is trying to compute r_agg where sum (w_i * r_i) = r_agg * sum (w_i), where r_agg represents the actual fee rate of the cluster after aggregating all the desired fees paid by individual inputs.

a different example, it might make sense to take r_max = max(r_i), which then forces all lower fee-rate inputs to pay the same fee-rate as the input with highest priority. unlike r_agg, this way creates a sweepFeeRate that never decrease the priority of an input, which seems useful if certain inputs are time-sensitive.

i could also see e.g taking the most recent r_i, which is maybe viewed as the most up-to-date fee rate from our estimator/user, and forcing all inputs to readjust to that. this value could fluctuate though, so maybe it's not ideal for time-sensitive inputs.

i agree that there are different heuristics we can choose here, and one may not necessarily be more correct. if the goal is simply to pick an arbitrary value between min(r_i) and max(r_i), then an unweighted average works. however it still doesn't translate into anything meaningful, which is why imo it seems less correct than some of the other options available.

You okay with me doing this as a follow up (must change it in the pre-existing case as well)?

Sure! Yeah I'm okay with a follow up :)

Issue: #4812

cfromknecht · 2020-11-20T02:26:29Z

sweep/tx_input_set.go

@@ -83,14 +83,18 @@ type txInputSet struct {
 	wallet Wallet
 }

+func dustLimit(relayFee chainfee.SatPerKWeight) btcutil.Amount {
+	return txrules.GetDustThreshold(


this will be wrong for litecoin..

oops. Added to the tracking issue: #3946

sweep/txgenerator.go

cfromknecht · 2020-11-20T02:35:16Z

sweep/tx_input_set.go

+	// If the input comes with a required tx out that is below dust, we
+	// won't add it.
+	reqOut := inp.RequiredTxOut()
+	if reqOut != nil && btcutil.Amount(reqOut.Value) < t.dustLimit {


Is this possible?

it depends on whether the relay fee changes, so I would say it's unlikely.

cfromknecht · 2020-11-20T02:42:00Z

input/size.go

+	//
+	// TODO(halseth): the non-confirmed version currently includes the
+	// overhead.
+	AcceptedHtlcScriptSizeConfirmed = AcceptedHtlcScriptSize // + HtlcConfirmedScriptOverhead


intentional comment?

halseth · 2020-11-23T12:34:22Z

Pushed new version that does checkpointing, and moved much of the logic into separate methods. PTAL

cfromknecht

latest version looking good! just some minor nits

contractcourt/briefcase.go

cfromknecht · 2020-11-25T19:13:35Z

input/size.go

@@ -328,6 +328,12 @@ const (
 	AcceptedHtlcScriptSize = 3*1 + 20 + 5*1 + 33 + 8*1 + 20 + 4*1 +
 		33 + 5*1 + 4 + 8*1

+	// AcceptedHtlcScriptSizeConfirmed 143 bytes
+	//
+	// TODO(halseth): the non-confirmed version currently includes the


iirc we decided having the extra constant wasn't worth it bc we were only overestimating by 3 bytes. what changed?

found the comment: https://github.com/lightningnetwork/lnd/pull/3821/files#r387578921

contractcourt/htlc_success_resolver.go

contractcourt/htlc_timeout_resolver.go

halseth · 2020-11-27T14:40:55Z

Pushed an itest for the HTLC aggregation, and noticed that we cannot rely on the output script when finding the second-level output (since they can be equal for equal HTLCs). Now using the input index instead: https://github.com/lightningnetwork/lnd/compare/b80b30502c9a35fd23128543abb354c531ad0cdc..5e5ab68d384c547e83ac355fe69632e2ecaed08d#diff-b38dbdd572300de530a55756fa7af0afc2935a2a643bdcdef2f12f2253c1d313R356

Here are the full changes since last review (mostly tests and logs in addition to the mentioned bug fix): https://github.com/lightningnetwork/lnd/compare/b80b30502c9a35fd23128543abb354c531ad0cdc..5e5ab68d384c547e83ac355fe69632e2ecaed08d

and addressing the review comments: https://github.com/lightningnetwork/lnd/compare/5e5ab68d384c547e83ac355fe69632e2ecaed08d..f5364880827f01a9e2df7ff71ba1e4852f1898d6

Roasbeef · 2020-11-30T01:33:27Z

lntest/itest/lnd_test.go

 				if ok {
 					return fmt.Errorf("duplicate HashLock")
 				}
-				htlcHashes[string(htlc.HashLock)] = struct{}{}
+				htlcHashes[h] = struct{}{}


input/size.go

Roasbeef · 2020-11-30T01:35:00Z

input/size.go

+	// AcceptedHtlcSuccessWitnessSizeConfirmed 327 bytes
+	//
+	// Input to second level success tx, spending 1 CSV delayed HTLC output.
+	AcceptedHtlcSuccessWitnessSizeConfirmed = 1 + 1 + 1 + 73 + 1 + 73 + 1 + 32 + 1 +


I mean a comment annotating the set of integers being summed up here, can mostly be copy-pasted from prior versions.

contractcourt/htlc_timeout_resolver.go

contractcourt/commit_sweep_resolver.go

contractcourt/htlc_success_resolver_test.go

lntest/itest/lnd_multi-hop_htlc_aggregation_test.go

Roasbeef · 2020-11-30T02:06:55Z

lntest/itest/lnd_multi-hop_htlc_aggregation_test.go

+	// Carol will also sweep her anchor output in a separate tx (since it
+	// will be low fee).
+	if c == commitTypeAnchors {
+		expectedTxes = 4


Dem sweet sweet savings

Roasbeef · 2020-11-30T02:09:14Z

lntest/itest/lnd_multi-hop_htlc_aggregation_test.go

+// resolve them using the second level timeout and success transactions. In
+// case of anchor channels, the second-level spends can also be aggregated and
+// properly feebumped, so we'll check that as well.
+func testMultiHopHtlcAggregation(net *lntest.NetworkHarness, t *harnessTest,


Love how comprehensive this test is!

Roasbeef · 2020-11-30T02:10:59Z

Mainly only non-blocking comments left, can't wait to get this in! This is such an outstanding PR and really moves lnd forward a large step with by finally starting to utilize more of what anchors gives us.

halseth · 2020-11-30T14:38:44Z

Addressed review, and also added unit tests that should cover all scenarios also for the timeout resolver: 558b386

While doing this I noticed I don't have to find the input index manually: 9eaf67f

And also found a small report bug for the re-ordered HTLC outputs: 55d1ae3

Roasbeef · 2020-12-01T01:13:06Z

Lingering linter error:

contractcourt/htlc_success_resolver_test.go:87:15: unlambda: replace `func(c ContractResolver,
	r ...*channeldb.ResolverReport) error {
	return testCtx.checkpoint(c, r...)
}` with `testCtx.checkpoint` (gocritic)
		Checkpoint: func(c ContractResolver,

Roasbeef

LGTM 🧗‍♀️

Should be ready to be rebased and land now! Once in tree, I also plan to run more experiments on testnet to exercise more general functionality and UX from the PoV of a CLI user.

contractcourt/briefcase.go

cfromknecht · 2020-12-02T23:21:13Z

lntest/itest/lnd_test.go

 					continue
 				}
 				return fmt.Errorf("node %x didn't have the "+
 					"payHash %v active", node.PubKey[:],
-					payHash)
+					h)


can this commit be equally accomplished by replacing %v with %x? also doesn't incur the 2x memory overhead

It would, but while debugging I found it useful to just spew the htlcHashed map, so nice to have them hex there IMO.

seems like a simple change to make in the event one needs to debug, but it's not that important

cfromknecht · 2020-12-02T23:47:43Z

contractcourt/htlc_timeout_resolver.go

+		log.Infof("%T(%x): waiting for CSV lock to expire at height %v",
+			h, h.htlc.RHash[:], waitHeight)
+
+		err := waitForHeight(waitHeight, h.Notifier, h.quit)


nit: much of the logic below looks duplicated from before, not sure if there is a way to reuse

The FindInputIndex is removed in later commit (will squash with this one), most of the remainder is custom I think.

yeah was just looking at everything from watiForHeight to SpendInput, it is very similar but i think it's fine here

contractcourt/htlc_timeout_resolver_test.go

contractcourt/htlc_success_resolver.go

contractcourt/htlc_timeout_resolver.go

Only value was populated for some, which would cause code to rely on the PkScript being there to fail.

Since the tests set a quite high fee rate before the node goes to chain, the HTLCs wouldn't be economical to sweep at this fee rate. Pre sweeper handling of the second-level transactions this was not a problem, since the fees were set when the second-levels were created, before the fee estimate was increased.

We define the witness constanst we need for fee estimation for this HTLC second level type.

These will only be used for size upper bound estimations by the sweeper.

To make it usable from other resolvers.

…er pointer To make the linter happy, make a pointer to the inner resolver. Otherwise the linter would complain with copylocks: literal copies lock value since we'll add a mutex to the resolver in following commits.

…RemoteCommitOutput This move the logic for sweeping the HTLC output on the remote commitment into its own method.

…oadcastSuccessTx

The sweep tx is not actually part of the resolver's encoded data, so the checkpointing was essentially a noop.

We add checkpoint assertions and resume the resolver from every checkpoint to ensure it can handle restarts.

success tx This commit makes the HTLC resolutions having non-nil SignDetails (meaning we can re-sign the second-level transactions) go through the sweeper. They will be offered to the sweeper which will cluster them and arrange them on its sweep transaction. When that is done we will further sweep the output on this sweep transaction as any other second-level tx. In this commit we do this for the HTLC success resolver and the accompanying HTLC success transaction.

Test success resolvers going through the sweeper.

This commit moves the code doing the initial spend of the HTLC output of the commit tx into its own method.

…ansaction This commit moves the logic for sweeping the confirmed second-level timeout transaction into its own method. We do a small change to the logic: When setting the spending tx in the report, we use the detected commitspend instead of the presigned tiemout tx. This is to prepare for the coming change where the spending transaction might actually be a re-signed timeout tx, and will therefore have a different txid.

In this commit we make the sweeper handle second level transactions for HTLC timeout resolvers for anchor channels.

Since we are checking HTLC aggregation, we must give the sweeper a bit more time to aggregate them to avoid flakes.

In case of anchor channel types, we mine one less block before we expect the second level sweep to appear in the mempool, since the sweeper sweeps one block earlier than the nursery.

Now that the HTLC second-level transactions are going through the sweeper instead of the nursery, there are a few things we must account for. 1. The sweeper sweeps the CSV locked HTLC output one block earlier than the nursery. 2. The sweeper aggregates several HTLC second levels into one transaction. This also means it is not enough to check txids of the transactions spent by the final sweep, but we must use the actual outpoint to distinguish.

cfromknecht

LGTM ⚓️

halseth added this to the 0.12.0 milestone Nov 18, 2020

halseth requested a review from Roasbeef November 18, 2020 18:41

halseth requested a review from cfromknecht as a code owner November 18, 2020 18:41

Roasbeef requested changes Nov 19, 2020

View reviewed changes

cfromknecht reviewed Nov 20, 2020

View reviewed changes

halseth force-pushed the anchor-htlc-aggregation branch 2 times, most recently from 291f669 to e5c10db Compare November 23, 2020 12:08

halseth requested review from cfromknecht and Roasbeef November 23, 2020 12:34

cfromknecht reviewed Nov 25, 2020

View reviewed changes

halseth force-pushed the anchor-htlc-aggregation branch 3 times, most recently from 5e5ab68 to f536488 Compare November 27, 2020 14:35

halseth requested a review from cfromknecht November 27, 2020 14:41

Roasbeef reviewed Nov 30, 2020

View reviewed changes

halseth force-pushed the anchor-htlc-aggregation branch 3 times, most recently from f50b1ba to 9eaf67f Compare November 30, 2020 14:37

halseth requested a review from Roasbeef November 30, 2020 14:38

Roasbeef approved these changes Dec 1, 2020

View reviewed changes

halseth mentioned this pull request Dec 1, 2020

Optimize cluster sweep fee rates #4812

Open

halseth force-pushed the anchor-htlc-aggregation branch from 9eaf67f to 6c2ed90 Compare December 1, 2020 12:57

cfromknecht reviewed Dec 2, 2020

View reviewed changes

halseth added 25 commits December 10, 2020 14:24

lnwallet/channel: properly set SignDesc.Output

eb8d22e

Only value was populated for some, which would cause code to rely on the PkScript being there to fail.

input+lnwallet+contractcourt: define SignDetails for HTLC resolutions

1e68cdc

sweep/txgenerator: log in case of no change output

6150995

sweeper/tx_input_set: add logging for input set construction

83f9aae

itest: use hex encoded hash in error message

241e21a

input/size: define witness constants needed

8eb6d7c

We define the witness constanst we need for fee estimation for this HTLC second level type.

input/witnessgen: define witness type for HTLC 2nd level inputs

65e50f6

These will only be used for size upper bound estimations by the sweeper.

contractcourt: decouple waitForHeight from commit sweep resolver

5f61314

To make it usable from other resolvers.

contractcourt/succes+timeout resolver: extract waitForSpend logic

4da2b29

contractcourt/success_resolver: extract remoteHTLC sweep into resolve…

9d33b00

…RemoteCommitOutput This move the logic for sweeping the HTLC output on the remote commitment into its own method.

contractcourt/success_resolver: extract HTLC success handling into br…

0b84d5f

…oadcastSuccessTx

contractcourt/success_resolver: remove sweep tx checkpoint

7142a30

The sweep tx is not actually part of the resolver's encoded data, so the checkpointing was essentially a noop.

contractcourt: revamp HTLC success unit test

d02b486

We add checkpoint assertions and resume the resolver from every checkpoint to ensure it can handle restarts.

contractcourt: add TestHtlcSuccessSecondStageResolutionSweeper

aabba32

Test success resolvers going through the sweeper.

contractcourt/timeout_resolver: extract logic into spendHtlcOutput

2f33425

This commit moves the code doing the initial spend of the HTLC output of the commit tx into its own method.

contraccourt+input: create resolver for timeout second level

4992e41

In this commit we make the sweeper handle second level transactions for HTLC timeout resolvers for anchor channels.

contractcourt/htlc_timeout_test: expand timeout tests

bb406c8

rpctest: increase sweeper BatchWindow during itests

70eb526

Since we are checking HTLC aggregation, we must give the sweeper a bit more time to aggregate them to avoid flakes.

itest/local_chain_claim test: mine one less blocks for anchor sweeps

4b9fbe2

In case of anchor channel types, we mine one less block before we expect the second level sweep to appear in the mempool, since the sweeper sweeps one block earlier than the nursery.

itest: add HTLC aggregation test

1627310

halseth force-pushed the anchor-htlc-aggregation branch from db60ade to 1627310 Compare December 10, 2020 13:24

cfromknecht approved these changes Dec 11, 2020

View reviewed changes

cfromknecht merged commit 4af2415 into lightningnetwork:master Dec 12, 2020

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

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

Conversation

halseth commented Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

halseth Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

cfromknecht Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halseth commented Nov 23, 2020

cfromknecht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halseth commented Nov 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Nov 30, 2020

halseth commented Nov 30, 2020

Roasbeef commented Dec 1, 2020

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht left a comment

Choose a reason for hiding this comment

halseth commented Nov 18, 2020 •

edited

Loading

cfromknecht Nov 24, 2020 •

edited

Loading

halseth Nov 25, 2020 •

edited

Loading

cfromknecht Nov 25, 2020 •

edited

Loading