BOLT 1: add networks to init message. #682

rustyrussell · 2019-10-03T04:54:04Z

This has been discussed for forever, but now we have TLVs
the correct encoding seems obvious.

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

t-bast

ACK, does this supercede #678 or does it only complement it?

rustyrussell · 2019-10-04T00:34:58Z

I think it replaces it, TBH. I'm not sure that query_channels means you should hang up.

t-bast

Sounds good then, I do think it supercedes it but wanted confirmation.

Roasbeef · 2019-10-04T19:57:33Z

We have no real support for cross-chain routing in the protocol as is today, as a result, what use is this?

rustyrussell · 2019-10-08T00:10:55Z

We have no real support for cross-chain routing in the protocol as is today, as a result, what use is this?

It means you can hang up if you accidentally connect to a testnet node?

cfromknecht · 2019-10-08T01:24:19Z

01-messaging.md

@@ -225,8 +225,16 @@ Both fields `globalfeatures` and `localfeatures` MUST be padded to bytes with 0s
   * [`gflen*byte`:`globalfeatures`]
   * [`u16`:`lflen`]
   * [`lflen*byte`:`localfeatures`]
+   * [`init_tlvs`:`tlvs`]


qq on how we define this (i'm not super familiar with the parsing code): intuitively i would think that this would be

* [`tlvs`: `init_tlvs`]

where tlvs is the type, and init_tlvs is the name. the current way is flipped from what I would expect, maybe i'm just not thinking about it correctly?

The type is actually defined below... Confusing I know, but it's a consequence of incrementally implementing parsing.

We could just imply TLV from now on, with msg foo having a foo_tlv implicitly?

either works for me, was just curious what the guidelines are :)

cfromknecht · 2019-10-08T01:27:43Z

01-messaging.md

@@ -243,6 +252,8 @@ The receiving node:
    - MUST ignore the bit.
  - upon receiving unknown _even_ feature bits that are non-zero:
    - MUST fail the connection.
+  - upon receiving `networks` containing no common chains


what is the prescribed action if some chains intersect but not all? would think receivers should restrict all messages they send to this intersection?

Hmm, like @Roasbeef I haven't thought too hard about multi-chain implementation.

Hmm, I think that addition (SHOULD) would be positive, yes.

Sword-Smith · 2019-10-29T20:59:09Z

As a person who has accidently connected to several main net nodes with his test net node, I strongly support this pull request. With the added hope that the various implementations hang up if the network is wrong.

rustyrussell · 2019-10-30T06:10:17Z

OK, added the "don't forward gossip they're not interested in" sentence.

This implements lightning/bolts#682.

This implements lightning/bolts#682. Changelog-Added: protocol: We now signal the network we are running on at init.

rustyrussell · 2019-12-13T01:11:34Z

FWIW, this was accidentally merged into the latest c-lightning master (ElementsProject/lightning#3300). Seems to have caused no issues on my test nodes, but explicit interop is probably desirable (i.e. someone else implement and check it works with ours):

Mainnet: 024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605@128.199.202.16

Testnet:
031a3478d481b92e3c28810228252898c5f0d82fc4d07f5210c4f34d4aba56b769@165.227.30.200

And yeah, I tested that they refuse to maintain connections to each other now :)

This has been discussed for forever, but now we have TLVs the correct encoding seems obvious. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell · 2019-12-13T03:56:52Z

Trivial rebase, and whitespace cleanup.

t-bast · 2019-12-13T09:52:44Z

Seems to have caused no issues on my test nodes

I just tested that it doesn't cause any issue with existing eclair nodes; they simply ignore the TLV stream as expected.

I'll implement this and will let you know if compatibility is ok. Simply looking at the encoded TLV stream that c-lightning sends, it looks ok to me.

cdecker · 2019-12-13T19:13:40Z

Thanks @t-bast for checking. That puts my mind at ease

t-bast · 2019-12-16T13:43:25Z

I've implemented this on Eclair and tested compatibility with c-lightning, and there's a small issue.
Here are the test cases I ran:

Eclair advertising NetworkA and c-lightning advertising NetworkA -> nodes stay connected -> OK
Eclair advertising NetworkA and c-lightning advertising NetworkB -> nodes disconnect -> OK
Eclair advertising [NetworkA, NetworkB] and c-lightning advertising NetworkB -> c-lightning disconnects -> NOK

This isn't an issue for Eclair/c-lightning compatibility since we support a single chain, but it may be an issue with lnd if they advertize Bitcoin+Litecoin: c-lightning would reject connections from such lnd nodes.

@rustyrussell @cdecker I think this is worth fixing quickly on c-lightning master branch.

darosior · 2019-12-23T22:57:57Z

@t-bast I think ElementsProject/lightning#3371 fixes that.

t-bast · 2020-02-04T12:38:10Z

This has been implemented by c-lightning and eclair.
I believe @cfromknecht ACK-ed in a previous IRC meeting, is that right?
In my opinion this just needs a rebase and can be merged.

Roasbeef

LGTM ⛷

t-bast reviewed Oct 3, 2019

View reviewed changes

t-bast approved these changes Oct 4, 2019

View reviewed changes

cfromknecht reviewed Oct 8, 2019

View reviewed changes

Roasbeef added the Meeting Discussion Raise at next meeting label Oct 28, 2019

rustyrussell added the Awaiting Interop Approved, pending 2 or more impl interoperating label Nov 25, 2019

darosior added a commit to darosior/lightning that referenced this pull request Nov 28, 2019

connectd: add network to init message

a07932a

This implements lightning/bolts#682.

darosior added a commit to darosior/lightning that referenced this pull request Nov 28, 2019

connectd: add network to init message

4861e2c

This implements lightning/bolts#682.

darosior mentioned this pull request Nov 28, 2019

Add network to init message ElementsProject/lightning#3300

Merged

darosior added a commit to darosior/lightning that referenced this pull request Nov 28, 2019

connectd: add network to init message

1643186

This implements lightning/bolts#682. Changelog-Added: protocol: We now signal the network we are running on at init.

BOLT 1: add networks to init message.

35f1f47

This has been discussed for forever, but now we have TLVs the correct encoding seems obvious. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the init-contains-networks branch from ef7c97c to 35f1f47 Compare December 13, 2019 03:56

t-bast mentioned this pull request Dec 13, 2019

Add networks to init message ACINQ/eclair#1254

Merged

darosior mentioned this pull request Dec 23, 2019

connectd: check all chain hashs of the network array in init ElementsProject/lightning#3371

Merged

darosior mentioned this pull request Dec 25, 2019

Test init networks TLV ElementsProject/lightning-rfc-protocol-test#13

Open

darosior mentioned this pull request Feb 15, 2020

Lightning Specification Meeting 2020/02/17 #735

Closed

12 tasks

Roasbeef approved these changes Feb 17, 2020

View reviewed changes

Merge branch 'master' into init-contains-networks

ade28ab

t-bast merged commit 7b01692 into lightning:master Feb 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BOLT 1: add networks to init message. #682

BOLT 1: add networks to init message. #682

rustyrussell commented Oct 3, 2019

t-bast left a comment

rustyrussell commented Oct 4, 2019

t-bast left a comment

Roasbeef commented Oct 4, 2019

rustyrussell commented Oct 8, 2019

cfromknecht Oct 8, 2019

rustyrussell Oct 9, 2019

cfromknecht Oct 9, 2019

cfromknecht Oct 8, 2019

rustyrussell Oct 9, 2019

Sword-Smith commented Oct 29, 2019

rustyrussell commented Oct 30, 2019

rustyrussell commented Dec 13, 2019

rustyrussell commented Dec 13, 2019

t-bast commented Dec 13, 2019

cdecker commented Dec 13, 2019

t-bast commented Dec 16, 2019

darosior commented Dec 23, 2019

t-bast commented Feb 4, 2020

Roasbeef left a comment

BOLT 1: add networks to init message. #682

BOLT 1: add networks to init message. #682

Conversation

rustyrussell commented Oct 3, 2019

t-bast left a comment

Choose a reason for hiding this comment

rustyrussell commented Oct 4, 2019

t-bast left a comment

Choose a reason for hiding this comment

Roasbeef commented Oct 4, 2019

rustyrussell commented Oct 8, 2019

cfromknecht Oct 8, 2019

Choose a reason for hiding this comment

rustyrussell Oct 9, 2019

Choose a reason for hiding this comment

cfromknecht Oct 9, 2019

Choose a reason for hiding this comment

cfromknecht Oct 8, 2019

Choose a reason for hiding this comment

rustyrussell Oct 9, 2019

Choose a reason for hiding this comment

Sword-Smith commented Oct 29, 2019

rustyrussell commented Oct 30, 2019

rustyrussell commented Dec 13, 2019

rustyrussell commented Dec 13, 2019

t-bast commented Dec 13, 2019

cdecker commented Dec 13, 2019

t-bast commented Dec 16, 2019

darosior commented Dec 23, 2019

t-bast commented Feb 4, 2020

Roasbeef left a comment

Choose a reason for hiding this comment