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

Reply channel range simplification #737

Merged

Conversation

rustyrussell
Copy link
Collaborator

First a trivial renaming of the confusing complete field.

Then, we insist that query_channel_range replies be in order (everyone now does this anyway) and non-overlapping (ditto: c-lightning had strict enforcement on this, which was overzealous but means everyone now does it that way). Currently c-lightning maintains a bitmap for every block it requested, but it's much simpler to assume ordered replies, and simply check if we received the final one.

…onale

This was confusing: the flag name made implementers *think* they
knew what it was for.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
The current loose constraints causes confusion (and now all major
implementations meet the stricter requirements anyway).

You are allowed to provide more blocks than requested, but you have
to be complete and in order, and each reply has to have some overlap
with the requested range.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the reply_channel_range-simplification branch from a769e08 to c950d4e Compare February 3, 2020 23:50
07-routing-gossip.md Outdated Show resolved Hide resolved
07-routing-gossip.md Show resolved Hide resolved
Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 83564cf.

I checked that this is the behavior of eclair 0.3.3, so we're good on the eclair side.

@t-bast
Copy link
Collaborator

t-bast commented Mar 27, 2020

@wpaulino could you take a look at this? And share your thoughts on the fact that this PR disallows splitting a block's content over two replies?

Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wpaulino could you take a look at this? And share your thoughts on the fact that this PR disallows splitting a block's content over two replies?

I agree with @rustyrussell that we can't detect the last reply properly if we allow overlapping heights. There may be a future where the number of channels created in a block exceed the size of a single uncompressed reply message, but we also have the compressed option which is more than enough.

- SHOULD set `complete` to 1.
- SHOULD set `full_information` to 1.
- the first `reply_channel_range` message:
- MUST set `first_blocknum` less than or equal to the `first_blocknum` in `query_channel_range`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should just be equal?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the rationale between this "less than or equal" is to allow pre-generated responses that may start a few blocks before the first_blocknum.

Imagine for example that blocks 3, 4, 5 and 6 fit in a pre-generated response. If you ask for first_blocknum = 5 instead of generating a response that matches exactly that, I can give you my pre-generated response that contains blocks 3, 4, 5 and 6 and save some computing power. And you have the results you wanted so you should be happy with that.

This is extra flexibility that doesn't cost much, so it's worth having IMHO.

@t-bast t-bast merged commit d4bafcb into lightning:master Mar 31, 2020
@t-bast
Copy link
Collaborator

t-bast commented Mar 31, 2020

Merging as agreed during yesterday's meeting: http://www.erisian.com.au/meetbot/lightning-dev/2020/lightning-dev.2020-03-30-19.06.log.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants