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

BOLT-09: move static remote to global feature bit #680

Open
wants to merge 1 commit into
base: master
from

Conversation

@Roasbeef
Copy link
Member

commented Oct 2, 2019

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.

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.
@Roasbeef Roasbeef requested review from t-bast and rustyrussell Oct 3, 2019
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Oct 3, 2019
This preempts the acceptance of
lightningnetwork/lightning-rfc#666 but it's
clear that feature bits are going to be distinct, so this is safe to
do anyway.

See lightningnetwork/lightning-rfc#680

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Oct 3, 2019

Concept ACK, but why not finalize #666 instead?
It renders this distinction moot as #666 broadcasts local + global in node_announcement.

rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Oct 3, 2019
This preempts the acceptance of
lightningnetwork/lightning-rfc#666 but it's
clear that feature bits are going to be distinct, so this is safe to
do anyway.

See lightningnetwork/lightning-rfc#680

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
araspitzu added a commit to ACINQ/eclair that referenced this pull request Oct 3, 2019
araspitzu added a commit to ACINQ/eclair that referenced this pull request Oct 3, 2019
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Oct 4, 2019
This preempts the acceptance of
lightningnetwork/lightning-rfc#666 but it's
clear that feature bits are going to be distinct, so this is safe to
do anyway.

See lightningnetwork/lightning-rfc#680

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2019

@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.

@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Oct 5, 2019

it's starting to creep into updating the existing hop hints to have feature bits for each channel/node.

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.

@t-bast
t-bast approved these changes Oct 5, 2019
rustyrussell added a commit to ElementsProject/lightning that referenced this pull request Oct 7, 2019
This preempts the acceptance of
lightningnetwork/lightning-rfc#666 but it's
clear that feature bits are going to be distinct, so this is safe to
do anyway.

See lightningnetwork/lightning-rfc#680

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
darosior added a commit to darosior/lightning that referenced this pull request Oct 7, 2019
This preempts the acceptance of
lightningnetwork/lightning-rfc#666 but it's
clear that feature bits are going to be distinct, so this is safe to
do anyway.

See lightningnetwork/lightning-rfc#680

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell

This comment has been minimized.

Copy link
Collaborator

commented Oct 8, 2019

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?

@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2019

@araspitzu

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

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.

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?email_source=notifications&email_token=AAHTWLXIV3VOHC5H27ZP2LTQNPU7RA5CNFSM4I42DPT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEASM7RY#issuecomment-539283399>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHTWLUO3EGQTJBEFCFZKG3QNPU7RANCNFSM4I42DPTQ .

@rustyrussell

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2019

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.

@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2019

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 ;)
I think this feature (and a few others) shows that what's important is how you communicate feature bits to others and being able to communicate what we currently call local features in node announcements (and potentially other messages as well).
So we're ok with either of those since this is very-soon-to-be-deprecated, if that helps lnd's release be smoother we don't mind putting this in the global field.

@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

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.

@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

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).

@t-bast

This comment has been minimized.

Copy link
Collaborator

commented Oct 10, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.