Gossip query extensions (EXPERIMENTAL_FEATURES) by rustyrussell · Pull Request #2900 · ElementsProject/lightning

rustyrussell · 2019-08-05T03:54:37Z

DO NOT MERGE UNTIL RFC FINALIZED (it's EXPERIMENTAL_FEATURES, so we could, but might as well wait until tomorrow's meeting). This was the agreed final form, w00t!

This brings is close to in line with lightning/bolts#557

At least, we reproduce the test vectors. We still need behavoural tests (ie. protocol tests), but the code itself is fairly straightforward.

cdecker

Two minor things, other than that all good to go :-)

cdecker · 2019-08-09T08:51:09Z

wire/extracted_peer_experimental_csv

 msgdata,reply_channel_range,chain_hash,chain_hash,
 msgdata,reply_channel_range,first_blocknum,u32,
-@@ -168,6 +189,20 @@
+@@ -168,6 +189,19 @@


This doesn't look like it belongs here. Is this a diff-fragment?

This file is a diff file; since the size of the diff patch has been updated the range to which the diff applies updates also.

cdecker · 2019-08-09T08:54:06Z

gossipd/test/run-extended-info.c

+{
+}
+
+static const char *test_vectors[] = {


Loading the test vectors from file would have allowed us to update them more easily in the future, but for now that's ok.

There'll be a rework of test vectors in the spec sometime, and we can clean up how we treat them then.

cdecker · 2019-08-09T08:55:00Z

CHANGELOG.md

 - JSON API: `listfunds` now returns also `funding_output` for `channels`
 - plugins: plugins can now suggest `lightning-cli` default to -H for responses.
 - Plugin: new notification `forward_event` offered/settled/failed/local_failed.
+- Protocol: --enable-experimental-features support gossip query extensions proposal aka https://github.com/lightningnetwork/lightning-rfc/pull/557


Suggested change

- Protocol: --enable-experimental-features support gossip query extensions proposal aka https://github.com/lightningnetwork/lightning-rfc/pull/557

- Protocol: `--enable-experimental-features` support gossip query extensions proposal aka https://github.com/lightningnetwork/lightning-rfc/pull/557

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

…XPERIMENTAL) These indicate what fields we are to return. If there's now TLV, or we haven't got --enable-experimental-features, it's set to all 1s so behaviour is unchanged. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

We're about to use the for gossip extended info too, which *don't* put the encoding byte at the beginning of the data stream. So this removes some "scids" from function names and separates out the "prepend a byte" case from the "external encoding_type" case. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

…d (EXPERIMENTAL) In fact, we always generate them, we only send them if asked. And we set the flags to 0 if not --enable-experimental-features, so we never send in that case. Generating checksums involves pulling the channel_update from the gossip_store, which is suboptimal: there's a FIXME to store the checksum in memory. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Make the TLV element a simple array. This is a bit neater, in fact, and makes the test vectors in that 557 PR work. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

These both allow us to reproduce the test vectors in the next patch. But using Z_DEFAULT_COMPRESSION is a reasonable idea anyway. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

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

niftynei

nice to see the EXPERIMENTAL_FEATURES and tlv's getting a workout :)

niftynei · 2019-08-09T16:55:45Z

gossipd/gossipd.c

-	if (encoding_end_prepend_type(&encoded_scids, max_encoded_bytes)) {
+	if (extension_bytes <= max_encoded_bytes
+	    && encoding_end_prepend_type(&encoded_scids,
+					 max_encoded_bytes - extension_bytes)) {


why is the type the unused space? max_encoded_bytes - extension_bytes

I don't understand the question?

niftynei · 2019-08-09T16:59:18Z

wire/extracted_peer_experimental_csv

 msgdata,reply_channel_range,chain_hash,chain_hash,
 msgdata,reply_channel_range,first_blocknum,u32,
-@@ -168,6 +189,20 @@
+@@ -168,6 +189,19 @@


This file is a diff file; since the size of the diff patch has been updated the range to which the diff applies updates also.

niftynei · 2019-08-09T17:12:42Z

gossipd/test/run-extended-info.c

+	opt = json_get_member(test_vector, obj, "extensions");
+	json_for_each_arr(i, t, opt) {
+		assert(json_tok_streq(test_vector, t,
+				      "WANT_TIMESTAMPS | WANT_CHECKSUMS"));


would it be worthwhile to also test these separately?

Yes, but the test vectors are grossly insufficient. We need behavoural tests

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

rustyrussell · 2019-08-10T02:48:23Z

Applied @cdecker fix and rebased for CHANGELOG.md conflict.

Ack c5ac1fe

rustyrussell changed the title ~~Gossip query extensions (EXPERIMENTAL_FEATURES)~~ WIP: Gossip query extensions (EXPERIMENTAL_FEATURES) Aug 5, 2019

rustyrussell added enhancement gossip labels Aug 5, 2019

rustyrussell force-pushed the inv-gossip branch from 747be01 to c196772 Compare August 6, 2019 01:29

rustyrussell requested a review from niftynei August 6, 2019 01:29

rustyrussell changed the title ~~WIP: Gossip query extensions (EXPERIMENTAL_FEATURES)~~ Gossip query extensions (EXPERIMENTAL_FEATURES) Aug 6, 2019

rustyrussell mentioned this pull request Aug 6, 2019

BOLT7: extend channel range queries with optional fields lightning/bolts#557

Merged

rustyrussell added this to the 0.7.2 milestone Aug 7, 2019

cdecker reviewed Aug 9, 2019

View reviewed changes

rustyrussell added 8 commits August 10, 2019 01:35

EXPERIMENTAL: pull in PR 557 (with minor fixes): range query support.

50fdeda

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

common: implement decoding of query-flags for query_short_channel_ids.

98d554a

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

spec: remove encoding byte from checksums.

f43ed9c

Make the TLV element a simple array. This is a bit neater, in fact, and makes the test vectors in that 557 PR work. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

gossipd: use default zlib compression, hack for zlib expansion.

77d8e3a

These both allow us to reproduce the test vectors in the next patch. But using Z_DEFAULT_COMPRESSION is a reasonable idea anyway. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

gossipd: test vectors for lightning/bolts#557

ae707b0

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

niftynei approved these changes Aug 9, 2019

View reviewed changes

CHANGELOG: note the support for ElementsProject#557.

c5ac1fe

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

rustyrussell force-pushed the inv-gossip branch from 14b663c to c5ac1fe Compare August 10, 2019 00:25

rustyrussell merged commit 5e78960 into ElementsProject:master Aug 10, 2019

	- Protocol: --enable-experimental-features support gossip query extensions proposal aka https://github.com/lightningnetwork/lightning-rfc/pull/557
	- Protocol: `--enable-experimental-features` support gossip query extensions proposal aka https://github.com/lightningnetwork/lightning-rfc/pull/557

Conversation

rustyrussell commented Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdecker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niftynei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustyrussell commented Aug 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rustyrussell commented Aug 5, 2019 •

edited

Loading