Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Zero Conf Channels #1401
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
Zero Conf Channels #1401
Changes from all commits
585493a
168b3a5
35cd39d
9569bfe
ff9e457
26288e3
8be97f0
7ed7a7d
86ad252
ffaf9bc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 would previously return an error when
min_depth == 0
, whereas now we'll enforce a minimum of one, so they may think we've agreed to proceed with zero confs once we sendfunding_created
back.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.
Right, this is kinda a 0conf philosophy question. This PR doesn't actually support the 0conf channel type (as it was written before the 0conf channel type existed) which is the "I require 0conf" option, but we'll add that in a followup.
In general, if you're doing a 0conf channel, you've proably pre-negotiated it in some way - most likely the recipient of the channel is pre-configured to trust an LSP. There's arguably not a lot of value in negotiating 0conf in the open/accept channel flow - by the time you're there both sides of the channel should really already know if the channel should be 0conf or not.
More generally, nodes can always delay
channel_ready
/funding_locked
as long as they want - theminimum_depth
field is just a hint, and the protocol can't enforce it in any way. We're not strictly-speaking "out of spec" by delaying more than the channel recipient wanted.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 change almost seems like it belongs in a separate commit or be documented in the commit message, seemed a bit unrelated at first
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.
Updated the commit message.
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 guess this predated the PR, but it's unclear to me what
funding_tx_confirmation_height
of 0 means.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.
Ah, it means "not yet confirmed", as certainly no channel is going to be confirmed at height 0 :) I kinda figured it was an obvious default but I can add docs if you prefer?
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.
Meh, maybe an
is_funding_confirmed
function would be more readable, but no need to do anything here.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.
Apart of more code complexity, I don't understand why we would force-close a 0-conf channel as we already assume it's safe without block inclusion ? And I'm not sure there is a real inconsistency risk with the
short_to_id
being inserted atfunding_locked
being a fake SCID ? Though one concern would be the funding inputs unconfirmed themselves.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.
No, we should fix that eventually - #867