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

Add require_confirmed_inputs TLV to open and accept #7

Closed

Conversation

t-bast
Copy link

@t-bast t-bast commented Oct 20, 2022

This lets any side of the protocol require the other side to use confirmed inputs, to avoid paying the fees of a low feerate unconfirmed ancestor. It is using an even TLV field to ensure that the other side understands it, and make sure that it will be correctly applied whenever it is set.

Since it is specified at the beginning of the protocol (in open_channel2 and accept_channel2), if the receiving node doesn't want to use confirmed inputs they can fail the negotiation early with a helpful error.


The sending node:
- if the receiver set `require_confirmed_inputs` in `open_channel2` or `accept_channel2`:
- MUST NOT send a `tx_add_input` that contains an unconfirmed input
Copy link
Owner

Choose a reason for hiding this comment

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

what happens if a node receives an input that's unconfirmed? should we add a note on the receiver's side that they should verify received inputs?

Copy link
Author

Choose a reason for hiding this comment

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

Added in the tx_complete requirements, since that's where it makes most sense to check.

@@ -1116,6 +1116,9 @@ This message initiates the v2 channel establishment workflow.
1. type: 1 (`channel_type`)
2. data:
* [`...*byte`:`type`]
1. type: 2 (`require_confirmed_inputs`)
2. data:
* [`0*byte`:``]
Copy link
Owner

@niftynei niftynei Oct 21, 2022

Choose a reason for hiding this comment

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

Hm i think there's a different way to express (1 byte) ...

Copy link
Author

Choose a reason for hiding this comment

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

Note that this isn't 1 byte, this is a 0-byte TLV. It should be encoded as: 0x0200 (tag = 0x02, length = 0x00). Such tlv "signals" will likely appear more often in the spec (we have the same need for async payments), but I'm not sure how it should be written to avoid breaking cln's parsing tool?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah I see, it's a zero-length TLV item.

Copy link
Owner

Choose a reason for hiding this comment

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

Let me check in with the parser.. i'm not sure we have an annotation for that yet. Do you know if this is the first one in the spec?

Copy link
Author

Choose a reason for hiding this comment

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

It is indeed the first one in the spec...

@niftynei
Copy link
Owner

Checking my understanding:

  • if either peer requests confirmed outputs, the both sides MUST only send confirmed outputs?

If a node receives a request for confirmed-only inputs, they MAY fail the channel if they can't comply.

There's nothing mentioned for when to check proposed inputs confirmation status; may be worth doing during the tx_complete checks or there abouts?

@@ -1159,6 +1164,10 @@ whichever is greater.

Note that `push_msat` has been omitted.

The sending node may require the other participant to only use confirmed inputs.
Copy link
Owner

Choose a reason for hiding this comment

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

require the other participant -> if either side asks for it then it should be a requirement on both sides?

@t-bast t-bast force-pushed the dual-fund-confirmed-inputs branch from e64d6b3 to 1be9fee Compare October 24, 2022 08:35
@t-bast
Copy link
Author

t-bast commented Oct 24, 2022

I've reworked this commit in 1be9fee to provide more details.

There's nothing mentioned for when to check proposed inputs confirmation status; may be worth doing during the tx_complete checks or there abouts?

Agreed, added!

if either peer requests confirmed outputs, the both sides MUST only send confirmed outputs?

No, I'd like that to be asymmetric which is more flexible. You set this flag to tell the other party that they're only allowed to use confirmed inputs. If both parties set the flag, then both sides can only use confirmed inputs.

The reason it's useful to allow the asymmetry is for example for the LSP <-> mobile wallet case. The mobile wallet will likely pay a fee to have the LSP open a channel to them with liquidity on their side (à la liquidity ads). The LSP will be the one driving the RBF logic to ensure the funding tx confirms, but they don't want their liquidity fee to be "burnt" by having to pay for unconfirmed ancestors of the mobile wallet. The mobile wallet on the other hand doesn't necessarily care whether the LSP uses confirmed inputs or not, as the LSP will be responsible for doing RBF and getting the funding tx to confirm, and will deal with the feerate of their unconfirmed ancestors. Also, LSPs will likely have a bigger utxo churn than mobile wallet users, and will have a hard time doing channel open at scale with only confirmed inputs.

Does that sound reasonable?

Copy link

@morehouse morehouse left a comment

Choose a reason for hiding this comment

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

LGTM

@t-bast t-bast force-pushed the dual-fund-confirmed-inputs branch from 1be9fee to 9a8c907 Compare October 25, 2022 12:36
@t-bast
Copy link
Author

t-bast commented Nov 22, 2022

@niftynei would you have time to implement this in cln and let me know if we can finalize the tlv format?

This lets any side of the protocol require the other side to use confirmed
inputs, to avoid paying the fees of a low feerate unconfirmed ancestor.
@t-bast t-bast force-pushed the dual-fund-confirmed-inputs branch from 9a8c907 to fa9a3e8 Compare December 2, 2022 11:38
@t-bast
Copy link
Author

t-bast commented Dec 2, 2022

Rebased.

@niftynei
Copy link
Owner

subsumed by #11

@niftynei niftynei closed this Dec 16, 2022
@t-bast t-bast deleted the dual-fund-confirmed-inputs branch December 19, 2022 08:41
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.

3 participants