multi: configurable anchor commitment conf target #6025

ellemouton · 2021-11-24T09:43:15Z

In this commit, a new config option is added so that users can set the
confirmation target to be used for anchor commitments. The default value
is set to 6. Before this commit, a conf target of 3 was used.

cc @ziggie1984

ziggie1984

Good Job, thanks for putting me in cc :)

config.go

ziggie1984 · 2021-11-27T16:25:12Z

htlcswitch/link.go

@@ -1104,10 +1108,17 @@ func (l *channelLink) htlcManager() {
 				continue
 			}

+			// For normal channels, we use a conf target of 3 to
+			// determine the current network fee.
+			confTarget := uint32(3)


nit: would be cool to have also a constant for this variable like for example: defaultCommitmentConfTarget = 3? Why are we using 3, is it just a common agreement ?

good idea, I will move it out to a constant.
Yes i think using 3 was just the norm.

ellemouton · 2021-12-14T08:44:39Z

!lightninglabs-deploy mute 2022-Jan-01

carlaKC

Great PR, only minor comments!

carlaKC · 2022-01-18T08:11:57Z

htlcswitch/link.go

@@ -1104,10 +1113,17 @@ func (l *channelLink) htlcManager() {
 				continue
 			}

+			// For normal channels, we use a conf target of 3 to


nit: I wouldn't hardcode 3 here in case we bump defaultCommitmentConfTarget.

carlaKC · 2022-01-18T08:12:37Z

htlcswitch/link.go

+// chain in within the given confirmation target. The returned value is
+// expressed in fee-per-kw, as this is the native rate used when computing the
+// fee for commitment transactions, and the second-level HTLC transactions.
+func (l *channelLink) sampleNetworkFee(confTarget uint32) (chainfee.SatPerKWeight, error) {


wrap at 80 while we're here?

carlaKC · 2022-01-18T08:29:46Z

config.go

@@ -348,6 +352,8 @@ type Config struct {

 	MaxCommitFeeRateAnchors uint64 `long:"max-commit-fee-rate-anchors" description:"The maximum fee rate in sat/vbyte that will be used for commitments of channels of the anchors type. Must be large enough to ensure transaction propagation"`

+	AnchorsCommitConfTarget uint32 `long:"anchors-commit-conf-target" description:"The target number of blocks to be used to estimate the fee to use for anchor commitments"`


Difficult to express succinctly, but can we add a note clarifying that this is only for commitments/force closes? I can see end user confusion thinking that we'll use this for cooperative close, which is not the case?

carlaKC · 2022-01-18T08:31:25Z

htlcswitch/link.go

-func (l *channelLink) sampleNetworkFee() (chainfee.SatPerKWeight, error) {
-	// We'll first query for the sat/kw recommended to be confirmed within 3
-	// blocks.
-	feePerKw, err := l.cfg.FeeEstimator.EstimateFeePerKW(3)


Since anchors are the default channel type in lnd, we're effectively dropping our fee rates for all our commitments to conf target = 3 to 6 with this change. Are we good with that?

yeah good question. Perhaps we should leave it at 3 for now and let those who want to change it change it?
Just seems weird to use anchor channels and then still set a high conf target. But maybe it is best to leave it as is while we wait for package relay? thoughts @Roasbeef ?

we're effectively dropping our fee rates

~~Wouldn't we be increasing our fee rates bc faster conf time?~~

NVM, thought we were going from 6 -> 3

Thinking we should stick to 3 as default unless we have a reason to change. It's 3 for autopilot

yeah i'm happy to change it back. The thinking was just that it is kinda weird to have anchor commitment fees be so high since it kinda defeats the purpose since, no? or do we ideally not want to have to bump the fee? thoughts appreciated @Roasbeef & @carlaKC

In this commit, a new config option is added so that users can set the confirmation target to be used for anchor commitments. The default value is set to 6. Before this commit, a conf target of 3 was used.

ellemouton

Thanks for the review @carlaKC 🚀 updated as per suggestion. Good question re dropping from a 3 to 6 conf target for our default chan type, will defs get a second opinion re than

ellemouton · 2022-02-08T14:07:52Z

htlcswitch/link.go

-func (l *channelLink) sampleNetworkFee() (chainfee.SatPerKWeight, error) {
-	// We'll first query for the sat/kw recommended to be confirmed within 3
-	// blocks.
-	feePerKw, err := l.cfg.FeeEstimator.EstimateFeePerKW(3)


yeah good question. Perhaps we should leave it at 3 for now and let those who want to change it change it?
Just seems weird to use anchor channels and then still set a high conf target. But maybe it is best to leave it as is while we wait for package relay? thoughts @Roasbeef ?

arshbot · 2022-02-15T17:56:36Z

htlcswitch/link.go

-func (l *channelLink) sampleNetworkFee() (chainfee.SatPerKWeight, error) {
-	// We'll first query for the sat/kw recommended to be confirmed within 3
-	// blocks.
-	feePerKw, err := l.cfg.FeeEstimator.EstimateFeePerKW(3)


Thinking we should stick to 3 as default unless we have a reason to change. It's 3 for autopilot

arshbot · 2022-02-15T17:58:18Z

sample-lnd.conf

@@ -365,6 +365,10 @@
 ; propagation (default: 10)
 ; max-commit-fee-rate-anchors=5

+; The confirmation target to be used to estimate the fee to be used for anchor
+; commitment transactions. (default: 6)


maybe mention anchor commitment txs are default and modifying this setting will apply to all channels? users may avoid this setting because it looks scary and technical unless told otherwise

lightninglabs-deploy · 2022-03-21T18:31:32Z

@ellemouton, remember to re-request review from reviewers when ready

ellemouton · 2022-03-22T05:23:36Z

!lightninglabs-deploy mute 2022-April-04

Roasbeef · 2024-01-05T02:10:34Z

I think this is no longer needed (?) now that we'll not sweep anchors until we actually need to? Then once their swept, BumpFee can be used to increase the CPFP rate.

ellemouton · 2024-01-08T11:09:15Z

I think this is no longer needed (?) now that we'll not sweep anchors until we actually need to? Then once their swept, BumpFee can be used to increase the CPFP rate.

but doesnt that require a user to actively go and bump it then? This just allows them to set the default target if they want.

happy to close though - lemme know what you think

ziggie1984 · 2024-01-08T17:57:35Z

So I think it makes sense to distinguish between anchor and non-anchor channels but I probably think we should just make sure we always use the min_relay_fee for anchors, but not only ours but taking the ones of our peers into account as well (relates to #8302)

ziggie1984 reviewed Nov 27, 2021

View reviewed changes

ellemouton force-pushed the confirgurableAnchorConfTarget branch from 64a005d to 3f2a64a Compare December 14, 2021 08:43

carlaKC self-requested a review January 11, 2022 08:38

carlaKC reviewed Jan 18, 2022

View reviewed changes

Roasbeef added this to the v0.15.0 milestone Jan 21, 2022

Roasbeef added commitments Commitment transactions containing the state of the channel config Parameters/arguments/config file related issues/PRs labels Jan 21, 2022

Roasbeef added this to In progress in v0.15.0-beta via automation Feb 2, 2022

Roasbeef moved this from In progress to Blocked / Chopping Block / Up For Adoption in v0.15.0-beta Feb 2, 2022

Roasbeef mentioned this pull request Feb 7, 2022

Lost anchor: stuck sweep transaction (unconfirmed balance) #6241

Closed

multi: configurable anchor commitment conf target

ff17f45

In this commit, a new config option is added so that users can set the confirmation target to be used for anchor commitments. The default value is set to 6. Before this commit, a conf target of 3 was used.

ellemouton force-pushed the confirgurableAnchorConfTarget branch from 3f2a64a to ff17f45 Compare February 8, 2022 14:08

ellemouton commented Feb 8, 2022

View reviewed changes

Roasbeef requested a review from arshbot February 14, 2022 18:31

Roasbeef added the P3 might get fixed, nice to have label Feb 14, 2022

arshbot reviewed Feb 15, 2022

View reviewed changes

Roasbeef removed this from Blocked / Chopping Block / Up For Adoption in v0.15.0-beta Mar 7, 2022

Roasbeef modified the milestones: v0.15.0, v0.16.0 Mar 7, 2022

Roasbeef modified the milestones: v0.16.0, v0.17.0 Aug 23, 2022

saubyk removed this from the Low Priority milestone Aug 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: configurable anchor commitment conf target #6025

multi: configurable anchor commitment conf target #6025

ellemouton commented Nov 24, 2021

ziggie1984 left a comment

ziggie1984 Nov 27, 2021

ellemouton Dec 14, 2021

ellemouton commented Dec 14, 2021

carlaKC left a comment

carlaKC Jan 18, 2022

carlaKC Jan 18, 2022

carlaKC Jan 18, 2022

carlaKC Jan 18, 2022

ellemouton Feb 8, 2022

arshbot Feb 15, 2022 •

edited

arshbot Feb 15, 2022

ellemouton Feb 28, 2022

ellemouton left a comment

ellemouton Feb 8, 2022

arshbot Feb 15, 2022

arshbot Feb 15, 2022

lightninglabs-deploy commented Mar 21, 2022

ellemouton commented Mar 22, 2022

Roasbeef commented Jan 5, 2024

ellemouton commented Jan 8, 2024

ziggie1984 commented Jan 8, 2024

		@@ -348,6 +352,8 @@ type Config struct {

		MaxCommitFeeRateAnchors uint64 `long:"max-commit-fee-rate-anchors" description:"The maximum fee rate in sat/vbyte that will be used for commitments of channels of the anchors type. Must be large enough to ensure transaction propagation"`

		AnchorsCommitConfTarget uint32 `long:"anchors-commit-conf-target" description:"The target number of blocks to be used to estimate the fee to use for anchor commitments"`

multi: configurable anchor commitment conf target #6025

Are you sure you want to change the base?

multi: configurable anchor commitment conf target #6025

Conversation

ellemouton commented Nov 24, 2021

ziggie1984 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton commented Dec 14, 2021

carlaKC 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

arshbot Feb 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton 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

lightninglabs-deploy commented Mar 21, 2022

ellemouton commented Mar 22, 2022

Roasbeef commented Jan 5, 2024

ellemouton commented Jan 8, 2024

ziggie1984 commented Jan 8, 2024

arshbot Feb 15, 2022 •

edited