09+11: require transitive feature dependencies#719
Merged
cdecker merged 4 commits intolightning:masterfrom Jan 21, 2020
Merged
Conversation
t-bast
reviewed
Dec 17, 2019
ddab573 to
ed9ad50
Compare
TheBlueMatt
reviewed
Jan 6, 2020
01-messaging.md
Outdated
| - MUST ignore the bit. | ||
| - upon receiving unknown _even_ feature bits that are non-zero: | ||
| - MUST fail the connection. | ||
| - MUST validate all known, transitive feature dependencies. |
Collaborator
There was a problem hiding this comment.
MUST validate doesn't mean anything - should I disconnect? Should I ignore the message? Should I....
Collaborator
Author
There was a problem hiding this comment.
oops, forgot the most important part, thanks! both have been updated
TheBlueMatt
reviewed
Jan 6, 2020
11-payment-encoding.md
Outdated
| - MUST set `var_onion_optin` if and only if it supports that feature. | ||
|
|
||
| A reader: | ||
| - MUST validate all known, transitive feature dependencies before paying. |
niftynei
reviewed
Jan 6, 2020
ed9ad50 to
06bc5e3
Compare
06bc5e3 to
294499b
Compare
6b51059 to
b9892d0
Compare
t-bast
approved these changes
Jan 16, 2020
Collaborator
t-bast
left a comment
There was a problem hiding this comment.
LGTM, and reducing spec size is always a win ;)
b9892d0 to
65291f8
Compare
t-bast
approved these changes
Jan 17, 2020
Collaborator
t-bast
left a comment
There was a problem hiding this comment.
Thanks @cfromknecht, ACK 65291f8
This commit: - Adds a new Dependencies column to the BOLT 9 feature table populated with existing feature dependencies. - Requires that all valid feature vectors set transitive dependencies. - Requires checking transitive dependencies when validating init messages and payment request. - Removes transitive feature requiremetns from the BOLT 11 writer, now that they are implicit by needing to comply with the BOLT 9 origin requirements.
As a final step, we now can remove several of the BOLT 11 writer's requirements now that it builds on BOLT 9's, particularly: - setting the even bit if a feature is required. - only setting a feature if the node supports a given feature. The lone requirement that remains pertains to setting the `s` value if and only if the `payment_secret` feature is set.
65291f8 to
3ebb315
Compare
t-bast
approved these changes
Jan 20, 2020
Collaborator
|
Merging following the spec meeting on 2020/01/20 |
SomberNight
added a commit
to SomberNight/electrum
that referenced
this pull request
Mar 16, 2020
This implements: - flat feature bits lightning/bolts#666 - feature bit dependencies lightning/bolts#719
SomberNight
added a commit
to SomberNight/electrum
that referenced
this pull request
Mar 17, 2020
This implements: - flat feature bits lightning/bolts#666 - feature bit dependencies lightning/bolts#719
SomberNight
added a commit
to SomberNight/electrum
that referenced
this pull request
Mar 17, 2020
This implements: - flat feature bits lightning/bolts#666 - feature bit dependencies lightning/bolts#719
SomberNight
added a commit
to SomberNight/electrum
that referenced
this pull request
Mar 24, 2020
This implements: - flat feature bits lightning/bolts#666 - feature bit dependencies lightning/bolts#719
SomberNight
added a commit
to SomberNight/electrum
that referenced
this pull request
Apr 1, 2020
This implements: - flat feature bits lightning/bolts#666 - feature bit dependencies lightning/bolts#719
sidhujag
pushed a commit
to syscoin/electrumsys
that referenced
this pull request
Apr 2, 2020
This implements: - flat feature bits lightning/bolts#666 - feature bit dependencies lightning/bolts#719
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds an additional column to our feature table in BOLT 09, enumerating any dependencies that a feature pair has. At the moment, there are 3 such pairs that have dependencies, and more will be added in the near term. The motivation is to offer a concise way to describe these relations, and also simplify how we specify behavior gated by features with dependencies.
Consider the
basic_mppfeature, which (as of #712) depends onpayment_secretand transitively depends onvar_onion_optin. Instead of specifying (or coding)has(basic_mpp) && has(payment_secret) && has(var_onion_optin)everywhere we need to gatebasic_mpp, this allows us to isolate the gate to onlybasic_mppsince the transitive requirements are assumed to be already validated..An implementation of this validation can be found here: https://github.com/lightningnetwork/lnd/blob/master/feature/deps.go which we are currently using as we build out MPP to simplify our own logic.
The test vectors have also been updated to set the
var_onion_optinfeature when setting a payment secret, as @t-bast points out, the current ones are missing this bit to be fully aligned with the existing spec.