lnutil: bolt 12 LnFeatures changes#10612
lnutil: bolt 12 LnFeatures changes#10612f321x wants to merge 5 commits intospesmilo:masterspesmilo/electrum:masterfrom f321x:bolt12_lnutil_additionsf321x/electrum-fork:bolt12_lnutil_additionsCopy head branch name to clipboard
Conversation
| for flag in our_flags: | ||
| if flag not in their_flags and get_ln_flag_pair_of_bit(flag) not in their_flags: | ||
| if flag not in their_flags_effective and get_ln_flag_pair_of_bit(flag) not in their_flags_effective: |
There was a problem hiding this comment.
This would mean that even if the remote node did not signal e.g. option_dataloss_protect in their INIT message, we just ignore that, assume they support it, and proceed as usual. I find this behaviour surprising and have some doubts this is what the spec really means by "ASSUMED".
There was a problem hiding this comment.
Ok according to this PR comment, iiuc, the intention of marking features ASSUMED really seems to be to just assume they are there and, at one point, ignore them completely (as bolt 9 says).
However in practice this doesn't seem to be done by implementations, and e.g. sending INIT without an assumed feature flag would not be accepted anywhere. ASSUMED seems to mean more like "always required to be set" in practice.
Maybe spec compliant thing would be to ignore them when validating features but keep signaling them forever for compatibility. But then, there are still nodes that don't support these features, so it is probably not safe to just ignore them.
For now i removed this assumed thing from the PR. Instead to get the bolt 12 feature validation working i rely on context dependent transitive dependencies, which seems conceptually cleaner than relying on assumed features anyway.
There was a problem hiding this comment.
Does the spec plan to eventually reuse these feature bits? :D
This whole approach to "ASSUMED" just seems weird, and more trouble than is worth.
The reasonable thing would be to just burn the feature bits and require them to always be signalled.
There was a problem hiding this comment.
Does the spec plan to eventually reuse these feature bits? :D
From my understanding of this comment it seems like a goal of the change is to eventually be able to fully remove these assumed feature bits from the spec. Once there is no reference to them anymore i assume they could also be reused:
This way, we'll eventually end up in a state where we can really assume those features and maybe even remove references to the feature bits they used (but maybe not 🤷). Meanwhile, we don't want to completely remove from the spec references to the fact that these features were at some point no assumed, and may or may not be supported.
3e4c4d6 to
f49030b
Compare
Renames the "INVOICE" lightning feature context to "BOLT11_INVOICE" as there is now a bolt 12 invoice feature context as well that differs. Also adds 4 new bolt 12 context to the Enum.
Make transitive dependencies of lightning features context dependent. This allows to validate different transitive dependencies for the same feature in different contexts. For example BASIC_MPP_* depends on payment_secret for bolt 11 invoices, but has no dependencies when being used in the context of bolt 12 invoices.
Add missing LNWALLET_FEATUERS features to LN_FEATURES_IMPLEMENTED.
f49030b to
2586c57
Compare
Changes to
LnFeaturesrequired for bolt 12.to_tlv_byteshelper methodBASIC_MPP_*withoutpayment_secretdep)