Log peer's features if they require some unknown features we don't support

[tnull]

Previously, when a peer sends us features we don't understand we wouldn't log them, but just log Peer requires features unknown to us.

Here, we fix this.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.23810% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 89.41%. Comparing base (1d2a27d) to head (9ee73df).
Report is 2 commits behind head on main.

Files	Patch %	Lines
lightning/src/ln/peer_handler.rs	50.00%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2947      +/-   ##
==========================================
+ Coverage   89.40%   89.41%   +0.01%     
==========================================
  Files         117      117              
  Lines       96016    96217     +201     
  Branches    96016    96217     +201     
==========================================
+ Hits        85842    86033     +191     
- Misses       7957     7964       +7     
- Partials     2217     2220       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[TheBlueMatt]

IIRC the to-string for a feature doesn't actually say which features are set which are unknown, only that there are some unknown features. In this case it seems like we should probably include what those features are.

[tnull]

IIRC the to-string for a feature doesn't actually say which features are set which are unknown, only that there are some unknown features. In this case it seems like we should probably include what those features are.

Done, we now log which features are unknown to us (in BOLT09 bit number format).

Also rebased to make CI pass.

[TheBlueMatt]

I think the first commit can now just get the rest of them squashed.

[TheBlueMatt]

LGTM, feel free to squash the fixups.

[tnull]

Squashed fixups without further changes.

[dunxen]

LGTM (with optout's comment on duplicate resolved or as a follow-up)

[TheBlueMatt]

I'm pretty sure the first commit should be squashed-into, its entirely changed by the second commit.

[tnull]

I'm pretty sure the first commit should be squashed-into, its entirely changed by the second commit.

Ah, true, squashed it in and rebased to make CI happy. However now created a new fixup to address your last comment.

[TheBlueMatt]

LGTM, feel free to squash.

[tnull]

Squashed without further changes.

[tnull]

Force-pushed the following fixups:

> git diff-tree -U2  9ee73dfb3 44cf6928d

diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs
index 9a0c2b30e..8de57c2b9 100644
--- a/lightning/src/ln/features.rs
+++ b/lightning/src/ln/features.rs
@@ -788,5 +788,5 @@ impl<T: sealed::Context> Features<T> {
                        if byte & unknown_features != 0 {
                                for bit in (0..8).step_by(2) {
-                                       if byte >> bit & 1 == 1 {
+                                       if ((byte & unknown_features) >> bit) & 1 == 1 {
                                                unknown_bits.push(i * 8 + bit);
                                        }
@@ -1067,9 +1067,13 @@ mod tests {

                let mut features = ChannelFeatures::empty();
-               features.set_unknown_feature_optional();
+               features.set_unknown_feature_required();
                features.set_custom_bit(123456786).unwrap();
                assert!(features.requires_unknown_bits());
                assert!(features.supports_unknown_bits());
-               assert_eq!(features.required_unknown_bits_from(&ChannelFeatures::empty()), vec![123456786]);
+               assert_eq!(features.required_unknown_bits_from(&ChannelFeatures::empty()), vec![123456786, 123456788]);
+
+               let mut limiter = ChannelFeatures::empty();
+               limiter.set_unknown_feature_optional();
+               assert_eq!(features.required_unknown_bits_from(&limiter), vec![123456786]);
        }

[TheBlueMatt]

Your turn @dunxen

[dunxen]

Very sorry for the delay here @tnull. Completely fell off my radar.

Thankfully no conflicts? LGTM now.