lnpeer: modern closing negotiation #7586

bitromortac · 2021-12-06T16:10:40Z

Implements a recent spec change concerning channel closing lightning/bolts#847.
The new fee negotiation is based on fee ranges but is also backwards compatible to the incremental closing negotiation.
For testing purposes there's a configuration constant MODERN_FEE to trigger the new behavior, which is on by default.
I also added a new regtest test for collaborative channel closing.

Todo:

Clarify what lower bound we should send.
Test compatibility with Eclair (new negotiation)
Test compatibility with LND (old negotiation)

Future:

Once anchor commitments are in, we should get rid of some constraints (marked with TODOs) like the limiting fees to the current ctx fees, as they can be set lower than the mempool tip.

ecdsa · 2022-02-17T16:15:26Z

electrum/lnpeer.py

+            except asyncio.exceptions.TimeoutError:
+                if not is_initiator and not their_fee:  # we only force close if a peer doesn't reply
+                    await self.lnworker.force_close_channel(chan.channel_id)
+                    raise Exception("Peer didn't reply with closing signed, force closed.")


Note: the BOLT does not say that we have to force close.
I suppose this choice follows from https://github.com/lightning/bolts/pull/847/files#r689977518

Note 2: _shutdown is called from two separate places, one of them is is a Taskgroup.
In that case, _shutdown could be cancelled if the remote closes the connection, and might not have time to force close.

This way of calling a task from non-homogeneous contexts seems dangerous. This comment is not about this specific pull request, but about how we expect code to be executed. A similar issue was encountered in request_force_close_from_backup

I see, but I also don't recall why I added the force close here, what would you suggest?

Sorry, I had not seen the new definition of "fail the channel". Indeed, we have to force close.
Maybe this should be using try_force_closing, as in #7645.

- MUST fail the channel translates to force close the channel (lightning/bolts@eb6f308)

Sorry, I had not seen the new definition of "fail the channel". Indeed, we have to force close. Maybe this should be using try_force_closing, as in #7645.

Saw your comment too late. We could wait for #7645 and then use self.try_force_closing, there are probably also some minor conflicts with this PR.

we don't need to wait for #7645. See the new API, schedule_force_closing in 556b987

Also, the BOLT does not make a distinction between funder and non-funder. I think we have to force close in both cases.

we don't need to wait for #7645. See the new API, schedule_force_closing in 556b987

I'll use it, but I meant that we introduce a wrapper lnpeer.schedule_force_closing that is added and could be used here as well.

Also, the BOLT does not make a distinction between funder and non-funder. I think we have to force close in both cases.

My motivation was to catch the case where we would have sent an initial closing_signed message (as the initiator) and wait for the first response of the peer, if it doesn't, force close. I haven't tackled the case when the peer doesn't reply during a general step of the negotiation, of course we could close there as well.

ecdsa · 2022-02-18T13:26:57Z

Does the regtest test bring something more than the already existing unit tests?

electrum/lnpeer.py

ecdsa · 2022-02-20T11:31:26Z

Clarify what lower bound we should send. We send max_fee // 2 right now.

That is not what this PR is doing, it is sending the interval [our_fee//2, our_fee], where our_fee results from user preferences, and is capped by max_fee. max_fee is the fee from the commitment tx.

ecdsa · 2022-02-20T11:51:11Z

I think that our current use of user preferences in order to set fee_rate should have been complemented with displaying a fee slider in the "close channel" dialog. Otherwise, it might be better to simply use the ETA, as we do with commitment transactions.

This slider approach may or may not be used with the modern interval, but if we do not use it then we should not use feerates derived from user preferences.

electrum/lnpeer.py

ecdsa · 2022-02-21T11:30:00Z

Regarding the choice of values, I think we could use the same fee rate as in the latest commitment (that is, until anchor outputs are supported).
our_fee would be lower than max_fee because the close tx is smaller than the commitment tx.
For the interval, I guess we could use [our_fee//2, our_fee*2]
Later we can propose a fee slider for our_fee with that range.

bitromortac · 2022-02-21T14:57:51Z

Does the regtest test bring something more than the already existing unit tests?

I think I was able to catch a bug by adding it, but we could remove it.

Clarify what lower bound we should send. We send max_fee // 2 right now.

That is not what this PR is doing, it is sending the interval [our_fee//2, our_fee], where our_fee results from user preferences, and is capped by max_fee. max_fee is the fee from the commitment tx.

That may be an old comment for the specific value, it was just referring to the range definition problem.

Regarding the choice of values, I think we could use the same fee rate as in the latest commitment (that is, until anchor outputs are supported).
our_fee would be lower than max_fee because the close tx is smaller than the commitment tx.
For the interval, I guess we could use [our_fee//2, our_fee*2]
Later we can propose a fee slider for our_fee with that range.

I switched to ETA of 2 blocks, the fee interval seems reasonable, but I have to think about this again, as already some time has passed since the PR.

ecdsa · 2022-02-22T10:25:09Z

electrum/lnpeer.py

+            # if fee_satoshis is equal to its previously sent fee_satoshis:
+            if our_fee == their_fee:
+                # SHOULD sign and broadcast the final closing transaction.
+                break  # we publish


The code on master calls send_closing_signed once before the while loop if we are the initiator, and once after the while loop if we are not the initiator. This ensures that the remote can publish the closing transaction. You seem to have replaced that with various calls inside the while loop. However, you do not call it everywhere. For example here, we break from the loop and no message will be sent. This creates a situation where the remote will try to force-close, because they do not receive our closing_signed. This correspond to what is described in the FIXME "the remote SHOULD send closing_signed, but some don't. ")

I see that there could be a problem, but in this case I interpreted it that way that the peer would know that we have agreement and that we would publish (that's why we have the else clause in the timeout scenario). It seems like there's no instruction in the spec that we should reply again with closing_signed. We could send it for safety reasons.

ecdsa · 2022-02-22T14:11:14Z

I will try to rewrite the fee negotiation, I think it needs to be broken down into smaller functions

Updates the closing fee negotiation to comply with most recent spec changes, see lightning/bolts#847 The closing negotiation is backwards compatible with the old negotiation.

bitromortac · 2022-02-22T15:19:37Z

I will try to rewrite the fee negotiation, I think it needs to be broken down into smaller functions

Ok sure, the spec is kind of complicated and I tried to follow it as close as possible. I have even simulated the closing negotiation asynchronously with different parameters.

ecdsa · 2022-02-23T08:27:13Z

electrum/lnpeer.py

+                # MUST propose a value "strictly between" the received fee_satoshis and its previously-sent fee_satoshis.
+                our_fee_proposed = (our_fee + their_fee) // 2
+                if not (min(our_fee, their_fee) < our_fee_proposed < max(our_fee, their_fee)):
+                    our_fee_proposed += (their_fee - our_fee) // 2


I do not understand what this code is doing.
First, I suppose that the intent of this line is to set our_fee, not our_fee_proposed.
So, maybe you wanted to write something like our_fee = our_fee_proposed + (their_fee - our_fee)//2
Second, if abs(our_fee - their_fee) > 1, then (our_fee + their_fee) // 2 will always be strictly between the values, as explained in the comment on line 1892. Therefore, that if..else block seems unnecessary.

yes, you're right, I think I experienced an off-by-one somewhere here and tried to fix it that way, it should be removed

ecdsa · 2022-02-23T08:45:15Z

electrum/lnpeer.py

+                    break
+                # SHOULD use `fee_satoshis` to sign and broadcast the final closing transaction
+                else:
+                    our_fee = their_fee


this assignment is not needed.
also, if our_fee == their_fee, we break from the loop on line 1931, so I guess this branch is never executed.

that's true, I was translating the spec proverbially (SHOULD use fee_satoshis)

bitromortac · 2022-02-23T15:22:55Z

I added a commit which may help with review to simulate different scenarios. I think it would be useful if we could design tests like that.

ecdsa · 2022-03-07T09:43:12Z

closing in favor of #7680

bitromortac force-pushed the 2111-modern-closing-fee branch from e86198d to 0d27785 Compare December 8, 2021 15:49

bitromortac changed the title ~~[wip] lnpeer: modern closing negotiation~~ lnpeer: modern closing negotiation Jan 20, 2022

ecdsa reviewed Feb 17, 2022

View reviewed changes

ecdsa reviewed Feb 20, 2022

View reviewed changes

electrum/lnpeer.py Outdated Show resolved Hide resolved

ecdsa reviewed Feb 20, 2022

View reviewed changes

electrum/lnpeer.py Outdated Show resolved Hide resolved

bitromortac force-pushed the 2111-modern-closing-fee branch from 0d27785 to da96c15 Compare February 21, 2022 14:57

bitromortac force-pushed the 2111-modern-closing-fee branch from da96c15 to 3c76c0e Compare February 21, 2022 15:20

ecdsa reviewed Feb 22, 2022

View reviewed changes

bitromortac added 2 commits February 22, 2022 15:40

lnwire: add fee_range tlv

ee040ea

lnpeer: modern fee negotiation

ccd1f4b

Updates the closing fee negotiation to comply with most recent spec changes, see lightning/bolts#847 The closing negotiation is backwards compatible with the old negotiation.

bitromortac force-pushed the 2111-modern-closing-fee branch from 3c76c0e to ccd1f4b Compare February 22, 2022 15:10

ecdsa reviewed Feb 23, 2022

View reviewed changes

review: simulate closing

d4cab30

fixup! lnpeer: modern fee negotiation

4653c56

ecdsa mentioned this pull request Feb 23, 2022

Modern fee negotiation #7680

Merged

ecdsa closed this Mar 7, 2022

lnpeer: modern closing negotiation #7586

lnpeer: modern closing negotiation #7586

Uh oh!

Conversation

bitromortac commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecdsa commented Feb 18, 2022

Uh oh!

Uh oh!

ecdsa commented Feb 20, 2022

Uh oh!

ecdsa commented Feb 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ecdsa commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bitromortac commented Feb 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecdsa commented Feb 22, 2022

Uh oh!

bitromortac commented Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ecdsa Feb 23, 2022 • 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!

bitromortac Feb 23, 2022 • 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!

ecdsa Feb 23, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

bitromortac commented Feb 23, 2022

Uh oh!

ecdsa commented Mar 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bitromortac commented Dec 6, 2021 •

edited

Loading

ecdsa commented Feb 20, 2022 •

edited

Loading

ecdsa commented Feb 21, 2022 •

edited

Loading

bitromortac commented Feb 22, 2022 •

edited

Loading

ecdsa Feb 23, 2022 •

edited

Loading

bitromortac Feb 23, 2022 •

edited

Loading

ecdsa Feb 23, 2022 •

edited

Loading