Move supported-feature-set logic into the supporting modules #1707

TheBlueMatt · 2022-09-09T04:45:13Z

Historically, LDK has considered the "set of known/supported
feature bits" to be an LDK-level thing. Increasingly this doesn't
make sense - different message handlers may provide or require
different feature sets.

In a previous PR, we began the process of transitioning with
feature bits sent to peers being sourced from the attached message
handler.

This PR makes further progress by moving the concept of which
feature bits are supported by our various modules directly into those modules.

It then has a long series of commits which remove all usages of the *Features::known constructor (almost all of which are in tests). Finally, it removes the *Features::known constructor entirely and moves the features context bit tracking set to include all defined feature bits, rather than the set of supported/required bits.

As a final followup, we should move the "is the peer connecting to me not setting bits which one of my message handlers set to required" checks to be via the handlers/provided_init_features rather than via the requires_unknown_bits check.

jkczyz · 2022-09-09T16:18:18Z

lightning/src/ln/channelmanager.rs

+pub fn provided_node_features() -> NodeFeatures {
+	provided_init_features().to_context()
+}
+
+/// Fetches the set of [`InvoiceFeature`] flags which are provided by or required by
+/// [`ChannelManager`].
+pub fn provided_invoice_features() -> InvoiceFeatures {
+	provided_init_features().to_context()
+}
+
+/// Fetches the set of [`ChannelFeature`] flags which are provided by or required by
+/// [`ChannelManager`].
+pub fn provided_channel_features() -> ChannelFeatures {
+	provided_init_features().to_context()
+}


Thoughts on having one method?

pub fn provided_features<T: From<InitFeatures>>() -> T { ... }

Not sure if having it based on InitFeatures would be weird.

Hmm, yea, From would be weird, I guess in theory we could do it for all contexts by just bounding by a Feature<C: Context>? I'm not 100% sure if we wouldn't want to at some point have slightly different features in different contexts? I don't immediately see a reason why we would, so can change it unless we come up with some reason.
Oh, nevermind, you even point out where we may change this in #1707 (comment) so we cant.

jkczyz · 2022-09-09T16:20:50Z

lightning/src/ln/peer_handler.rs

+		let mut features = InitFeatures::empty();
+		features.set_data_loss_protect_optional();
+		features.set_upfront_shutdown_script_optional();
+		features.set_variable_length_onion_optional();
+		features.set_static_remote_key_optional();
+		features.set_payment_secret_optional();
+		features.set_basic_mpp_optional();
+		features.set_wumbo_optional();
+		features.set_shutdown_any_segwit_optional();
+		features.set_channel_type_optional();
+		features.set_scid_privacy_optional();
+		features.set_zero_conf_optional();
+		features


Should we call the one version in channelmanager.rs instead of duplicating?

They're different - this only ever sets optional bits, never required ones.

Would be nice to have simple test that each feature in channelmanager::provided_init_features is supported here, even if not required. Not sure if there is an easy way to do that. Otherwise, a comment on the former to update this may be in order.

Hmmmmmmmmmmmm. Our current features code doesn't really have any kind of logic for iterating feature flags themselves. We could iterate the bits themselves, but it doesn't seem worth it to expose them for this? I'll leave a comment at least.

jkczyz · 2022-09-09T16:42:18Z

lightning/src/ln/channelmanager.rs

+/// Fetches the set of [`InvoiceFeature`] flags which are provided by or required by
+/// [`ChannelManager`].
+pub fn provided_invoice_features() -> InvoiceFeatures {
+	provided_init_features().to_context()


Do we care that invoice-only features like option_payment_metadata won't be returned?

Presumably when we support payment metadata for receiving we'll want to set that, but we don't support receiving with metadata today.

jkczyz · 2022-09-09T16:49:06Z

lightning/src/ln/channelmanager.rs

+
+/// Fetches the set of [`InvoiceFeature`] flags which are provided by or required by
+/// [`ChannelManager`].
+pub fn provided_invoice_features() -> InvoiceFeatures {


Should this be test-only?

No? lightning-invoice requests these flags to set in the invoices we construct, or should.

InvoiceBuilder has methods for setting individual features (or sets of features) along with any necessary data needed in the invoice for the feature. A user never passes InvoiceFeatures directly. This allows compile-time enforcement of correctness.

For example InvoiceBuilder::payment_secret sets the supplied PaymentSecret and the necessary feature bits. It also makes InvoiceBuilder::basic_mpp available.

Looks like test_builder_ok caught this because InvoiceBuilder::payment_secret sets dependent feature variable_length_onion.

Ah, right, I suppose if we ever have a feature bit in the invoice which doesn't require some additional data in the invoice we'd need this. In any case, I don't see any harm in leaving it public.

Hmm.... maybe something else is going on there. Looking.

Ok, it just looks like test_builder_ok is not manually setting variable_length_onion. It isn't using provided_invoice_features.

Ah, right, I suppose if we ever have a feature bit in the invoice which doesn't require some additional data in the invoice we'd need this. In any case, I don't see any harm in leaving it public.

We already have that with basic_mpp, but it's just a separate builder method. I don't think we should allow users to pass in InvoiceFeatures directly. Maybe would need to set an unknown bit, but in that case we can provide a method to do so.

I don't think we should allow users to pass in InvoiceFeatures directly.

Seems like something that is going to depend on the types of features. If we end up with some feature which doesn't require additional data, we'll need this.

I kinda wonder if we should have some kind of "check features are X" method - require that users pass the ChannelManager's invoice features set to the InvoiceBuilder so that the InvoiceBuilder can check that the required features have all been filled in.

In any case, I realized that the invoice features we need vary anyway - if its a phantom invoice we cant set MPP - so I went ahead and made it test-cfg.

I kinda wonder if we should have some kind of "check features are X" method - require that users pass the ChannelManager's invoice features set to the InvoiceBuilder so that the InvoiceBuilder can check that the required features have all been filled in.

InvoiceBuilder::build_signed returns Result<Invoice, CreationError>, so it could perform checks against any features, provided either at builder construction time or at invoice building time using a dedicated method.

jkczyz · 2022-09-09T18:52:20Z

lightning/src/ln/features.rs

+					const ASSERT_BITS_IN_MASK: u8 =
+						((<$context>::KNOWN_FEATURE_MASK[<Self as $feature>::BYTE_OFFSET] & (<Self as $feature>::REQUIRED_MASK | <Self as $feature>::OPTIONAL_MASK))
+						 >> (<Self as $feature>::EVEN_BIT % 8)) - 3;


Looks like UnknownFeature doesn't work so well with this.

TheBlueMatt · 2022-09-12T19:58:52Z

Actually, its simpler to handle the followup there first, see #1717

valentinewallace · 2022-09-13T20:18:39Z

Needs rebase

TheBlueMatt · 2022-09-14T01:40:58Z

Rebased, now depends on #1720

codecov-commenter · 2022-09-14T02:00:46Z

Codecov Report

Base: 90.81% // Head: 90.76% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (fab768a) compared to base (58f76f2).
Patch coverage: 96.32% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1707      +/-   ##
==========================================
- Coverage   90.81%   90.76%   -0.05%     
==========================================
  Files          86       86              
  Lines       46670    46631      -39     
  Branches    46670    46631      -39     
==========================================
- Hits        42382    42325      -57     
- Misses       4288     4306      +18

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`96.94% <ø> (-0.12%)`	⬇️
lightning/src/ln/peer_handler.rs	`55.96% <0.00%> (-0.52%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.73% <50.00%> (ø)`
lightning/src/util/test_utils.rs	`76.90% <66.66%> (ø)`
lightning/src/ln/functional_test_utils.rs	`93.44% <88.88%> (ø)`
lightning/src/routing/router.rs	`92.06% <96.29%> (ø)`
lightning-background-processor/src/lib.rs	`96.23% <100.00%> (ø)`
lightning-invoice/src/lib.rs	`87.48% <100.00%> (+0.09%)`	⬆️
lightning-invoice/src/payment.rs	`90.86% <100.00%> (ø)`
lightning-invoice/src/utils.rs	`96.77% <100.00%> (ø)`
... and 21 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

jkczyz

LGTM with a possible test addition.

jkczyz · 2022-09-14T15:24:29Z

lightning/src/ln/peer_handler.rs

+		let mut features = InitFeatures::empty();
+		features.set_data_loss_protect_optional();
+		features.set_upfront_shutdown_script_optional();
+		features.set_variable_length_onion_optional();
+		features.set_static_remote_key_optional();
+		features.set_payment_secret_optional();
+		features.set_basic_mpp_optional();
+		features.set_wumbo_optional();
+		features.set_shutdown_any_segwit_optional();
+		features.set_channel_type_optional();
+		features.set_scid_privacy_optional();
+		features.set_zero_conf_optional();
+		features


Would be nice to have simple test that each feature in channelmanager::provided_init_features is supported here, even if not required. Not sure if there is an easy way to do that. Otherwise, a comment on the former to update this may be in order.

lightning/src/ln/peer_handler.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/features.rs

TheBlueMatt · 2022-09-14T18:23:50Z

Rebased and addressed feedback.

lightning/src/ln/features.rs

Historically, LDK has considered the "set of known/supported feature bits" to be an LDK-level thing. Increasingly this doesn't make sense - different message handlers may provide or require different feature sets. In a previous PR, we began the process of transitioning with feature bits sent to peers being sourced from the attached message handler. This commit makes further progress by moving the concept of which feature bits are supported by our ChannelManager into channelmanager.rs itself, via the new `provided_*_features` methods, rather than in features.rs via the `known_channel_features` and `known` methods.

As we move towards specify supported/required feature bits in the module(s) where they are supported, the global `known` feature set constructors no longer make sense. Here we stop relying on the `known` method in the `routing` module, which was only used in tests.

This diff is commit, like the last, stops relying on the `known` feature set constructor, doing so entirely with import changes and sed rules.

As we move towards specify supported/required feature bits in the module(s) where they are supported, the global `known` feature set constructors no longer make sense. Here we stop relying on the `known` method in the functional_test_utils module.

As we move towards specify supported/required feature bits in the module(s) where they are supported, the global `known` feature set constructors no longer make sense. Here we stop relying on the `known` method in the channel modules.

As we move towards specify supported/required feature bits in the module(s) where they are supported, the global `known` feature set constructors no longer make sense. Here we stop relying on the `known` method in the `lightning-invoice` crate.

As we move towards specify supported/required feature bits in the module(s) where they are supported, the global `known` feature set constructors no longer make sense. Here we stop relying on the `known` method in the `lightning-net-tokio` crate.

As we move towards specify supported/required feature bits in the module(s) where they are supported, the global `known` feature set constructors no longer make sense. Here we stop relying on the `known` method in the `lightning-background-processor` and `lightning-persister` crate tests.

As we move towards specify supported/required feature bits in the module(s) where they are supported, the global `known` feature set constructors no longer make sense. Here we stop relying on the `known` method in our fuzz tests.

As we move towards specify supported/required feature bits in the module(s) where they are supported, the global `known` feature set constructors no longer make sense. In anticipation of removing the `known` constructor, this commit removes all remaining references to it outside of features.rs.

As we move towards specify supported/required feature bits in the module(s) where they are supported, the global `known` feature set constructors no longer make sense. Here we (finally) remove the `known` constructor entirely, modifying tests in the `features` module as required.

Now that the `*Features::known` constructor has been removed, there is no reason to define feature bits as either optional required in `features.rs` - that logic now belongs in the modules that are responsible for the given features. Instead, we only list all features in each context.

Now that the features contexts track the full set of all known features, rather than the set of supported features, all defined features should be listed in the context definition macro. This adds a compile-time assertion to check that all bits for known features are set in the context known set.

TheBlueMatt · 2022-09-14T20:10:57Z

FIxed last comment, one bench issue, and squashed. Diff is:

$ git diff-tree -U1 fab768ae5 9170804ca
diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs
index da26cb0d5..b372e6af6 100644
--- a/lightning/src/ln/features.rs
+++ b/lightning/src/ln/features.rs
@@ -747,3 +747,3 @@ mod tests {
 		// Set a bunch of features we use, plus initial_routing_sync_required (which shouldn't get
-		// converted as its only relevant in an init context).
+		// converted as it's only relevant in an init context).
 		init_features.set_initial_routing_sync_required();
diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index 4d47170d1..801acaa2e 100644
--- a/lightning/src/routing/router.rs
+++ b/lightning/src/routing/router.rs
@@ -5928,3 +5928,3 @@ mod benches {
 	use chain::keysinterface::{KeysManager,KeysInterface};
-	use ln::channelmanager::{ChannelCounterparty, ChannelDetails};
+	use ln::channelmanager::{self, ChannelCounterparty, ChannelDetails};
 	use ln::features::{InitFeatures, InvoiceFeatures};

valentinewallace

ACK if CI passes

TheBlueMatt mentioned this pull request Sep 9, 2022

Fetch InitFeatures from both Channel and Routing Message Handlers #1701

Merged

TheBlueMatt force-pushed the 2022-09-no-global-features branch from e31571a to d4a1bc5 Compare September 9, 2022 04:46

jkczyz self-requested a review September 9, 2022 15:42

TheBlueMatt force-pushed the 2022-09-no-global-features branch from d4a1bc5 to 1087e93 Compare September 9, 2022 16:51

jkczyz reviewed Sep 9, 2022

View reviewed changes

TheBlueMatt mentioned this pull request Sep 9, 2022

Fix several compile warnings added in some of my recent commits #1710

Merged

TheBlueMatt force-pushed the 2022-09-no-global-features branch from 1087e93 to 184d221 Compare September 12, 2022 19:56

TheBlueMatt added the blocked on dependent pr label Sep 12, 2022

valentinewallace removed the blocked on dependent pr label Sep 13, 2022

TheBlueMatt force-pushed the 2022-09-no-global-features branch from 184d221 to 374f6fe Compare September 14, 2022 01:40

TheBlueMatt added the blocked on dependent pr label Sep 14, 2022

TheBlueMatt force-pushed the 2022-09-no-global-features branch from 374f6fe to 9f74638 Compare September 14, 2022 02:05

jkczyz previously approved these changes Sep 14, 2022

View reviewed changes

valentinewallace self-requested a review September 14, 2022 15:41

valentinewallace reviewed Sep 14, 2022

View reviewed changes

lightning/src/ln/peer_handler.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/features.rs Outdated Show resolved Hide resolved

lightning/src/ln/features.rs Show resolved Hide resolved

TheBlueMatt dismissed jkczyz’s stale review via fab768a September 14, 2022 18:23

TheBlueMatt force-pushed the 2022-09-no-global-features branch from 9f74638 to fab768a Compare September 14, 2022 18:23

jkczyz previously approved these changes Sep 14, 2022

View reviewed changes

valentinewallace previously approved these changes Sep 14, 2022

View reviewed changes

lightning/src/ln/features.rs Outdated Show resolved Hide resolved

TheBlueMatt added 5 commits September 14, 2022 20:08

Stop relying on the *Features::known method in functional tests

3b3713f

This diff is commit, like the last, stops relying on the `known` feature set constructor, doing so entirely with import changes and sed rules.

TheBlueMatt added 8 commits September 14, 2022 20:09

Stop relying on *Features::known in fuzzing tests

f6fa624

As we move towards specify supported/required feature bits in the module(s) where they are supported, the global `known` feature set constructors no longer make sense. Here we stop relying on the `known` method in our fuzz tests.

TheBlueMatt dismissed stale reviews from valentinewallace and jkczyz via 9170804 September 14, 2022 20:10

TheBlueMatt force-pushed the 2022-09-no-global-features branch from fab768a to 9170804 Compare September 14, 2022 20:10

valentinewallace approved these changes Sep 14, 2022

View reviewed changes

valentinewallace removed the blocked on dependent pr label Sep 14, 2022

jkczyz approved these changes Sep 14, 2022

View reviewed changes

TheBlueMatt merged commit 48d21ba into lightningdevkit:main Sep 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move supported-feature-set logic into the supporting modules #1707

Move supported-feature-set logic into the supporting modules #1707

TheBlueMatt commented Sep 9, 2022

jkczyz Sep 9, 2022

TheBlueMatt Sep 9, 2022 •

edited

Loading

jkczyz Sep 9, 2022

TheBlueMatt Sep 9, 2022

jkczyz Sep 14, 2022

TheBlueMatt Sep 14, 2022

jkczyz Sep 9, 2022

TheBlueMatt Sep 9, 2022

jkczyz Sep 9, 2022

TheBlueMatt Sep 9, 2022

jkczyz Sep 9, 2022

jkczyz Sep 9, 2022

TheBlueMatt Sep 9, 2022

jkczyz Sep 9, 2022

jkczyz Sep 9, 2022

jkczyz Sep 9, 2022

TheBlueMatt Sep 12, 2022 •

edited

Loading

jkczyz Sep 14, 2022 •

edited

Loading

jkczyz Sep 9, 2022

TheBlueMatt commented Sep 12, 2022

valentinewallace commented Sep 13, 2022

TheBlueMatt commented Sep 14, 2022

codecov-commenter commented Sep 14, 2022 •

edited

Loading

jkczyz left a comment

jkczyz Sep 14, 2022

TheBlueMatt commented Sep 14, 2022

TheBlueMatt commented Sep 14, 2022

valentinewallace left a comment

Move supported-feature-set logic into the supporting modules #1707

Move supported-feature-set logic into the supporting modules #1707

Conversation

TheBlueMatt commented Sep 9, 2022

Choose a reason for hiding this comment

TheBlueMatt Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

jkczyz Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Sep 12, 2022

valentinewallace commented Sep 13, 2022

TheBlueMatt commented Sep 14, 2022

codecov-commenter commented Sep 14, 2022 • edited Loading

Codecov Report

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Sep 14, 2022

TheBlueMatt commented Sep 14, 2022

valentinewallace left a comment

Choose a reason for hiding this comment

TheBlueMatt Sep 9, 2022 •

edited

Loading

TheBlueMatt Sep 12, 2022 •

edited

Loading

jkczyz Sep 14, 2022 •

edited

Loading

codecov-commenter commented Sep 14, 2022 •

edited

Loading