-
Notifications
You must be signed in to change notification settings - Fork 493
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
Extension/dynamic commitments #1117
base: master
Are you sure you want to change the base?
Extension/dynamic commitments #1117
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice read! I think what's unclear to me is what the new commit tx looks like if we decide to change the funding output, my current understanding is,
kickoff_tx:
inputs:
0: old funding outpoint
outputs:
0: new funding output
...
commit_tx:
inputs:
0: new funding outpoint
outputs:
...
Also what does the coop closing tx look like?
closing_tx_1:
inputs:
0: new funding outpoint
outputs:
...
closing_tx_2:
inputs:
0: old funding outpoint
outputs:
...
If it's closing_tx_1
, we need to broadcast kickoff_tx
first. If it's closing_tx_2
, we can save one tx.
The other thing I'm trying to understand is, if the goal is to upgrade to STC, I don't think we need to change the funding output? We just need to create new commit tx with p2tr
outputs, and let it spend the original funding output, then there's no need to build the kickoff tx?
- MUST remember its last sent `dyn_propose` parameters. | ||
- if it is currently waiting for a response (`dyn_ack` or `dyn_reject`): | ||
- MUST NOT send another `dyn_propose` | ||
- SHOULD close the connection if it exceeds an acceptable time frame. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean abort the proposal phase or close the channel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disconnect. If no proposal response is received for a while it can prevent the channel from being usable and so the connection should be cycled.
ext-dynamic-commitments.md
Outdated
does not and cannot use the output script format detailed in BOLT 3 for the | ||
funding output. Pairing this fact with what we described in the previous | ||
section, it is necessarily the case that the funding output of a Taproot channel | ||
cannot be properly announced by the current gossip system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this context, but it seems the sections Gossip Verification
and Taproot
are talking about the same thing?
It also sounds like dyn will solve this issue while in fact not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent was for the Gossip Verification
section to make clear the fact that we verify the existence of UTXOs that underwrite a channel, and the Taproot
section was supposed to detail the fact that taproot outputs could not be attested to this way. Maybe I should just combine the sections?
Regarding dyncomms fixing this, you're right it doesn't. Dyncomms simply sidesteps it by not spending the original funding output while still building the new commitments off of a taproot output.
ext-dynamic-commitments.md
Outdated
and the `responder`. It is necessary for both nodes to agree on which node is | ||
the `initiator` and which node is the `responder`. This is important because if | ||
the dynamic commitment negotiation results in a re-anchoring step (described | ||
later), it is the `initiator` that is responsible for paying the fees for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
ext-dynamic-commitments.md
Outdated
- If either channel party changes `max_accepted_htlcs`: Rules Change | ||
- If either channel party changes `funding_pubkey`: Funding Output Update | ||
- If new `channel_type` requires different funding output script than the old | ||
`channel_type`: Funding Output Update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise Commitment Update
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably. This is one of the things that probably needs to be worked out a bit better. The channel_type
parameter is unfortunately not easy to work with because it is simultaneously an enumerated type as well as a set of feature bits. Since signing a new commitment doesn't hurt and would probably cover all cases, I think we can require that all channel_type
changes that aren't funding output changes require a new commitment signing.
ext-dynamic-commitments.md
Outdated
* txout count: 3 | ||
* `txout[0]`: `anchor_output_1` or `anchor_output_2` | ||
* `txout[1]`: `anchor_output_1` or `anchor_output_2` | ||
* `txout[2]`: `p2tr_funding_output` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it doesn't necessarily need to be a p2tr
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is one of those things that needs a full Type -> Type matrix
ext-dynamic-commitments.md
Outdated
|
||
1. Initialize the commitment transaction version and locktime. | ||
2. Initialize the commitment transaction input. | ||
3. Calculate which committed HTLCs need to be trimmed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think HTLCs are cleared at this stage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are not. Quiescence guarantees that traffic has stopped, not that htlcs have been flushed.
Paging @Roasbeef and @jharveyb for confirmation, but I believe if we do Since the legacy output necessarily can't have tapas in it, we only have to worry about the case where the kickoff would be hypothetically able to put them in. I realize this is something I haven't gotten clarification on that I need to.
STC means that the funding output of the channel is p2tr, not that we want to change the commitment outputs to p2tr (although it would be assumed that we would do those too). |
Did some digging and found this in the TAP channel bLIP:
This seems to indicate that the kickoff transaction would be capable of loading assets into a channel by adding its own tapleaf to the tap tree, so we need to prohibit cut-through in that case. Currently this proposal does not specify any mechanism for doing this, but explicitly blessing cut-through would make this impossible without some sort of special casing. |
e86c08f
to
c0b96c0
Compare
ext-dynamic-commitments.md
Outdated
1. type: 0 (`dust_limit_satoshis`) | ||
2. data: | ||
* [`u64`:`dust_limit_satoshis`] | ||
1. type: 1 (`max_htlc_value_in_flight_msat`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't all of these TLVs be even? Under okay to be odd reasoning, the receiving node can receive and ignore all of the odd fields here even if they don't understand what they are which could lead to miscommunication between nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. This is a good point.
I have been trying to figure out exactly when IOKTBO applies as there are a number of "core protocol messages" with odd message numbers, but yes, iiuc this is never the case with TLVs and this is a mistake. The choice to do it was naively more to keep the rejection vector compact since limiting to even numbers only makes it sparse, but this is probably a poor tradeoff since even though it's a 2x increase we're literally talking about a single byte 😬.
I will fix this in the next revision. Thank you for pointing it out!
ext-dynamic-commitments.md
Outdated
| |<-(4)---{dyn_ack|dyn_reject}---| | | ||
+-------+ +-------+ | ||
|
||
1. type: 111 (`dyn_propose`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feature bit + even message number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely the right move. This will be addressed. I haven't given as much attention to the specifics of the type/message number selection as I've been focusing on the overall shape of the design, but it's definitely important to make sure this matches the expectations of the rest of the protocol.
current negotiation. | ||
- if it will not accept **any** dynamic commitment negotiation: | ||
- SHOULD send a `dyn_reject` with zero value for `update_rejections` | ||
- if it does not agree with one or more parameters: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea of odd/even fields doesn't really map cleanly to a scenario where we actively need to reject fields (ie, we can't just ignore odd values like we do elsewhere).
This doesn't feel like a place where we'll want to extend functionality (as there are only so many channel params), but for the sake of future-proofing WDYT of something like:
- Set current negotiation TLVs to even values
- Reject any unknown odd TLVs in
update_rejections
That provides a possible upgrade path where we can start adding new values with odd tlv field numbers and know whether our counterparty actually understood them or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't feel like a place where we'll want to extend functionality
This is proooooobably right. Although given we already have open_channel
and open_channel2
I was going about this a bit more defensively.
Set current negotiation TLVs to even values
Definitely agree here.
Reject any unknown odd TLVs in
update_rejections
Yeah rejection is purely discretionary and this field is largely here to improve the overall UX. We could always provide unstructured or no information on why a proposal was rejected but I figured with a strictly enumerable set of parameters that marking the ones we find dissatisfactory would allow progress to be made more effectively.
As an aside, I don't know that legitimate use of odd fields here will ever make sense, since the whole purpose here is to coordinate on fundamental values. This careful coordination is generally incompatible with the idea of safely ignoring unknown values, so I think your assessment is correct that it will go unused. Even still, I think your recommendation makes sense because even if we never use them I think state machines negotiating the changes should reject things they don't understand in this context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished a first, high level review pass (sorry for breaking comments into two passes!)
The major question that I have for this PR is whether you'd be open to breaking the change up into two parts:
- Rules + commitment update (not including STC
channel_type
) - Funding output update
Reasoning being that:
- As is, achieving interop on this proposal is blocked on a second implementation of STC because you can't get coverage for
kickoff
without supporting theoption_taproot
type - (1) seems like something we all want, independent of the ongoing discussion around how to go about (2)
- We get the nice win of being able to upgrading legacy channels to zero fee anchors.
- It puts a mechanism in place that we could use to upgrade to V3 channels, which is another important security improvement.
ext-dynamic-commitments.md
Outdated
_NOTE FOR REVIEWERS_: The semantics of CPFP carve-out are not entirely clear | ||
as to whether or not there is only _one_ CPFP-Carve-Out "slot" or if the only | ||
two requirements are the 40kWU limit and a single unconfirmed ancestor. If we | ||
have more than one "slot" available, this is no longer a concern. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The carve out is "just one more", so it only works if each party has only one spendable output otherwise your counterparty can occupy the carve out slot themselves. Was recently looking at this myself and dug up this conversation from the original anchors PR.
ext-dynamic-commitments.md
Outdated
|
||
##### Kickoff Transaction Structure | ||
|
||
* version: 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on deployment timeline, it could make sense to make this a V3 transaction with a single shared key anchor if we forsee the kickoff being spent with its anchor before the commitment is broadcast.
This would help with pinning because sibling eviction allows us to have as many spendable outputs as we like (we don't run into "just one more" issues) , but has the (very) big downside that the commitment tx can't be broadcast with it because unconfirmed V3 parents required an unconfirmed V3 child (which the commitment isn't, but the sweep could be).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on deployment timeline, it could make sense to make this a V3 transaction with a single shared key anchor if we forsee the kickoff being spent with its anchor before the commitment is broadcast.
Shared key anchor seems like an interesting idea. I need to get smart on the details for V3 txs before I can give you a principled stance.
...but has the (very) big downside that the commitment tx can't be broadcast with it because unconfirmed V3 parents required an unconfirmed V3 child (which the commitment isn't, but the sweep could be).
fwiw, there's nothing that fundamentally prevents a commitment tx under this scheme from being V3, but it admittedly probably makes implementation complexity very unattractive. It would require conflating "rendering" with "funding output conversion" and I really like that I can think about them independently right now.
I am SO open to this that I have actually advocated for this before internally at LL, so thank you for the ammunition 😈 CC @Roasbeef That said, there is a nasty problem that we have to deal with if we go that route. The current
Hypothetically if these were all truly orthogonal, we could negotiate the axes here independently and just exclude the funding output script from consideration for the time being and it would be relatively well-specified with that small stipulation. We would also then be able to exclude the Of these different type axes they fall into 3 categories:
These categories also appear stable to me as we can easily map the opening channel parameters onto them quite easily:
I think people are rightly dreading the idea of having to implement the funding output conversion of the proposal and I think the desire to break these proposals up comes from a place of wanting incremental improvement. Fair enough! Let's look into the future a bit, though. Let's say now that we already have point 1 from your breakup suggestion and now we definitely want to implement point 2. Do we want to implement a new negotiation scheme? My intuition here is no. We would want to piggy back on the existing negotiation scheme and add a provision for executing the new (more complicated) change. Feel free to contest this, but the rest of my explanation assumes you'll agree. If we want to take advantage of the existing negotiation scheme we can somewhat easily implement the execution capability incrementally. After all, the existing negotiation scheme, as specified, allows for naming the particular parameters that you would like to reject. You could have a spec compliant implementation of this proposal that just always rejects upgrades to taproot. On a more practical level you would probably just reject any channel type you don't know how to interpret but the net result is the same. Later, should you decide to allow the new channel type altogether, you can implement the new execution capability and then simply stop rejecting requests to change that parameter. With that out of the way there is still a question of how to organize the documents and whether or not we want to have separate documents for various pieces here:
This is definitely a procedural hiccup. I see what you mean. If this proposal includes the spec'ed execution of a funding output conversion, do we technically have interop if we just implement negotiation but unconditionally reject everything? That's a hard question to answer and would welcome any thoughts on both how important this procedural issue is and any suggestions on how to do it given that this mechanism may be extensible into the future.
This is definitely the thing that has me scratching my head as I'm unaware of the details of V3 channels. Can you point me to anything that describes them? I assume it has to do with package relay policy and that at minimum we have a different version number on the commitment tx graph, but I'm not fully up to speed on the state-of-the-art here. |
The difficulty here is that
This seems to be the heart of the disagreement on how to go about part (2), so I don't think this is a foregone conclusion. I don't want to spam up this PR with that cross-proposal discussion so I've opened up a thread on delving here, but tl;dr I think that focusing on part (1) first is an incremental step forward that doesn't bias how we go about (2).
Given that the kickoff transaction is the bulk of complexity of the proposal, I'd say not? For the sake of comparison, I wouldn't consider interactive tx to have interop if we all just implemented
By "upgrade to V3 channels", I mean have a protocol in place that allows us to upgrade our commitment transaction format (#1 in your list of upgrade types). We'd still need to do the protocol work of defining what that commitment transaction looks like, implementing a subset of what's written up here here (specifically, without ephemeral anchors) - also added some details in that delving post. |
I think I see what you mean here in that by changing the funding output we have no choice but to also change the commitment tx rendering process since the commitment tx references the funding output by construction. What I'm not quite following here is the statement "...and upgrade to a taproot commitment with a v0 funding output". Do you mean "introduce a separate proposal to upgrade to taproot from a v0 funding output"?
I definitely do not believe it is a foregone conclusion, I just wanted to track the assumption in the rest of the argument to flesh it out entirely. If you don't want to implement that part you could always unconditionally reject chantype upgrades that include
It really depends on what interop is trying to communicate. In the case of If we assume that there are aspects to these protocols that are left up to the discretion of node policy, then we have to assume we might encounter any valid node policy. When things are "interoperable" to me it means that I should not reach any state that results in UB. That's a somewhat low bar and may be unsatisfying. The next natural line in the sand is "I should be able to reach at least one non-erroring terminal state". Finally, the most stringent "I should be able to reach all non-erroring terminal states". The last one seems impossible in the presence of any discretionary rejections. Maybe this is another candidate for a delving thread. I guess what I'm asking you here is, if I were to tell you implementations A and B "have interop" what expectations do you have of that claim, and which of those expectations do you depend on.
Got it. Yeah I believe this would fit quite neatly into the existing proposal framework and it definitely falls under the rendering parameters for sure. This ship may have sailed but I really hope we factor |
Definitely not (no moar proposals plz 😅 )! Meant that we could use dynamic commitments to only update our commitment transaction to use taproot without upgrading the funding output, introducing an inbetween channel type that has a segwit v0 funding output but uses taproot commitments. But as you point out, that certainly muddies the meaning of
Looks like we've had this conversation before, my take from reading through that discussion is that we're only going to move forward in an "additive" manner rather than face this explosion of channel type permutations which makes sense to me. LMK if I'm missing your point here?
Yeah def off topic for on this PR - delving or Monday's spec meeting 👍 That said, my sub-cent opinions here
I wouldn't consider that sufficient interop to merge to the bolts fwiw.
Indeed, but if we don't ever reach the state where two implementations don't add a features then (by my understanding) it shouldn't be a bolt and we should blip it. |
If such a channel type were to exist then yes we definitely could without issue. I don't think we (LND) are prioritizing making such a channel type a reality though. LND is interested in converting the funding output because we want to put taproot assets into the channel which requires the funding output change. I'm not a Tap wizard though so I am not 100% sure what the details of those constraints are. |
5cde7f8
to
ae7ea06
Compare
This PR specifies an extension-bolt to allow channel parameters to be renegotiated after channel open without requiring channel closure.