-
Notifications
You must be signed in to change notification settings - Fork 494
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
BOLT-09: move static remote to global feature bit #680
Conversation
In this commit, we move the new static remote feature bit from a local feature bit, to a global feature bit. This should be a global bit as peers will want to seek out others peers based on if they support the new safety feature or not. Having it as a local feature bit means they need to connect out, and complete the handshake before finally being able to determine if a peer supports this new safety feature or not.
This preempts the acceptance of lightning/bolts#666 but it's clear that feature bits are going to be distinct, so this is safe to do anyway. See lightning/bolts#680 Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This preempts the acceptance of lightning/bolts#666 but it's clear that feature bits are going to be distinct, so this is safe to do anyway. See lightning/bolts#680 Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This preempts the acceptance of lightning/bolts#666 but it's clear that feature bits are going to be distinct, so this is safe to do anyway. See lightning/bolts#680 Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@t-bast #666 is a bigger overhaul, it's starting to creep into updating the existing hop hints to have feature bits for each channel/node. I don't think we'll have enough time to test+implement that for our upcoming release without delaying it by several weeks potentially. This is a simple move to how things should've been initially IMO. |
Honestly I don't think this is necessary for #666. I think #666 is ready as-is and what you propose is a further enhancement for a follow-up PR. However I do agree that it's more testing than just moving the static_remote_key bits to global features, which is indeed where it should have been, so we can merge this PR whenever you want. |
This preempts the acceptance of lightning/bolts#666 but it's clear that feature bits are going to be distinct, so this is safe to do anyway. See lightning/bolts#680 Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This preempts the acceptance of lightning/bolts#666 but it's clear that feature bits are going to be distinct, so this is safe to do anyway. See lightning/bolts#680 Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Huh? No, it's definitely not a global feature, as it only affects direct peers. However, like every local feature, it's interesting to broadcast it, so we should advertize it in node_announcement (which is what c-lightning master now does, along with every other local feature). So I would say: definitely advertize it in node_announcement for your coming release. Whether you treat it as a local or global feature bit now only affects the init msg. The spec will catch up, and it's definitely legal since we decided feature bits will be distinct between global/local already? |
It's a global feature insofar as it may affect _which_ peers you open
channels or connect with. Arguably the same should've been done for DLP.
…On Mon, Oct 7, 2019, 7:20 PM Rusty Russell ***@***.***> wrote:
However I do agree that it's more testing than just moving the
static_remote_key bits to global features, which is indeed where it should
have been, so we can merge this PR whenever you want.
Huh? No, it's definitely not a global feature, as it only affects direct
peers.
However, like every local feature, it's interesting to broadcast it, so we
should advertize it in node_announcement (which is what c-lightning master
now does, along with every other local feature).
So I would say: definitely advertize it in node_announcement for your
coming release. Whether you treat it as a local or global feature bit now
only affects the init msg. The spec will catch up, and it's definitely
legal since we decided feature bits will be distinct between global/local
already?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#680>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHTWLUO3EGQTJBEFCFZKG3QNPU7RANCNFSM4I42DPTQ>
.
|
To me it looks like a local feature because it only affects the two peers in the channel, a peer being selective when choosing the remote side doesn't justify having this feature global since it's not really used for any purpose than that (i.e routing), anyway #666 trumps these discussion since it allows having a local feature broadcast globally.
|
No, it was never meant to be a global bit. It doesn't make any more sense than any existing bit. Just advertize them both in node_announcement. |
This is about the init message, not the node announcement. As is, there
hasn't yet been any grand unification. Looking at the eclair PR, they've
already switched over and are on board (based on t-bast's first comment)
with this switch.
…On Tue, Oct 8, 2019, 5:05 PM Rusty Russell ***@***.***> wrote:
No, it was never meant to be a global bit. It doesn't make any more sense
than any existing bit. Just advertize them both in node_announcement.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#680>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHTWLWE2UPDPKVBSQCGZI3QNUN4JANCNFSM4I42DPTQ>
.
|
Honestly our thought about this is that we'd like to make progress on #666 soon, which deprecates arguing about whether this is a global or local feature ;) |
As I said above #666 has a much larger scope, and there're still questions of proper backwards compatibility. IMO it's too aggressive w.r.t assumptions of the the level of synchronization that currently exists amongst the various implementations when rolling our new updates. It effectively removes the global field from the init message, thereby ignoring the possibility of other implementations in the wild that may be using that space to gate experimental new features. This just moves 2 bits around. I'm not sure how soon this "soon" will really be given many of us have travel plans upcoming over the next few weeks as well. |
I spoked with Rusty offline, and CL is planning to move to just advertising all their bits in both fields. This works as we don't have any overlap between local/global bits right now. It effectively implements a superset of what's in this PR. AFAICT, the eclair PR still looks at the global init features as well. So it appears we're all aligned on this front now (#666 withstanding). |
Eclair can align on whatever is chosen here, we plan on doing the same as CL right after the static_remote_key PR is integrated in master so we don't really mind. |
Now a subset of #666. |
In this commit, we move the new static remote feature bit from a local
feature bit, to a global feature bit. This should be a global bit as
peers will want to seek out others peers based on if they support the
new safety feature or not. Having it as a local feature bit means they
need to connect out, and complete the handshake before finally being
able to determine if a peer supports this new safety feature or not.