feature: add transitive feature bit validation#3834
feature: add transitive feature bit validation#3834cfromknecht merged 4 commits intolightningnetwork:masterfrom
Conversation
joostjager
left a comment
There was a problem hiding this comment.
As mentioned in the parent PR: I think it wouldn't have been necessary to make the dep checker as luxurious and optimized as it is now for the stage we are in. I would have been happy with two if staments too. But the work has already been done, so let's go ahead with it.
My main question is whether we need to validate features also at the remaining entry points. Two that are missing are:
- paying an invoice
- node announcement gossip
c54606e to
9b03f64
Compare
Paying an invoice should be covered since we validate payreq features while decoding. Even if the invoice is paid manually, this is checked during Node announcements are a little trickier, but we could start to validate them when we receive them. We will still have older announcements that will be grandfathered into our routing tables even if they are invalid. This can also happen if we update our dependencies that retroactively makes announcements invalid. For these reasons, I went with just validating them in the router as they're being used, but maybe there is a better long term strategy. Do you think we should add the dependency validation on new node anns? Perhaps it makes sense, there's no reason to store them if we will end up failing to build a path/route with them. |
|
Hmm, yes the stored announcements are a problem. Even if we start treating new ones differently, we still can't leave out the check for the old ones. Probably best then to leave it as you do it now, when we use them. |
9b03f64 to
2c234a8
Compare
joostjager
left a comment
There was a problem hiding this comment.
Changes to the dep checker may seem small, but very helpful in understanding the logic. Nice.
halseth
left a comment
There was a problem hiding this comment.
Few nits, otherwise LGTM 👍
2c234a8 to
34fd272
Compare
Split off from #3679
This PR adds a new
feature.ValidateDepsmethod to thefeaturepackage, which can be used to ensure that all transitive dependencies of a given feature are advertised in the feature vector. With base amp, for example, the dependency chain isoption_mpp->option_payment_addr->option_onion_tlv. When validating, we want to fail ifoption_mppis set but either of two dependencies are not. As more feature bits are added, these chains could become several layers deep. Things like AMP or extended gossip queries will continue to add to these constraints, and this gives a simple way of expressing and validating these relations.We then add this transitive validation to remote init features and when parsing BOLT11 invoices, additionally verifying that the payment address is set if the corresponding feature bit is advertised.