Features module improvements #590

jkczyz · 2020-04-16T06:19:19Z

Refactor the features module to be more generalized and less error prone when adding new features. Each feature trait is defined in terms of even and odd bits. Each Context is defined in terms of features it supports.

From this, methods in Features no longer need to be defined specially for each context nor need to have bitmasks modified when adding a new feature.

Best reviewed one commit at a time. ~~Based on #589.~~

TheBlueMatt

Nice. Aside from a few macros which I find hard to read this is definitely way less error-prone.

lightning/src/ln/features.rs

valentinewallace · 2020-04-23T13:58:27Z

fix build? :)

jkczyz · 2020-04-23T15:15:57Z

fix build? :)

Eeek! Let me rebase. I think I can move some of the behavior changes before the refactor to make reviewing easier. Will ping when ready.

jkczyz · 2020-04-23T23:08:38Z

@valentinewallace Should be good for review. All tests compile and pass now! I moved the commits around a bit, so the ordering on Github is off.

TheBlueMatt

I can't say I'm a huge fan of generalizing with_known_relevan_init_flags - its a super awkward hack that we use cause the protocol isn't really prescriptive enough for what we need (which probably needs to be fixed upstream). Making it a first-class feature seems like the wrong direction.

Otherwise this looks great.

lightning/src/ln/features.rs

TheBlueMatt · 2020-04-23T23:19:29Z

lightning/src/ln/features.rs

+//! [BOLT #9]: https://github.com/lightningnetwork/lightning-rfc/blob/master/09-features.md
+//! [messages]: ../msgs/index.html
+//! [`Features`]: struct.Features.html
+//! [`Context`]: sealed/trait.Context.html


We could really use docs on known vs supported, I think. Just the english words dont nearly convey what is going on.

Added a paragraph explaining the distinction. Let me know whether it is clear.

jkczyz · 2020-04-24T16:56:59Z

I can't say I'm a huge fan of generalizing with_known_relevan_init_flags - its a super awkward hack that we use cause the protocol isn't really prescriptive enough for what we need (which probably needs to be fixed upstream). Making it a first-class feature seems like the wrong direction.

Otherwise this looks great.

Gotcha! Restricted it to InitFeatures and addressed other comments.

valentinewallace

This is looking really good! Mainly just wondering about adding a few more sanity checks/docs

lightning/src/ln/features.rs

TheBlueMatt · 2020-04-27T23:17:05Z

Needs rebase to address silent merge conflicts (new tests) and pick up the Github Actions config.

The test_upfront_shutdown_script functional test clears this feature flag. However, the method used to clear the flag is implemented by bit toggling. Thus, if the flag is not set the method would actually set it. Implement the method using bit clearing instead.

jkczyz

Needs rebase to address silent merge conflicts (new tests) and pick up the Github Actions config.

Done.

lightning/src/ln/features.rs

codecov · 2020-04-28T07:28:08Z

Codecov Report

Merging #590 into master will increase coverage by 0.01%.
The diff coverage is 99.57%.

@@            Coverage Diff             @@
##           master     #590      +/-   ##
==========================================
+ Coverage   91.10%   91.11%   +0.01%     
==========================================
  Files          34       34              
  Lines       20447    20544      +97     
==========================================
+ Hits        18628    18719      +91     
- Misses       1819     1825       +6

Impacted Files	Coverage Δ
lightning/src/lib.rs	`100.00% <ø> (ø)`
lightning/src/ln/onion_utils.rs	`95.02% <ø> (ø)`
lightning/src/ln/peer_channel_encryptor.rs	`94.40% <ø> (ø)`
lightning/src/util/chacha20poly1305rfc.rs	`97.95% <ø> (ø)`
lightning/src/util/enforcing_trait_impls.rs	`100.00% <ø> (ø)`
lightning/src/util/events.rs	`20.43% <ø> (ø)`
lightning/src/util/macro_logger.rs	`89.28% <ø> (ø)`
lightning/src/util/ser_macros.rs	`96.59% <ø> (ø)`
lightning/src/util/test_utils.rs	`77.84% <81.81%> (+1.37%)`	⬆️
lightning/src/chain/chaininterface.rs	`91.93% <100.00%> (ø)`
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12e2a81...ee27e84. Read the comment docs.

ariard

Code Review ACK 26fae5e. Great work!

ariard · 2020-04-28T23:04:13Z

lightning/src/ln/features.rs

+			BasicMPP,
+		],
+	});
+	define_context!(ChannelContext {


Yes I think this works correctly even with C+ distinction for option_support_large_channel, I mean it's up to us to encode as required features only at context declaration.

lightning/src/ln/features.rs

valentinewallace

Great reduction in error-proneness! & solid improvements to feature testing

jkczyz · 2020-04-29T16:19:06Z

Added 54607ff to address the loss of coverage in peer_handler.rs. Not sure why funtional_tests.rs saw a dip.

TheBlueMatt

Can you squash the fixup commits and we can merge this? :)

The initial_routing_sync feature is set by peer_handler whenever a full sync of the network graph is desired. It is not explicitly set when creating features with InitFeatures::supported(). An upcoming refactor will change supported() to known(), which will return all features known by the implementation. Thus, the initial_routing_sync flag will need to be set by default. This commit makes the behavior change ahead of the refactor.

Each feature is represented by two bits within Features' flags field. Working with these flags requires bitwise operations, which can be error prone. Rather than directly checking and manipulating bits, encapsulate the bits within each feature trait and provide mechanisms for doing so. This removes the need to comment on which features correspond to bitwise expressions since the expressions use feature trait identifiers instead. With this approach, byte literals and expressions can be evaluated at compile time still. However, for these cases, knowing which byte within the flags that a feature corresponds to still must be determined by the implementor. Remove the special case where initial_routing_sync has no even bit. Now, it (bit 2) is considered known by the implementation.

Features for a given context are duplicated throughout the features module. Use a macro for defining a Context and the applicable features such that features only need to be defined for a Context in one place. The Context provides bitmasks for selecting known and unknown feature flags. BOLT 1 and BOLT 9 refer to features as "known" if a peer understands them. They also use the term "supported" to mean either optional or required. Update the features module to use similar terminology. - Define contexts in terms of required and optional features rather than just supported features - Define known features as those that are optional or required - Rename supported() constructor to known() For completeness, clear_optional_bit for each feature is now called clear_bits and clears both optional and required bits.

Refactoring the features module allowed for making code specific to certain contexts generalizable. Specifically, KNOWN_FEATURE_MASK is defined on Context instead of hardcoded in each method specialization. Thus, such methods are no longer required.

Converting from InitFeatures to other Features is accomplished using Features::with_known_relevant_init_flags. Define a more general to_context method which converts from Features of one Context to another. Additionally, ensure the source context only has known flags before selecting flags for the target context.

Include tests for requires_unknown_bits and supports_unknown_bits when an unknown even bit, odd bit, or neither is set. Refactor bit clearing such that tests and production code share the same code path. Fix a potential spec incompatibility (currently only exposed in testing code) where trailing zero bytes are not removed after a bit is cleared.

jkczyz · 2020-04-29T18:13:40Z

Can you squash the fixup commits and we can merge this? :)

Yes! Done and should be good to go.

TheBlueMatt reviewed Apr 17, 2020

View reviewed changes

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

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

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

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

jkczyz force-pushed the 2020-04-feature-flags branch 2 times, most recently from 5fd75de to cb299eb Compare April 23, 2020 00:50

jkczyz force-pushed the 2020-04-feature-flags branch 2 times, most recently from 5b3a54c to bcd929d Compare April 23, 2020 20:31

TheBlueMatt reviewed Apr 23, 2020

View reviewed changes

jkczyz force-pushed the 2020-04-feature-flags branch from bcd929d to 771a749 Compare April 24, 2020 16:54

valentinewallace reviewed Apr 27, 2020

View reviewed changes

ariard added High Review Priority Moderate Review Difficulty labels Apr 27, 2020

TheBlueMatt mentioned this pull request Apr 28, 2020

Require static_remotekey #539

Merged

jkczyz commented Apr 28, 2020

View reviewed changes

jkczyz force-pushed the 2020-04-feature-flags branch from 771a749 to 26fae5e Compare April 28, 2020 06:45

ariard reviewed Apr 28, 2020

View reviewed changes

valentinewallace approved these changes Apr 28, 2020

View reviewed changes

jkczyz force-pushed the 2020-04-feature-flags branch from 26fae5e to 7432b1c Compare April 29, 2020 16:07

TheBlueMatt approved these changes Apr 29, 2020

View reviewed changes

jkczyz added 5 commits April 29, 2020 11:07

Improve features module documentation

77b467c

jkczyz added 3 commits April 29, 2020 11:11

Sanity check that known features are not required

ee27e84

jkczyz force-pushed the 2020-04-feature-flags branch from 7432b1c to ee27e84 Compare April 29, 2020 18:12

TheBlueMatt merged commit 9098240 into lightningdevkit:master Apr 29, 2020

jkczyz mentioned this pull request May 1, 2020

Make endianness of our feature flag bitfields more intuitive #542

Closed

Features module improvements #590

Features module improvements #590

Uh oh!

Conversation

jkczyz commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

valentinewallace commented Apr 23, 2020

Uh oh!

jkczyz commented Apr 23, 2020

Uh oh!

jkczyz commented Apr 23, 2020

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

jkczyz Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

jkczyz commented Apr 24, 2020

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Apr 27, 2020

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

ariard Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

jkczyz commented Apr 29, 2020

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

jkczyz commented Apr 29, 2020

Uh oh!

Uh oh!

jkczyz commented Apr 16, 2020 •

edited

Loading

codecov bot commented Apr 28, 2020 •

edited

Loading