cnct+sweep: cpfp-aware anchor sweeping #4592

joostjager · 2020-09-04T11:05:10Z

This PR modifies the sweeper to take into account potential unconfirmed parent transactions. This enables tracking of the current fee estimate for anchors of not yet confirmed commitment transactions. Previously an anchor-commitment that wouldn't confirm needed to be bumped manually.

2020-09-08 11:20:19.860 [INF] SWPR: Creating sweep transaction 1b76f06cd667c4d611ce9bd397634e97ac5539e960507c5ad0202965dcae481d for 2 inputs (7d1d2012a7ba95cd8140c47f4688ae0114ae9a88ebfadda107408fd937cfa92b:0 (CommitmentAnchor), ab682225912097d789c33b06b14d1d730869a3869e9b558537085ab2dcef2250:1 (WitnessKeyHash)) using 20000 sat/kw, tx_weight=719, tx_fee=0.0002546 BTC, parents_count=1, parents_fee=0.0001124 BTC, parents_weight=1116

halseth · 2020-09-08T09:37:55Z

input/input.go

@@ -41,6 +42,18 @@ type Input interface {
 	// HeightHint returns the minimum height at which a confirmed spending
 	// tx can occur.
 	HeightHint() uint32
+
+	// Cpfp returns information about a possibly unconfirmed parent tx.
+	Cpfp() *TxInfo


call this ParentFeeInfo or something? I found having it named CPFP a bit confusing.

I had that first, but the reason I renamed it is that it isn't just the parent info, but also signals specific cpfp behavior. Like when the input is included in a sweep.

Renamed to UnconfParent(). The comments already says that that is what it is.

halseth · 2020-09-08T09:42:22Z

sweep/sweeper.go

+					chainfee.SatPerKWeight(parentTx.Weight)
+
+			if parentFeeRate >= feeRate {
+				log.Debugf("Skipping cpfp input %v: fee_rate=%v, "+


cfromknecht

Great commit structure 😊 On first pass the changes read well. Can do another pass when the TODOs are completed (or are they?)

sweep/weight_estimator.go

cfromknecht · 2020-09-16T02:11:51Z

sweep/weight_estimator.go

+	weight += w.parentsWeight
+
+	// Subtract fee already paid by parents.
+	fee := w.feeRate.FeeForWeight(weight) - w.parentsFee


it seems possible that this could be negative? or is the theory that this should never do that because of the parentFeeRate >= w.feeRate check? i wonder if that is always true when considering rounding edge cases

I don't think there is a rounding edge case, because even in the case that the parent almost totally pays for itself, the child tx has a minimum weight too that needs to be covered.

An alternative would be to remove the parentFeeRate >= w.feeRate check, but here instead return the maximum of feeRate.FeeForWeight(weight) and feeRate.FeeForWeight(weight+parentsWeight)-parentsFee

even in the case that the parent almost totally pays for itself, the child tx has a minimum weight too that needs to be covered.

the minimum weight of a transaction is less than the divisor (1000), so it still seems possible to me. would be nice to know for sure tho

An alternative would be to remove the parentFeeRate >= w.feeRate check, but here instead return the maximum of feeRate.FeeForWeight(weight) and feeRate.FeeForWeight(weight+parentsWeight)-parentsFee

I don't think that is possible, because there may be multiple inputs with different parents, some of which can be cpfp'ed and others not.

No, you are right. The floor should be what is required for just the child tx. Added that.

sweep/weight_estimator.go

joostjager · 2020-09-16T06:15:34Z

@cfromknecht Yes, the todos are completed. I forgot to update the PR description.

Preparation for a cpfp-aware weight estimator. For cpfp, a regular weight estimator isn't sufficient, because it needs to take into account the weight of the parent transaction(s) as well.

halseth

Nice PR, mostly LGTM 🤠

halseth · 2020-09-16T11:17:29Z

sweep/weight_estimator.go

+	weight += w.parentsWeight
+
+	// Subtract fee already paid by parents.
+	fee := w.feeRate.FeeForWeight(weight) - w.parentsFee


An alternative would be to remove the parentFeeRate >= w.feeRate check, but here instead return the maximum of feeRate.FeeForWeight(weight) and feeRate.FeeForWeight(weight+parentsWeight)-parentsFee

lntest/itest/lnd_test.go

Roasbeef · 2020-09-16T23:47:19Z

contractcourt/channel_arbitrator.go

+const (
+	// anchorSweepConfTarget is the conf target used when sweeping
+	// commitment anchors.
+	anchorSweepConfTarget = 6


Drive by comment, but perhaps this shouldn't be a constant, and instead should be some factor of the CLTV delay with any incoming HTLCs (if they exist), otherwise falling back to a distinct value.

I'd be worried that if we have plenty of blocks left, we would pick a high conf target. Then if we get closer to the deadline, we don't change our conf target and might miss it. I think it is safer to always use 6 blocks until we have a sweeper that is aware of the deadline. Also the signed commit tx is estimated with a 3 block target, so fees need to go up quite a bit first before the 6 block target sweep can cpfp. (Although I sometimes see that bitcoind estimatesmartfee gives me the same estimate for all targets <= 6).

Extend the fee estimator to take into account parent transactions with their weights and fees. Do not try to cpfp parent transactions that have a higher fee rate than the sweep tx fee rate.

For unconfirmed commit tx anchors, supply the sweeper with cpfp info and a confirmation target fee estimate. The sweeper will try to pay for the parent commit tx as long as the current fee estimate exceeds the pre-signed commit tx fee rate.

cfromknecht

LGTM!! 💪

joostjager changed the title ~~Sweep deadline~~ cnct: cpfp-aware anchor sweeping Sep 4, 2020

joostjager changed the title ~~cnct: cpfp-aware anchor sweeping~~ cnct+sweep: cpfp-aware anchor sweeping Sep 4, 2020

joostjager force-pushed the sweep-deadline branch 15 times, most recently from 7be0b7e to 442b77d Compare September 8, 2020 09:21

joostjager requested a review from halseth September 8, 2020 09:23

halseth reviewed Sep 8, 2020

View reviewed changes

joostjager force-pushed the sweep-deadline branch 5 times, most recently from f237fb1 to 2a8b7ef Compare September 11, 2020 06:30

joostjager mentioned this pull request Sep 11, 2020

cnct+sweep+itest: preparations for cpfp-aware sweeper #4606

Merged

joostjager force-pushed the sweep-deadline branch 4 times, most recently from 73dcfe3 to d7cf7f6 Compare September 14, 2020 11:46

joostjager force-pushed the sweep-deadline branch 7 times, most recently from 7b897a1 to 7a2f8a3 Compare September 15, 2020 08:40

joostjager marked this pull request as ready for review September 15, 2020 09:04

joostjager requested review from cfromknecht and Roasbeef as code owners September 15, 2020 09:04

joostjager requested review from halseth and removed request for Roasbeef September 15, 2020 09:04

cfromknecht reviewed Sep 16, 2020

View reviewed changes

sweep/weight_estimator.go Outdated Show resolved Hide resolved

joostjager force-pushed the sweep-deadline branch 2 times, most recently from c02727f to b8d2b91 Compare September 16, 2020 06:14

joostjager requested a review from cfromknecht September 16, 2020 06:14

joostjager force-pushed the sweep-deadline branch from b8d2b91 to 3c1bf6b Compare September 16, 2020 06:17

sweep: wrap weight estimator

3e3d848

Preparation for a cpfp-aware weight estimator. For cpfp, a regular weight estimator isn't sufficient, because it needs to take into account the weight of the parent transaction(s) as well.

joostjager force-pushed the sweep-deadline branch 2 times, most recently from 6e96ef8 to 8a33ed5 Compare September 16, 2020 10:17

halseth approved these changes Sep 16, 2020

View reviewed changes

Roasbeef reviewed Sep 16, 2020

View reviewed changes

joostjager added 2 commits September 17, 2020 12:30

sweep: make sweeper aware of unconfirmed parent transactions.

681496b

Extend the fee estimator to take into account parent transactions with their weights and fees. Do not try to cpfp parent transactions that have a higher fee rate than the sweep tx fee rate.

cnct: cpfp-sweep anchors

29602e8

For unconfirmed commit tx anchors, supply the sweeper with cpfp info and a confirmation target fee estimate. The sweeper will try to pay for the parent commit tx as long as the current fee estimate exceeds the pre-signed commit tx fee rate.

joostjager force-pushed the sweep-deadline branch from 8a33ed5 to 29602e8 Compare September 17, 2020 10:30

joostjager added this to the 0.12.0 milestone Sep 17, 2020

cfromknecht approved these changes Sep 18, 2020

View reviewed changes

joostjager merged commit 6ae05c6 into lightningnetwork:master Sep 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cnct+sweep: cpfp-aware anchor sweeping #4592

cnct+sweep: cpfp-aware anchor sweeping #4592

joostjager commented Sep 4, 2020 •

edited

Loading

halseth Sep 8, 2020

joostjager Sep 8, 2020

joostjager Sep 14, 2020

halseth Sep 8, 2020

cfromknecht left a comment

cfromknecht Sep 16, 2020

joostjager Sep 16, 2020

halseth Sep 16, 2020

cfromknecht Sep 16, 2020 •

edited

Loading

joostjager Sep 16, 2020

joostjager Sep 17, 2020

joostjager commented Sep 16, 2020

halseth left a comment

halseth Sep 16, 2020

Roasbeef Sep 16, 2020

joostjager Sep 17, 2020

cfromknecht left a comment

cnct+sweep: cpfp-aware anchor sweeping #4592

cnct+sweep: cpfp-aware anchor sweeping #4592

Conversation

joostjager commented Sep 4, 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

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

cfromknecht Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager commented Sep 16, 2020

halseth 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

cfromknecht left a comment

Choose a reason for hiding this comment

joostjager commented Sep 4, 2020 •

edited

Loading

cfromknecht Sep 16, 2020 •

edited

Loading