rust-lightning icon indicating copy to clipboard operation
rust-lightning copied to clipboard

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

Open tnull opened this issue 1 year ago • 8 comments

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.

tnull avatar Mar 19 '24 15:03 tnull

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 :warning:

:exclamation: 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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Mar 19 '24 15:03 codecov-commenter

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.

tnull avatar Apr 08 '24 13:04 tnull

LGTM, feel free to squash the fixups.

TheBlueMatt avatar Apr 10 '24 21:04 TheBlueMatt

Squashed fixups without further changes.

tnull avatar Apr 11 '24 08:04 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.

tnull avatar Apr 12 '24 08:04 tnull

LGTM, feel free to squash.

TheBlueMatt avatar Apr 12 '24 17:04 TheBlueMatt

Squashed without further changes.

tnull avatar Apr 15 '24 06:04 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]);
        }

tnull avatar Apr 15 '24 15:04 tnull