Support for custom feature bits #2204

jkczyz · 2023-04-19T23:26:31Z

Each CustomMessageHandler may support custom Features and thus those must be included in the Init message. Update the trait to include a way to advertise these. Additionally, they must be taken account when checking if our peer requires unknown features (i.e., it is ok if both we and our peer support the same "unknown" features).

Also adds support for setting custom feature bits so that CustomMessageHandler implementations can set them.

Fixes #2131

codecov-commenter · 2023-04-20T00:33:52Z

Codecov Report

Patch coverage: 91.44% and project coverage change: -0.16 ⚠️

Comparison is base (818dbdf) 91.50% compared to head (14a41d6) 91.34%.

❗ Current head 14a41d6 differs from pull request most recent head 432d180. Consider uploading reports for the commit 432d180 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2204      +/-   ##
==========================================
- Coverage   91.50%   91.34%   -0.16%     
==========================================
  Files         104      102       -2     
  Lines       52087    54840    +2753     
  Branches    52087    54840    +2753     
==========================================
+ Hits        47660    50092    +2432     
- Misses       4427     4748     +321

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	`91.98% <ø> (+4.75%)`	⬆️
lightning/src/ln/peer_handler.rs	`63.65% <84.00%> (+1.07%)`	⬆️
lightning/src/ln/features.rs	`98.92% <98.68%> (+0.46%)`	⬆️
lightning/src/ln/msgs.rs	`84.65% <100.00%> (-4.06%)`	⬇️

... and 73 files with indirect coverage changes

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

alecchendev

Nice!

alecchendev · 2023-04-26T01:53:18Z

lightning/src/ln/features.rs

+		for (byte, o_byte) in self.flags.iter_mut().zip(o.flags.iter()) {
+			*byte &= !*o_byte;
+		}


Just noting here I was confused by this at first but I realized Features::flags are little endian so this makes sense 👍

alecchendev · 2023-04-26T01:57:43Z

lightning/src/ln/features.rs

+		let last_non_zero_byte = self.flags.iter().rposition(|&byte| byte != 0);
+		let size = if let Some(offset) = last_non_zero_byte { offset + 1 } else { 0 };
+		self.flags.resize(size, 0u8);


Don't think this is covered by tests, figure it'd probably be easy to if you weren't planning on it already :)

Good catch!

alecchendev · 2023-04-26T02:40:34Z

lightning/src/ln/features.rs

+	/// Sets a custom feature bit. Errors if `bit` is outside the custom range as defined by
+	/// [bLIP 2] or if it is a known `T` feature.


What is a known T feature bit that's outside the reserved 0-255 range for BOLTs? Would it be something like feature bits documents in blips like hosted_channels?

I believe it's just a custom feature bit that LDK explicitly knows about and would already handle.

rust-lightning/lightning/src/ln/features.rs

Lines 86 to 92 in 72b51ca

/// The context in which [`Features`] are applicable. Defines which features are known to the

/// implementation, though specification of them as required or optional is up to the code

/// constructing a features object.

pub trait Context {

/// Bitmask for selecting features that are known to the implementation.

const KNOWN_FEATURE_MASK: &'static [u8];

}

Which would probably be already documented as a bLIP, yeah

alecchendev · 2023-04-26T02:42:52Z

lightning/src/ln/features.rs

+		let known_bit = <sealed::InvoiceContext as sealed::PaymentSecret>::EVEN_BIT;
+		let byte_offset = <sealed::InvoiceContext as sealed::PaymentSecret>::BYTE_OFFSET;
+		assert_eq!(byte_offset, 1);
+		assert_eq!(features.flags[byte_offset], 0b00000010);
+		assert!(features.set_custom_bit(known_bit).is_err());
+		assert_eq!(features.flags[byte_offset], 0b00000010);


Does this test anything new since <sealed::InvoiceContext as sealed::PaymentSecret>::EVEN_BIT = 14 which gets caught by the initial <256 check in set_custom_bit?

This checks that if set_custom_bit errors then the feature bit remain unchanged.

TheBlueMatt · 2023-04-27T18:16:03Z

lightning/src/ln/features.rs

+	/// [bLIP 2] or if it is a known `T` feature.
+	///
+	/// [bLIP 2]: https://github.com/lightning/blips/blob/master/blip-0002.md#feature-bits
+	pub fn set_custom_bit(&mut self, bit: usize) -> Result<(), ()> {


Should this have a required/optional version and just deduce the bit by &ing with 2?

From review club: or validate that either optional or required is set, not both.

Should this have a required/optional version and just deduce the bit by &ing with 2?

Hmm... I'm not sure I follow how that's useful or how that would even work for that matter (i.e., shift + AND 0b10 would either set no bits or the odd bit). And the user has to pass in something. Would they be expected to pass in an even or an odd bit? Seems error prone whereas explicitly giving the bit to set isn't. Maybe I'm missing something here.

Ok yeah I can't really see what would be an appropriate input in each case. I don't think there's a way of making it seems obvious/natural other than just providing the bit to set.

Oops, missed the above comment. We'd basically mod the bit selected by two (and add one for optional), but the issue with the current API is its not at all clear whether we're setting a required or optional bit in the custom-bit-setting logic, but we do use the required/optional distinction in the connection checking logic (and across the codebase). Even if we don't split the method for convenience, we definitely need to carefully document the even/odd distinction here (which feels more verbose than just two methods, but whatever).

I don't quite get it. Are you saying we'd have two methods -- one for required and one for optional -- where two different inputs for each would result in the same value being set? (e.g., giving the required setter an odd bit would set the even bit?) Seems either way the caller needs to pass some bit, so how do they decide what to pass?

Yes, I'm suggesting mapping the bit they pass to the "correct" one to the optional/required bit for that bit-pair. Its definitely awkward, but forcing the user to consider the even/odd thing manually seems just as awkward if not more?

So for example, if the user gives an odd or even bit from the feature's bit-pair to set_required_custom_bit() it should just set the corresponding even bit of the pair? I think as long as that little awkwardness is also documented then it's fine.

alecchendev · 2023-04-27T23:06:25Z

lightning/src/ln/peer_handler.rs

+	/// which are sent in our [`Init`] message.
+	///
+	/// [`Init`]: crate::ln::msgs::Init
+	fn provided_init_features(&self, their_node_id: &PublicKey) -> InitFeatures;


Mentioned it briefly in review club - I'm wondering why there's only provided_x_features for node and init in the various message handlers and no provided_channel_features? I see in the channelmanager module there's provided_channel_features but it just wraps provided_init_features with the user config. Is this just because right now there aren't much use for the channel announcement message to communicate features?

Possibly but will let @TheBlueMatt confirm.

The ChannelFeatures are channel-specific - they describe things like "does the channel support PLTCs, is the channel taproot, etc", they're not something that's generic for a handler. Thus, they're not useful outside of the specific channel announcement (generated by the channelmanager) ie not useful in the peermanager.

Ah gotcha, thanks

TheBlueMatt

Needs a small rebase, one comment, otherwise LGTM.

TheBlueMatt · 2023-05-03T22:36:27Z

lightning/src/ln/peer_handler.rs

@@ -1323,7 +1346,8 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM

 		// Need an Init as first message
 		if let wire::Message::Init(msg) = message {
-			if msg.features.requires_unknown_bits() {
+			let additional_features = msg.features.clone() - self.init_features(&their_node_id);


Is there a reason to do this via clone + subtract, vs just having a requires_unknown_bits_not_in() method?

Mostly to be able to reuse requires_unknown_bits.

Mmm, right, so this is confusing and buggy already today. Like, if we have a set of handlers that don't set, for example, data_loss_protect, but if our peer requires it we'll continue happily here (though our peer should disconnect us). Further, if we do require an unknown bit, and our peer doesn't support it, we'll happily continue talking to them. Instead, we should really check both ways for any required feature, not just required uknown-to-LDK features.

Yeah, it seems requires_unknown_bits is obsolete given it uses KNOWN_FEATURE_MASK. A set of handlers may exclude a known feature. If the peer requires the feature, the method would return false instead of true. We really want to use a mask specific to the features supported by the handlers.

Ok, I think this is implemented correctly now. PTAL. I can remove the set difference code when I squash the fixups.

TheBlueMatt

LGTM, feel free to squash + rebase imo.

lightning/src/ln/features.rs

The `lightning-custom-message` crate will need access to Features::or in order combine features of a composite handler. Expose this via a core::ops::BitOr implementation.

CustomMessageHandler implementations may need to advertise support for features. Add methods to CustomMessageHandler to provide these and combine them with features from other message handlers.

When checking features, rather than checking against which features LDK knows about, it is more useful to check against a peer's features. Add Features::requires_unknown_bits_from such that the given features are used instead.

jkczyz · 2023-05-08T15:34:07Z

LGTM, feel free to squash + rebase imo.

Done

TheBlueMatt · 2023-05-08T17:17:11Z

Oops, sorry, missed your above comment, lgtm aside from #2204 (comment)

lightning/src/ln/peer_handler.rs

TheBlueMatt · 2023-05-10T16:27:21Z

LGTM, at least glancing ta the fixup commits, feel free to squash IMO.

Each message handler provides which features it supports. A custom message handler may support unknown features. Therefore, these features should be checked against instead of the features known by LDK. Additionally, fail the connection if the peer requires features unknown to the handler. The peer should already fail the connection in the latter case.

Custom message handlers may need to set feature bits that are unknown to LDK. Provide Features::set_required_custom_bit and Features::set_optional_custom_bit to allow for this.

TheBlueMatt

Your turn @dunxen

dunxen

Just wanted to check the set_optional_/set_required_ methods and they seem good to me actually. Not so awkward and quite explicit. 👍

TheBlueMatt added this to the 0.0.116 milestone Apr 20, 2023

jkczyz force-pushed the 2023-04-custom-feature-bits branch from 14a41d6 to 72b51ca Compare April 21, 2023 01:08

alecchendev reviewed Apr 26, 2023

View reviewed changes

TheBlueMatt reviewed Apr 27, 2023

View reviewed changes

alecchendev reviewed Apr 27, 2023

View reviewed changes

jkczyz force-pushed the 2023-04-custom-feature-bits branch from 72b51ca to 03f0e13 Compare May 3, 2023 18:10

TheBlueMatt reviewed May 3, 2023

View reviewed changes

jkczyz force-pushed the 2023-04-custom-feature-bits branch from 03f0e13 to bd7e721 Compare May 5, 2023 20:13

TheBlueMatt reviewed May 6, 2023

View reviewed changes

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

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

jkczyz added 4 commits May 8, 2023 10:18

Define core::ops::BitOr for Features<T>

687e587

The `lightning-custom-message` crate will need access to Features::or in order combine features of a composite handler. Expose this via a core::ops::BitOr implementation.

Provide features in CustomMessageHandler

415973e

CustomMessageHandler implementations may need to advertise support for features. Add methods to CustomMessageHandler to provide these and combine them with features from other message handlers.

Add PeerManager::init_features to DRY up code

21708b5

Add Features::requires_unknown_bits_from

3313abb

When checking features, rather than checking against which features LDK knows about, it is more useful to check against a peer's features. Add Features::requires_unknown_bits_from such that the given features are used instead.

jkczyz force-pushed the 2023-04-custom-feature-bits branch from bd7e721 to c9553cf Compare May 8, 2023 15:33

jkczyz commented May 8, 2023

View reviewed changes

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

jkczyz force-pushed the 2023-04-custom-feature-bits branch from c9553cf to 125d6d5 Compare May 10, 2023 04:53

jkczyz added 3 commits May 10, 2023 14:04

Add Features::set_{required|optional}_custom_bit

883908c

Custom message handlers may need to set feature bits that are unknown to LDK. Provide Features::set_required_custom_bit and Features::set_optional_custom_bit to allow for this.

Update missed comment in Features test

432d180

jkczyz force-pushed the 2023-04-custom-feature-bits branch from 125d6d5 to 432d180 Compare May 10, 2023 19:07

TheBlueMatt approved these changes May 18, 2023

View reviewed changes

dunxen approved these changes May 18, 2023

View reviewed changes

TheBlueMatt merged commit 498f233 into lightningdevkit:main May 18, 2023

tnull mentioned this pull request May 31, 2023

LSPS0 message handling lightningdevkit/lightning-liquidity#4

Merged

yanganto mentioned this pull request Jun 20, 2023

LSPS0 - Feature bit kuutamolabs/kld#341

Closed

		/// Sets a custom feature bit. Errors if `bit` is outside the custom range as defined by
		/// [bLIP 2] or if it is a known `T` feature.

	/// The context in which [`Features`] are applicable. Defines which features are known to the
	/// implementation, though specification of them as required or optional is up to the code
	/// constructing a features object.
	pub trait Context {
	/// Bitmask for selecting features that are known to the implementation.
	const KNOWN_FEATURE_MASK: &'static [u8];
	}

Support for custom feature bits #2204

Support for custom feature bits #2204

Uh oh!

Conversation

jkczyz commented Apr 19, 2023

Uh oh!

codecov-commenter commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alecchendev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alecchendev Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alecchendev Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jkczyz commented May 8, 2023

Uh oh!

TheBlueMatt commented May 8, 2023

Uh oh!

codecov-commenter commented Apr 20, 2023 •

edited

Loading

alecchendev Apr 26, 2023 •

edited

Loading

alecchendev Apr 27, 2023 •

edited

Loading

TheBlueMatt left a comment •

edited

Loading