-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
multi: allow min-depth of zero for non-zero conf channels #8796
multi: allow min-depth of zero for non-zero conf channels #8796
Conversation
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
f007b18
to
0ddcb39
Compare
funding/manager.go
Outdated
|
||
// Zero conf channels will require an SCID alias, so request one | ||
// now and add it to the reservation. | ||
aliasScid, err := f.cfg.AliasManager.RequestAlias() |
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.
👍
hmmm ok from this comment on the original issue, it sounds like we dont actually need to go and change this to a zero conf channel. We can just allow them to send a min-depth of 0 (even though this is not a zero conf channel) and then we just continue as normal (and only send channel_ready when we see fit). so the diff can actually be way smaller cc @Crypt-iQ - wdyt? |
yup that seems fine |
0ddcb39
to
fcb4382
Compare
Ok I've made the update discussed above. It's hard to write a test for this case cause we dont really support this between two LND nodes opening a non-zero conf channel cause we want to register for confirmations and this is not allowed with a depth of 0. So to test this you can apply this patch which allows our channel acceptor to set a MinDepth of 0 & also makes sure we still register for 1 conf if we are on the receiver side of this operation. Then to actually test the flow, you can run the |
2f08f20
to
c64ba9d
Compare
@Crypt-iQ: review reminder |
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.
Needs a rebase, otherwise LGTM🌊
Even if the channel type is not zero conf. We will still use a min depth of at least 1. We just dont fail if our peer indicates trust.
c64ba9d
to
9d1320a
Compare
If our peer indicates to us that they are OK with a zero conf channel (ie, they trust us) by setting min-depth to 0 in accept_channel, then as long as they support SCID aliases, we should just upgrade the channel to ZeroConf and continue the funding flow.EDIT: updated to just allow the peer to set min-depth to 0. We dont upgrade the channel type.
Fixes #8783
NOTE: we already allow ourselves to do this when our peer sends us an OpenChannel without a zero-conf channel type:
lnd/funding/manager.go
Lines 1570 to 1583 in fed7609
So all this PR does is allow our peer to do this too :)