Skip to content

Document better optional features #1514

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 13, 2022

Conversation

mattfaltyn
Copy link
Contributor

Closes #438.

Issue: Post #428 merge, would be great to document FeatureContext* setters (lightning/src/ln/msgs.rs), I don't think optional features are documented anyway in the current codebase beyond references to BOLT.

Changes: Added documentation in features.rs on what the currently-supported features do in the LDK based on BOLT-9 feature flags.

@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2022

Codecov Report

Merging #1514 (86299c5) into main (716539e) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1514      +/-   ##
==========================================
- Coverage   90.93%   90.91%   -0.02%     
==========================================
  Files          80       80              
  Lines       43469    43469              
  Branches    43469    43469              
==========================================
- Hits        39527    39520       -7     
- Misses       3942     3949       +7     
Impacted Files Coverage Δ
lightning/src/ln/features.rs 99.46% <ø> (ø)
lightning/src/chain/mod.rs 63.63% <0.00%> (-4.55%) ⬇️
lightning/src/ln/functional_tests.rs 96.98% <0.00%> (-0.08%) ⬇️
lightning/src/chain/channelmonitor.rs 91.18% <0.00%> (-0.06%) ⬇️

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 716539e...86299c5. Read the comment docs.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This already needs rebase for the ZeroConf feature lol. sorry about that.

//! - `DataLossProtect` - requires/supports that a node, which has somehow fallen behind (e.g. has been restored from old backup),
//! can detect that it has fallen behind
//! (see [BOLT-2](https://github.com/lightning/bolts/blob/master/02-peer-protocol.md) for more information).
//! - `set_data_loss_protect_optional` - sets this feature to optional.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, do we need to list the methods? In general I think users should never change them manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good - I will remove them.

@@ -18,6 +18,81 @@
//! the term "supports" is used in reference to a particular set of [`Features`]. That is, a node
//! supports a feature if it advertises the feature (as either required or optional) to its peers.
//! And the implementation can interpret a feature if the feature is known to it.
//!
//! The following features are currently supported in the LDK:
//! - `DataLossProtect` - requires/supports that a node, which has somehow fallen behind (e.g. has been restored from old backup),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should mention somewhere what we require vs support. I think currently we may only require StaticRemoteKey, but in the near future we'll probably require VariableLengthOnion too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make two lists: one for required features (StaticRemoteKey and VariableLengthOnion) and one for supported features (everything else).

TheBlueMatt
TheBlueMatt previously approved these changes Jun 10, 2022
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for having a look! Just a few suggestions/nits/remarks.

//! (see [BOLT-3](https://github.com/lightning/bolts/blob/master/03-transactions.md) for more information).
//!
//! The following features are currently supported in the LDK:
//! - `DataLossProtect` - requires/supports that a node, which has somehow fallen behind (e.g. has been restored from old backup),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! - `DataLossProtect` - requires/supports that a node, which has somehow fallen behind (e.g. has been restored from old backup),
//! - `DataLossProtect` - requires/supports that a node which has somehow fallen behind, e.g., has been restored from an old backup,

//! - `ShutdownAnySegwit` - requires/supports that future segwit versions are allowed in `shutdown`
//! (see [BOLT-2](https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#closing-initiation-shutdown) for more information).
//! - `ChannelType` - node supports the channel_type field in open/accept
//! (see [BOLT-2](https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#closing-initiation-shutdown) for more information).
Copy link
Contributor

@tnull tnull Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this link to BOLT 2 and the following two should not link to the closing-initiation-shutdown subsection.

//! - `SCIDPrivacy` - supply channel aliases for routing
//! (see [BOLT-2](https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#closing-initiation-shutdown) for more information).
//! - `Keysend` - send funds to a node without an invoice
//! (see [BOLT-11](https://github.com/lightning/bolts/blob/master/11-payment-encoding.md) for more information).
Copy link
Contributor

@tnull tnull Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pointing users towards this URL may be misleading since BOLT 11 does AFAIK not contain any further information on keysend payments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right - I will link to the Keysend feature assignment proposal.

@TheBlueMatt TheBlueMatt merged commit d6feb1c into lightningdevkit:main Jun 13, 2022
@TheBlueMatt TheBlueMatt mentioned this pull request Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document better optional features (data_loss_protect, upfront_shutdown_script,...)
4 participants