chancloser+lnwire: add range-based negotiation #7062

Crypt-iQ · 2022-10-19T20:52:33Z

This PR adds FeeRange to ClosingSigned and makes lnd spec-compliant wrt FeeRange. Last 3 commits are new.

This patch also introduces the first place we'll send out a warning in lnd per the spec:

- if the message contains a fee_range:
  - if there is no overlap between that and its own fee_range:
    - SHOULD send a warning

Handling the warning is left for a future PR, since it is unclear what the funder should do as the FeeRange is already set and shouldn't change.

needs tests
depends on Fix warning messages #6840
depends on multi: implement spec-compliant coop close #6760
probably should wait for bolt2: fee_range clarification lightning/bolts#1013
testing with other impls

Fixes #4413

If upfront shutdown is not used, the delivery script would previously not be persisted, leading to a spec violation on retransmitting Shutdown.

This is accomplished by: * passing Shutdown messages to the link when appropriate * having the link tell peer.Brontide when closing_signed can be sent * handling retransmission of Shutdown * persisting the delivery scripts for retransmission * reworking the ChanCloser such that closing_signed isn't immediately sent after sending and receiving Shutdown

Roasbeef

Thanks for picking this up! The initial set of changes looks pretty good, though I have some questions w.r.t if we can segregate some of the new logic a bit more cleanly.

One other recurring thread that pops into my mind is: users seem to want their co-op close to never fail. This is achievable in practice, if the responder simpler acquiesce to w/e the initiator wants, given that they pay the fees in the first place. The only edge case here is if the initiator's fees are so high to the point that your own output gets dusted.

Roasbeef · 2022-10-28T23:47:10Z

lnwallet/chancloser/chancloser.go

@@ -375,6 +375,17 @@ func (c *ChanCloser) initChanShutdown() (*lnwire.Shutdown, error) {
 			c.chanPoint, err)
 	}

+	// Persist the delivery script used before marking the state as coop
+	// broadcasted so we can recover in case of a crash.
+	if len(c.Channel().LocalUpfrontShutdownScript()) == 0 {


How about we also commit the fee range at this point as well? This way we can deterministically resume if the connection dies for w/e reason.

Roasbeef · 2022-10-28T23:55:57Z

htlcswitch/link.go

+
+	// shutdownInit is a bool that is set when we've initiated a coop
+	// close.
+	shutdownInit bool


How about we encapsulate this new state into a smaller struct, something along the lines of shutdownContext or w/e? This way we can unit test it individually, and it may be a bit easier to review it in an isolated location vs new logic in the main event loop of the link.

Roasbeef · 2022-10-28T23:56:31Z

htlcswitch/link.go

+			)
+
+			// Flip the shutdownInit and shutdownSent bools.
+			l.shutdownInit = true


Doesn't need to be mutex protected?

Roasbeef · 2022-10-28T23:57:30Z

htlcswitch/link.go

@@ -1150,6 +1228,132 @@ func (l *channelLink) htlcManager() {
 				"PendingLocalUpdateCount")
 		}

+		// If we are attempting to shutdown the link to cooperatively


I think this should be extracted to a new method, also see above re further encapsulating all this logic in a new struct. This main loop is long enough already imo....

Roasbeef · 2022-10-28T23:59:02Z

htlcswitch/link.go

+				if err != nil {
+					l.fail(LinkFailureError{
+						code: ErrInternalError,
+					}, "failed persisting script: %v", err)


Should this report back to the caller (which now seemingly has more context on what happens here) that the attempt failed? Otherwise, iiuc, this'll just hang on RPC/CLI.

Roasbeef · 2022-10-29T00:11:11Z

lnwire/closing_signed.go

+
+// GetOverlap takes two FeeRanges and returns the overlapping FeeRange between
+// the two. If there is no overlap, nil is returned.
+func (f *FeeRange) GetOverlap(other *FeeRange) *FeeRange {


How about an error instead to make the error case more explicit?

Roasbeef · 2022-10-29T00:12:45Z

lnwire/closing_signed.go

+	// stream.
+	if val, ok := typeMap[FeeRangeType]; ok && val == nil {
+		// Check that the fee range is sane before setting it.
+		if fr.MinFeeSats > fr.MaxFeeSats {


Hmm, I think we might want to handle this check at a higher layer, particularly given that a concrete error isn't returned here, so the readHandler in the peer can't distinguish from violated constraints vs a garbled message (unparseable).

IMO this check belongs here, it checks that the message is sane. We'd need a new function to check sanity otherwise, which accomplishes the same thing but requires another call in the chancloser

why not have the check in the case closeFeeNegotiation: case in ProcessCloseMsg function where we also do the validation of other close messages? Tend to agree that this Decode message should only decode & not validate the meanings of the fields.

Roasbeef · 2022-10-29T00:14:26Z

lnwallet/chancloser/chancloser.go

+	isFunder := c.cfg.Channel.IsInitiator()
+
+	// If FeeRange is set, perform FeeRange-specific checks.
+	if remoteMsg.FeeRange != nil {


I thought fee range was actually just in shutdown?

it's in closing_signed

Roasbeef · 2022-10-29T00:15:46Z

lnwallet/chancloser/chancloser.go

+			//	 between received and (about-to-be) sent
+			//       fee_range.
+			if overlap == nil {
+				return nil, ErrNoRangeOverlap


What if instead we just have the funder send their desired fee rate? Based on inbound issues, people seem to want a trait where their co-op close actually never fails, instead deferring to w/e the funder wanted since they're the one paying for fees.

This is a spec-level check, if the fundee does not send an overlapping fee-range, we fail

Roasbeef · 2022-10-29T00:16:31Z

lnwallet/chancloser/chancloser.go

+			warning.ChanID = remoteMsg.ChannelID
+			warning.Data = lnwire.WarningData("ClosingSigned: no " +
+				"fee_range overlap")
+			return warning, nil


Similar here re the responder just accepting w/e the funder wants in the case of no overlap.

That's possible

ellemouton

Left a few comments on the last 3 commits (since the commits before are covered by a different PR).
Main question is about why the funder just drops the ClosingSigned message that they construct

ellemouton · 2022-11-03T08:18:22Z

lnwire/closing_signed.go

+	// stream.
+	if val, ok := typeMap[FeeRangeType]; ok && val == nil {
+		// Check that the fee range is sane before setting it.
+		if fr.MinFeeSats > fr.MaxFeeSats {


why not have the check in the case closeFeeNegotiation: case in ProcessCloseMsg function where we also do the validation of other close messages? Tend to agree that this Decode message should only decode & not validate the meanings of the fields.

ellemouton · 2022-11-03T08:27:26Z

lnwallet/chancloser/chancloser.go

+// - a message and no error (continue negotiation)
+// - no message and an error (negotiation failed)
+// - no message and no error (indicating we agree with the remote's fee)
+func (c *ChanCloser) handleRemoteProposal(


nit: might be worth splitting into handleRemoteProposalLegacy and handlRemoteProposalFeeRange or something just to avoid the huge indented code in an if-block

ellemouton · 2022-11-03T08:38:02Z

lnwallet/chancloser/chancloser.go

+		// Get the intersection of our two FeeRanges if one exists.
+		overlap := c.idealFeeRange.GetOverlap(remoteMsg.FeeRange)
+
+		if isFunder {


excellent documentation in this method 💪

ellemouton · 2022-11-03T08:40:23Z

lnwallet/chancloser/chancloser.go

+			//   - ...
+			//   - otherwise:
+			//     - MUST reply with the same fee_satoshis.
+			_, err := c.proposeCloseSigned(remoteMsg.FeeSatoshis)


here we just construct a new ClosingSigned message but dont do anything with it? Should we not return the new message so that we send it to the peer?

if we are agreeing proposeCloseSigned will add the fee to a map. then the calling function will check the map and send the closing signed. confusing but the logic is pre-existing

ellemouton · 2023-07-28T16:05:14Z

Adding this comment so the bot sees it as active

Crypt-iQ · 2023-07-28T16:07:50Z

comment

lightninglabs-deploy · 2024-01-23T23:20:07Z

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

lightninglabs-deploy · 2024-01-24T16:11:27Z

Closing due to inactivity

lightninglabs-deploy · 2024-01-24T17:14:03Z

Closing due to inactivity

lightninglabs-deploy · 2024-01-24T19:19:41Z

Closing due to inactivity

lightninglabs-deploy · 2024-01-24T20:23:06Z

Closing due to inactivity

Crypt-iQ added channel closing Related to the closing of channels cooperatively and uncooperatively fees Related to the fees paid for transactions (both LN and funding/commitment transactions) spec labels Oct 19, 2022

Crypt-iQ mentioned this pull request Oct 19, 2022

lnwire+chancloser: add new fee range TLV to co-op msg & use during negotiation #5644

Closed

Crypt-iQ added 5 commits October 27, 2022 19:10

multi: persist delivery script during coop close

0ee7f19

If upfront shutdown is not used, the delivery script would previously not be persisted, leading to a spec violation on retransmitting Shutdown.

channeldb: persist shutdown info with channel, add helper funcs

8958ba4

optional itest: coop close integration test

e25691b

lnwire: add FeeRange TLV to ClosingSigned

4a18d3f

Crypt-iQ force-pushed the coop_close_970_with_range branch from e6361bd to 015d219 Compare October 27, 2022 23:14

Crypt-iQ added 2 commits October 27, 2022 19:34

chancloser: add FeeRange logic

16e024d

chancloser: add tests

fa21e78

Crypt-iQ force-pushed the coop_close_970_with_range branch from 015d219 to fa21e78 Compare October 27, 2022 23:34

Roasbeef mentioned this pull request Oct 28, 2022

[bug]: unable to retry coop close attempt with higher fee rate (between two nodes running v0.15.3) #7091

Open

Roasbeef requested changes Oct 29, 2022

View reviewed changes

Roasbeef requested a review from ellemouton October 29, 2022 00:18

ellemouton reviewed Nov 3, 2022

View reviewed changes

ellemouton closed this Jul 28, 2023

Crypt-iQ reopened this Jul 28, 2023

Crypt-iQ closed this Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chancloser+lnwire: add range-based negotiation #7062

chancloser+lnwire: add range-based negotiation #7062

Crypt-iQ commented Oct 19, 2022 •

edited

Loading

Roasbeef left a comment

Roasbeef Oct 28, 2022

Roasbeef Oct 28, 2022

Roasbeef Oct 28, 2022

Roasbeef Oct 28, 2022

Roasbeef Oct 28, 2022

Roasbeef Oct 29, 2022

Roasbeef Oct 29, 2022

Roasbeef Oct 29, 2022

Crypt-iQ Oct 31, 2022

ellemouton Nov 3, 2022

Roasbeef Oct 29, 2022

Crypt-iQ Oct 31, 2022

Roasbeef Oct 29, 2022

Crypt-iQ Oct 31, 2022

Roasbeef Oct 29, 2022

Crypt-iQ Oct 31, 2022

ellemouton left a comment

ellemouton Nov 3, 2022

ellemouton Nov 3, 2022

ellemouton Nov 3, 2022

ellemouton Nov 3, 2022

Crypt-iQ Nov 3, 2022

ellemouton commented Jul 28, 2023

Crypt-iQ commented Jul 28, 2023

lightninglabs-deploy commented Jan 23, 2024

lightninglabs-deploy commented Jan 24, 2024

lightninglabs-deploy commented Jan 24, 2024

lightninglabs-deploy commented Jan 24, 2024

lightninglabs-deploy commented Jan 24, 2024

chancloser+lnwire: add range-based negotiation #7062

chancloser+lnwire: add range-based negotiation #7062

Conversation

Crypt-iQ commented Oct 19, 2022 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton commented Jul 28, 2023

Crypt-iQ commented Jul 28, 2023

lightninglabs-deploy commented Jan 23, 2024

lightninglabs-deploy commented Jan 24, 2024

lightninglabs-deploy commented Jan 24, 2024

lightninglabs-deploy commented Jan 24, 2024

lightninglabs-deploy commented Jan 24, 2024

Crypt-iQ commented Oct 19, 2022 •

edited

Loading