-
Notifications
You must be signed in to change notification settings - Fork 492
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
Explicitly allow funding_locked early, and support alias scids (feat 46/47/50/51) #910
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.
This seems to address all my questions/feedback I had while implementing, outside of the questions around feature bit in the previous PR - nodes that want to be able to use an alias for their incoming payments may still want to connect to nodes that implement sending aliases. That said, I dunno if anyone has any plans on implementing such peer-seeking behavior.
02-peer-protocol.md
Outdated
- wait until the funding transaction has reached `minimum_depth` before | ||
sending this message. | ||
- set `next_per_commitment_point` to the per-commitment point to be used | ||
- MUST set `next_per_commitment_point` to the per-commitment point to be used |
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.
So this implies we always send the thing we sent in our last revoke_and_ack
(if there was a previous one)?. Just making sure we're all on the same page.
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 this means send the per-commitment-point for the first commitment transaction (hence the #1)? Rather than the last one
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.
Yes, this changed in the second (fixup) commit that was pushed after this comment was made :).
- MUST NOT send the same `alias` for multiple peers. | ||
- MUST always recognize the `alias` as a `short_channel_id` for incoming HTLCs to this channel. | ||
- if `option_scid_alias_only` was negotiated: | ||
- MUST NOT allow incoming HTLCs to this channel using the real `short_channel_id` |
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.
If option_scid_alias_only
is negotiated and incoming HTLCs of the real short_channel_id
are disallowed, does this break the spontaneous nature of key-send? The payer will need to know one of the alias short_channel_id
which will be different from the publicly announced "real" one in the channel_announcement
/ channel_update
messages. Additionally, regular routing may require a payer to know aliases of ALL option_scid_alias_only
-negotiating hops along the way.
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.
scid aliases don't (currently) make sense for public channels - with the node ids public its obvious to the sender which channel is/isnt being used. They only really make sense to provide privacy to non-public nodes. Agreed this could be more clear 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.
It also makes sense in the zero-conf case before the scid is known
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, but that doesn't work for public channels - you have to announce all public channels with the "real" ID for anti-DoS reasons, this isn't unique to keysend. I think what you're really saying here is that option_scid_alias_only
needs to disable announcements/imply/require the channel to be private.
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 was getting confused between the option and the TLV segment. Let's say the option isn't negotiated and a zero-conf channel is opened that is public after minimum_depth 6 confs. Keysend should work once public since routing to the real short_channel_id is allowed?
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.
Its already implied cause you're not allowed to set a public channel as scid_alias
.
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 clause you're referencing only requires that if the option_scid_alias channel_type was negotiated rather than the feature bit?
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.
Yea, the current text of this PR, however, only adds the only-use-alias-when-forwarding requirement for the "channel_type
has option_scid_alias
set", which is correct, I believe. There is nothing implied by the feature bit 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.
For private zero-conf channels, this line seems to be ignored (the zero-conf and option-scid-alias channel types seem to be exclusive)
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'm honestly entirely unsure what we're discussing here at this point - the above comments all seem to be on the same page - you can't set scid_alias on a public channel, and for private channels you can set it allowing the limiting of relay to the alias.
@rustyrussell you'd mentioned previously this may make sense/need to be a channel type given the persistent scid alias info - I think I agree (but don't feel strongly at all really) - do you plan to update this PR? |
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 don't understand the point of option_scid_alias_only
. Shouldn't a node just advertise its ability to understand scid
aliases? Something like option_scid_alias
.
Then if you want your peer to only use aliases, just request private channels? The only scid
that will be used will be those provided in routing hints. Otherwise things gets confusing, as this comment thread shows: https://github.com/lightningnetwork/lightning-rfc/pull/910/files#r722406596.
02-peer-protocol.md
Outdated
Nodes which have funded the channel, or trust their peers to have | ||
done, or wish to use `push_msat` from other side's opening (without | ||
accepting incoming HTLCs) can simply start using the channel instantly | ||
by sending `funding_locked`. |
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.
"can simply start using the channel instantly" is misleading because you still need to wait for the counterparty's funding_locked
before sending payments.
How about:
Nodes which have funded the channel, or trust their peers to have | |
done, or wish to use `push_msat` from other side's opening (without | |
accepting incoming HTLCs) can simply start using the channel instantly | |
by sending `funding_locked`. | |
Nodes which have funded the channel, or trust their peers to have | |
done, or wish to use `push_msat` from other side's opening (without | |
accepting incoming HTLCs) can simply send their `funding_locked` right | |
away. As soon as both parties have sent their `funding_locked`, the channel | |
can be used even if the funding transaction hasn't yet been confirmed. |
02-peer-protocol.md
Outdated
- if it sets `alias`: | ||
- if the `announce_channel` bit was set in both `open_channel` and `accept_channel`: | ||
- SHOULD initially set `alias` to value not related to the real `short_channel_id`. | ||
- SHOULD re-transmit `funding_locked` with the real `short_channel_id` once the channel is confirmed. |
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.
For public channels, the real short_channel_id
is included in announcement_signatures
. Why would we need to retransmit funding_locked
?
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 could also suggest that announcement_signatures should only be sent after minimum_depth in case funding_locked is sent early realized announcement_signatures
is only after 6 confirmations and is independent of minimum_depth
This is an alternative to lightning#910, it is very close with the following changes: - make zero-conf and `scid` `alias` independent Fundee may signal that it is willing to trust the funder by setting `minimum_depth` to zero, but that's unenforceable anyway. - make `scid` `alias` and channel announcement independent There is no reason to advertize the "real" `scid` in `funding_locked`, because the real `scid` is already provided in `announcement_signatures` - have nodes only advertise their ability to understand `scid` `alias` (feature `option_scid_alias`) If this feature is negotiated (both peers support it), then they must only use `alias`es (but that's unenforceable). - accept that some combinations don't make sense, like "zero-conf without alias", or "alias and public channels"
02-peer-protocol.md
Outdated
- MUST always recognize the `alias` as a `short_channel_id` for incoming HTLCs to this channel. | ||
- if `option_scid_alias_only` was negotiated: | ||
- MUST NOT allow incoming HTLCs to this channel using the real `short_channel_id` | ||
- MAY send multiple `funding_locked` messages with different `alias` values. |
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.
Is this "may send multiple ..." during the lifetime of 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.
Yes, per channel...
No, this is critical to protect against probing. |
I have explored this approach in this branch https://github.com/pm47/lightning-rfc/tree/zeroconf-as-alias-pm. In summary:
My point is if both parties support Answering #923 (comment):
Not for e.g. Rusty's "spend push_msat" use case which is valid imo.
Nope, I missed it (970??). I guess it could be a channel type (@t-bast wdyt?) but this is a separate concern from the semantics of this option.
That's a very good point: in fact, if we were to implement Answering #923 (comment):
It's public in the sense that both peers have set the
I guess it's a channel failure, or a warning, depending on how much you care. |
I agree that node alias is roughly the same as channel alias. I'm partial to the funding_locked alias though since it is simpler to implement for the lnd side. It meshes well with our current design of having channels be represented by ShortChannelID under-the-hood. |
Yes, text fixed. The intent was that everyone should be setting this option in future, and thus respecting the alias. We can certainly rename it to |
True, and you won't be able to because it probably is still in mempool. Ah. You mean the requirement to re-xmit. OK, I'll remove that.
Yep, done.
Zero-conf without alias would work for direct payments, but they're not generally useful. But aliases on public channels are good for routehints before funding tx is 6 deep.
Yep. Spec now says this means you gotta ignore requests for "real" scid on unannouncable channels.
Or trust. But just supply a damn scid, it's easy :)
SCID inside an onion is a compact representation of the next node id. Always has been. It's only a direct channel reference for channel_update.
I will close the channel. Don't do that :) |
Hmm, should we remove the requirement that announcement_signatures only be sent after funding_locked has been exchanged? We'll be always-sending "funding_locked" (which we should rename to channel_start or something), and I don't think anyone was waiting more than 6 blocks before anyway.... |
1bf9356
to
6d7fab7
Compare
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.
Looking good! Will implement and report back.
I don't think anyone was waiting more than 6 blocks before anyway....
We do: we scale confirmations for large channels, e.g. 30 blocks for 10+ BTC.
I think we are missing something in the channel_update section of BOLT 7:
This fake Another option would be to not send the |
Yerch... you need to use an alias, but whose, and which one? I think the general rule here is "update_channel should use your own alias for unannounced channels". That works whether the update_channel is for an error response, as well as this case. @TheBlueMatt wanted to allow multiple aliases, but this kills that idea, and I think it should be removed. (Aside: I am looking at switching this to channel types, too: this makes far more sense when we try to upgrade, though it's a little more work in the short term. I'll try implementing it today...) |
Sorry I've been totally MIA on this PR, will get back to it in a week or two.
Jumping on the tag, I don't see why? We can just make channel_update use the "real" scid, or 0, or whatever? Or we can use that to provide the aliases and not the funding_locked message? In any case, I'd strongly prefer to be able to rotate the "private" SCID - its quite useful for privacy, at least once we add pubkey aliasing. I'd prefer to not have to add SCID rotating when we get there and just do it while we're doing this anyway. |
You can't use 0, because we can have more than one channel. You can't use the "real" scid since it might not exist yet (zeroconf). You could send a channel_update every time the you get a new alias, I guess that's friendly (you call this channel 7x7x7? OK, here's the channel_update for "7x7x7"). I'm actually not sure what happens when we get a bad fee on a private channel, whether we send a channel_update or not. I'm pretty sure we don't in c-lightning. We should def. not leak the real scid in that case! |
Rotating can be probed without pubkey rotation which doesn't work without telling your peer what the association is (otherwise they can send via the new pubkey on the old channel scid, etc). And we decided not to do that, going for blinded paths instead. This simply fixes the "look at my UTXO!!" problem. |
Indeed, that's exactly what I said, gotta have pubkey aliases :)
"we" did? I think I'd love to go that route too, but last I heard work had largely stalled and there was some significant disagreement about trampoline and friends and how to build it appropriately. I'd love to learn that work is not stalled, however! |
Well, the onion message enctlv format is the same as the HTLC one, so hopefully progress in one leads to progress in the other? |
Side note: we considered using this mechanism to make the sender pay for on-the-fly channel creation in Phoenix (or split the cost between sender/receiver). Routing hints would have the "normal" feerate (the one if an existing channel could be used), but if a new channel needed to be created, our node would return a
|
Agree, and maybe a good opportunity to introduce a new
Would make it easier because since |
True! I guess what you're telling me is the issues that folks had are resolved, and I shouldn't worry and its definitely moving forward? If that's definitely the case than I'm totally happy to drop the alias-rotation stuff, I just figured it had ~no implementation cost and was an easy future win, given murkyness of trampoline's future. |
lnd 0.13.1+ properly handles a channel update being returned in a failure message (lightningnetwork/lnd#5332). I think this may be fine but dirty, as long as the REAL scid isn't leaked for priv channels.
I don't think it's possible to broadcast the |
And weaken it: the opener doesn't need to respect it. Note also that the `funding_locked`-can-change-alias refers to the same peer. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Allows upgrade in future. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And `next_per_commitment_point` to explictly `second_per_commitment_point`; this is particularly important since `channel_ready` can be retransmitted after the channel has been in use, for example. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
58f4250
to
34e9cd9
Compare
Squashed down the fixup commits including two trivial changes from my end. Total diff since Rusty's last commit:
|
ACK 34e9cd9 |
Isn't this vulnerable to double spend? |
@1440000bytes , yes it is. But this is why if you are on the receiver side you should only accept an htlc on such a channel if you trust the peer you are doing this with (in the pre-six-conf phase). You would not do this with someone that you think will double spend. So this will mostly be between a user and their LSP. |
Minimal set of changes to update the peer_wire.csv to include the TLV field in the `funding_locked` message, and add type 1=alias from that PR too.
Minimal set of changes to update the peer_wire.csv to include the TLV field in the `funding_locked` message, and add type 1=alias from that PR too.
Since lightning#910, nodes are allowed to use aliases instead of real scids. It is helpful to be explicit about it and set a flag in `channel_update` when an alias is used instead of a real scid. We also make the `htlc_maximum_msat` field mandatory: every node on the network currently sets it, so we can simplify the spec.
This implements lightning/bolts#910. Co-authored-by: Bastien Teinturier <31281497+t-bast@users.noreply.github.com>
Minimal set of changes to update the peer_wire.csv to include the TLV field in the `funding_locked` message, and add type 1=alias from that PR too.
Minimal set of changes to update the peer_wire.csv to include the TLV field in the `funding_locked` message, and add type 1=alias from that PR too.
Minimal set of changes to update the peer_wire.csv to include the TLV field in the `funding_locked` message, and add type 1=alias from that PR too.
Minimal set of changes to update the peer_wire.csv to include the TLV field in the `funding_locked` message, and add type 1=alias from that PR too.
Minimal set of changes to update the peer_wire.csv to include the TLV field in the `funding_locked` message, and add type 1=alias from that PR too.
Since lightning#910, nodes are allowed to use aliases instead of real scids. It is helpful to be explicit about it and set a flag in `channel_update` when an alias is used instead of a real scid. We also make the `htlc_maximum_msat` field mandatory: every node on the network currently sets it, so we can simplify the spec.
Since lightning#910, nodes are allowed to use aliases instead of real scids. It is helpful to make it explicit that updates using such aliases must not be forwarded to other nodes by setting a flag in `channel_update`. This flag is also generally useful for unannounced channels, regardless of whether they use an scid alias or not. We also make the `htlc_maximum_msat` field mandatory: every node on the network currently sets it, so we can simplify the spec.
Since #910, nodes are allowed to use aliases instead of real scids. It is helpful to make it explicit that updates using such aliases must not be forwarded to other nodes by setting a flag in `channel_update`. This flag is also generally useful for unannounced channels, regardless of whether they use an scid alias or not. We also make the `htlc_maximum_msat` field mandatory: every node on the network currently sets it, so we can simplify the spec.
This PR does three things:
Closes: #895