Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zzz-extension-bolt: dynamic commitments #1026

Closed
wants to merge 1 commit into from

Conversation

Crypt-iQ
Copy link
Contributor

This commit introduces three messages dyn_begin_propose, dyn_propose, and dyn_propose_reply which allow both channel participants to change their static channel parameters. Initially, only changing the dust limit and the CSV delay are supported.

I made a separate document since it's easier to see the changeset, but I could also cram it into bolt 2.

NOTE: the design here is separate from what was laid out in the mailing list post (https://lists.linuxfoundation.org/pipermail/lightning-dev/2022-March/003531.html) since the commitment format isn't being upgraded here. Only static channel parameters are being updated, so perhaps a different name could be used.

This commit introduces three messages `dyn_begin_propose`,
`dyn_propose`, and `dyn_propose_reply` which allow both channel
participants to change their static channel parameters. Initially,
only changing the dust limit and the CSV delay are supported.
Three new messages are introduced that let each side propose what they want to
change about the channel.

### `dyn_begin_propose`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to re-use #869 here? At a glance it can probably be quite trivially expanded to include some of the functionality being used here:

  1. Add a reject field to the message to stfu message
  2. (optionally) Add a message_type tlv that indicates why you're pausing the channel (eg, in this example message_type=115/dyn_propose)

Copy link
Contributor Author

@Crypt-iQ Crypt-iQ Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it this way because 869 allows HTLC's on the commit tx, and I didn't want to have to think about a new dust limit trimming HTLCs -- but a general flow like 869 w/o htlc's could be useful here and probably other future flows

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So i think this is the way people are leaning and it makes sense, but we need to figure out the warning message bit where it contains a rejected tlv which indicates the fields and why they're rejected (high/low).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but we need to figure out the warning message bit where it contains a rejected tlv which indicates the fields and why they're rejected (high/low).

I can take a stab at this, looked into it a while back. I think @rustyrussell's idea of bad field / bad value / suggested value will fit pretty well for this use case. Simple enough to repeat that field if we want to point out all the fields we don't like (although I agree that in practise the first one we encounter and don't like should be good enough)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, feel free to request a review

@TheBlueMatt
Copy link
Collaborator

Can you share the rationale for wanting to change the dust limit? While changing the dust limit based on fee rates sounds nice, it would re-add all of the complexity of dust exposure management that we were able to get rid of with anchor-0-htlc-fee.

@t-bast
Copy link
Collaborator

t-bast commented Sep 27, 2022

My main comment from yesterday's spec meeting is that having no HTLCs in the commitment is too restricting. As a node operator, you cannot know beforehand how long it will take you to reach that state, as you may have at least one HTLC that could be stuck for an indefinite amount of time. While you've started the propose flow and are waiting for all HTLCs to be resolved, you're not earning fees on that channel (and you may be penalized by path-finding algorithms because the channel is unusable).

I think it's worth the effort to make those parameters upgrade with pending HTLCs in the commitment, even if it requires adding non-trivial logic to recompute trimming thresholds and verify we're not overflowing the 483 HTLCs limit per side. That means we could simply leverage the quiescence state and build on top of it instead of introducing a separate mechanism.

@Crypt-iQ
Copy link
Contributor Author

Crypt-iQ commented Sep 27, 2022

Can you share the rationale for wanting to change the dust limit?

I thought it made sense to change, but now I don't know. Maybe the node operator wants to have a smaller commit tx or something.

While changing the dust limit based on fee rates sounds nice, it would re-add all of the complexity of dust exposure management that we were able to get rid of with anchor-0-htlc-fee.

I think this exists independently of anchor-0fee since both have the dust exposure issue? In our code, we have a method that calculates the exposure on the channel and in the context of this flow, if the new dust limit makes that method error, we'd reject the proposal so it doesn't end up being that complicated for us

@TheBlueMatt
Copy link
Collaborator

TheBlueMatt commented Sep 27, 2022

I think this exists independently of anchor-0fee since both have the dust exposure issue?

Well, if the dust limit is static that calculation is super trivial - just add the dust HTLCs and you're done. Even better, if it's static you just set the limit so that your exposure / 400 is what you want. Anchor-0fee makes the dust limit static for the first time, making it dynamic again kinda sucks IMO.

@Crypt-iQ Crypt-iQ closed this Jun 23, 2023
@Crypt-iQ Crypt-iQ deleted the dyncom_092122_no_upgrade branch June 23, 2023 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants