Document `lightning` crate features #1478

mattfaltyn · 2022-05-12T17:19:41Z

Closes #1462.

Issue: We have a few features defined in Cargo.toml with comments describing their use, but we don't expose them in the crate-level lib.rs documentation, making docs pretty much invisible. We should copy the text over, expand it, and clarify it so that you can see the features on docs.rs.

Changes: I have added the features defined in Cargo.toml (with comments) into the crate-level lib.rs documentation - please let me know if you want more comprehensive descriptions and/or if the structure of the documentation should change. I am new to Rust and happy to learn :)

TheBlueMatt

Thanks for tackling this one!

TheBlueMatt · 2022-05-12T17:22:27Z

lightning/src/lib.rs

+//! 
+//! Some of the features include: 
+//! 
+//! 1. Internal test utilities exposed to other repo crates:


I don't think we should document this - no one but us should ever use it, and we just use it for other creates in the same repo.

lightning/src/lib.rs

TheBlueMatt · 2022-05-12T17:24:19Z

lightning/src/lib.rs

+//! max_level_trace = []
+//! ```
+//! 
+//! 3. Allow signing of local transactions that may have been revoked or will be revoked, for functional testing (e.g. justice tx handling). This is unsafe to use in production because it may result in the counterparty publishing taking our funds.


Lets not document "unsafe in production" stuff :)

lightning/src/lib.rs

TheBlueMatt · 2022-05-12T17:31:43Z

lightning/src/lib.rs

+//! unsafe_revoked_tx_signing = []
+//! _bench_unstable = []
+//! 
+//! no-std = ["hashbrown", "bitcoin/no-std", "core2/alloc"]


Should add comments for no-std and std saying, well, "std enables things which require std, including std::io trait implementations and things which utilize time" and "no-std exposes write trait implementations from the core2crate"

lightning/src/lib.rs

tnull

Thanks for having look!
So far I think you're mostly stating what the features are, but not what they do. It's probably best to keep it brief, but it would still be highly appreciated if you could at least roughly describe why a user would want to enable/disable a particular feature.

tnull · 2022-05-13T10:04:40Z

lightning/src/lib.rs

+//! std = ["bitcoin/std"]
+//! ```
+//! 
+//! 4. Generate low-r bitcoin signatures, which saves 1 byte in 50% of the cases:


Mh, from/for what does it save 1 byte in 50%? Could you explain what the feature does and maybe supply a reference for further reading, e.g., bitcoin/bitcoin#13666 or https://bitcoin.stackexchange.com/questions/111660/what-is-signature-grinding?

@TheBlueMatt @tnull thank you for your comments and suggestions! I made the recommend changes!

TheBlueMatt · 2022-05-17T18:49:54Z

lightning/src/lib.rs

+//! 
+//! 2. Allow signing of local transactions that may have been revoked or will be revoked, 
+//! for functional testing (e.g. justice tx handling): 
+//! * `unsafe_revoked_tx_signing` - allows unsafe signing in dev code, such as functional testing of justice transactions outside our crate


Lets not document test-only features like unsafe_revoked_tx_signing and _bench_unstable.

TheBlueMatt · 2022-05-17T18:50:18Z

lightning/src/lib.rs

+//! for functional testing (e.g. justice tx handling): 
+//! * `unsafe_revoked_tx_signing` - allows unsafe signing in dev code, such as functional testing of justice transactions outside our crate
+//! * `_bench_unstable` - allows test features for benching
+//! * `no-std ` - exposes write trait implementations from the `core2` crate


Why is this not listed (ie with a 2. in front)?

TheBlueMatt · 2022-05-17T18:50:33Z

lightning/src/lib.rs

+//! `default` features are: 
+//! 
+//! * `std` - enables things which require `std`, including `std::io` trait implementations and things which utilize time
+//! * `grind_signatures` - enables generation of [low-r bitcoin signatures](https://bitcoin.stackexchange.com/questions/111660/what-is-signature-grinding), which saves 1 byte per signature in 50% of the cases (see [bitcoin PR #13666](https://github.com/bitcoin/bitcoin/pull/13666))


Should we just move these to the list above and then list default features separately?

@TheBlueMatt what other features are in default apart from std and grind_signatures? I just see those two in Cargo.toml.

I just meant that we should maybe document std and grind_signatures in the same section as everything else, and then separately mention that default includes those two, rather than documenting them in a separate section.

Ah I see will do - thanks for the clarification!

tnull · 2022-05-18T06:53:30Z

lightning/src/lib.rs

+//! 
+//! `default` features are: 
+//! 
+//! * `std` - enables things which require `std`, including `std::io` trait implementations and things which utilize time


nit: "things" is a bit unspecific/casual here. Maybe "functionalities", avoiding the obvious "features"?

tnull · 2022-05-18T06:54:53Z

lightning/src/lib.rs

+//! 
+//! Available features are: 
+//! 
+//! 1. Skip logging of messages at levels below the given log level:


Maybe we can switch the order here, so that the arguably more important (default) features are described up top, and the logging/debugging things are listed beneath?

codecov-commenter · 2022-05-19T16:54:27Z

Codecov Report

Merging #1478 (7067373) into main (75ca50f) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1478      +/-   ##
==========================================
- Coverage   90.93%   90.93%   -0.01%     
==========================================
  Files          77       77              
  Lines       42394    42394              
  Branches    42394    42394              
==========================================
- Hits        38553    38550       -3     
- Misses       3841     3844       +3

Impacted Files	Coverage Δ
lightning/src/lib.rs	`100.00% <ø> (ø)`
lightning-net-tokio/src/lib.rs	`75.91% <0.00%> (-0.31%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.08% <0.00%> (-0.04%)`	⬇️

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 75ca50f...7067373. Read the comment docs.

mattfaltyn · 2022-05-20T17:34:02Z

@TheBlueMatt @tnull made the recommended changes! Thanks for your continued reviews!

TheBlueMatt · 2022-05-20T18:21:56Z

lightning/src/lib.rs

+//! * `std` 
+//! * `grind_signatures`
+//! * `no-std ` - exposes write trait implementations from the `core2` crate
+//! * `justice_tx_handling` - allows signing of local transactions that may have been revoked or will be revoked for functional testing


I don't think we ever want anyone using this - its basically a debug feature to enable unsafe behavior during testing.

Sounds good - I will remove it.

TheBlueMatt

Looks good to me! Can you rebase to clean up the git history (rather than merging in upstream changes, there shouldn't generally be any reason to do that) and we can land it? 🚀

TheBlueMatt · 2022-05-20T22:46:39Z

lightning/src/lib.rs

+//! 
+//! * `std` 
+//! * `grind_signatures`
+//! * `no-std ` - exposes write trait implementations from the `core2` crate


nit: maybe say "at least one of no-std or std are required"

TheBlueMatt · 2022-05-20T22:46:52Z

lightning/src/lib.rs

@@ -17,6 +17,24 @@
 //! figure out how best to make networking happen/timers fire/things get written to disk/keys get
 //! generated/etc. This makes it a good candidate for tight integration into an existing wallet
 //! instead of having a rather-separate lightning appendage to a wallet.
+//! 


nit: a number of lines here have whitespace at EOL, would be nice to remove that.

mattfaltyn · 2022-05-21T00:10:17Z

Looks good to me! Can you rebase to clean up the git history (rather than merging in upstream changes, there shouldn't generally be any reason to do that) and we can land it? 🚀

Rebased! Let me know if you want me to make any more changes :)

TheBlueMatt · 2022-05-21T00:21:49Z

Looks like there's still three commits? Can you squash down the fixup commits into a consistent git history that doesn't contain fixes for earlier commits in later ones.

mattfaltyn · 2022-05-21T03:46:08Z

Looks like there's still three commits? Can you squash down the fixup commits into a consistent git history that doesn't contain fixes for earlier commits in later ones.

Not sure how I missed that - thanks for the heads up!

TheBlueMatt · 2022-05-21T05:02:47Z

Can you reword the commit from "Removed justice_tx_handling" to something about documenting features?

mattfaltyn · 2022-05-21T18:43:09Z

Can you reword the commit from "Removed justice_tx_handling" to something about documenting features?

Yup! Done!

TheBlueMatt

LGTM, though it looks like there's still EOL whitespace on a few lines. Also, if you fix that can you break lines at around 100 chars long?

TheBlueMatt · 2022-05-24T20:17:20Z

Please don't push merge commits to PRs, please rebase instead.

mattfaltyn · 2022-05-24T20:37:17Z

Please don't push merge commits to PRs, please rebase instead.

Sorry I am having a hard time rebasing properly - any good guides to read?

mattfaltyn · 2022-05-24T20:38:28Z

Would I be able to delete this PR and create a new one? It has become very messy and I don't want to make it a headache for you.

tnull · 2022-05-24T20:44:11Z

Sorry I am having a hard time rebasing properly - any good guides to read?

Not sure about a guide right now, but you basically just want to use git pull --rebase <remote> <branch> (for me typically git pull --rebase upstream main, assuming upstream is your remote pointing to this repository, not your fork).

You may also want to have a look at the man git-rebase, in particular at the section for git rebase --interactive, which allows you to not only rebase, but also to squash down commits etc.

Since I find that I almost always want to rebase instead of merge, I set

[pull]
	rebase = true

in my .gitconfig, which allows me to default to rebasing.

Would I be able to delete this PR and create a new one? It has become very messy and I don't want to make it a headache for you.

Don't worry! You're almost there, please don't give up now! :)

TheBlueMatt · 2022-05-24T20:58:00Z

I generally point people to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

mattfaltyn · 2022-05-24T23:25:33Z

@TheBlueMatt @tnull Thank you for your support! I think I managed to clean things up with your help :)

mattfaltyn · 2022-05-25T15:51:25Z

@tnull can you please re-approve this PR?

tnull · 2022-05-25T16:10:43Z

@tnull can you please re-approve this PR?

I already did, but I think we need to get another person that has merge rights to do that.

mattfaltyn · 2022-05-25T20:57:52Z

@tnull can you please re-approve this PR?

I already did, but I think we need to get another person that has merge rights to do that.

Thanks for letting me know! @TheBlueMatt is there anything I should do at this point in time with regards to this PR?

mattfaltyn changed the title ~~Document lightning crate features #1462~~ Document lightning crate features May 12, 2022

TheBlueMatt reviewed May 12, 2022

View reviewed changes

tnull reviewed May 13, 2022

View reviewed changes

mattfaltyn force-pushed the issue1462 branch from 82ffb46 to dc2f234 Compare May 17, 2022 18:06

TheBlueMatt reviewed May 17, 2022

View reviewed changes

tnull reviewed May 18, 2022

View reviewed changes

mattfaltyn force-pushed the issue1462 branch from e6d041b to 163ab58 Compare May 20, 2022 17:32

TheBlueMatt reviewed May 20, 2022

View reviewed changes

mattfaltyn force-pushed the issue1462 branch from be518a5 to e2284f5 Compare May 21, 2022 00:07

mattfaltyn force-pushed the issue1462 branch 2 times, most recently from e95e8c0 to b68ad7a Compare May 21, 2022 03:44

tnull previously approved these changes May 21, 2022

View reviewed changes

mattfaltyn dismissed tnull’s stale review via a22caab May 21, 2022 18:42

mattfaltyn force-pushed the issue1462 branch from b68ad7a to a22caab Compare May 21, 2022 18:42

mattfaltyn requested a review from TheBlueMatt May 24, 2022 15:58

TheBlueMatt previously approved these changes May 24, 2022

View reviewed changes

mattfaltyn dismissed TheBlueMatt’s stale review via 87b346f May 24, 2022 20:31

mattfaltyn force-pushed the issue1462 branch 2 times, most recently from 87b346f to c741044 Compare May 24, 2022 20:35

Document lightning crate features

7067373

mattfaltyn force-pushed the issue1462 branch from c741044 to 7067373 Compare May 24, 2022 23:23

TheBlueMatt approved these changes May 24, 2022

View reviewed changes

tnull approved these changes May 24, 2022

View reviewed changes

TheBlueMatt added the Seeking Code Review label May 25, 2022

jkczyz approved these changes May 25, 2022

View reviewed changes

jkczyz merged commit 440ea94 into lightningdevkit:main May 25, 2022

Document lightning crate features #1478

Document lightning crate features #1478

Conversation

mattfaltyn commented May 12, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattfaltyn May 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 19, 2022 • edited Loading

Codecov Report

mattfaltyn commented May 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattfaltyn commented May 21, 2022

TheBlueMatt commented May 21, 2022

mattfaltyn commented May 21, 2022

TheBlueMatt commented May 21, 2022

mattfaltyn commented May 21, 2022

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented May 24, 2022

mattfaltyn commented May 24, 2022

mattfaltyn commented May 24, 2022

tnull commented May 24, 2022 • edited Loading

TheBlueMatt commented May 24, 2022

mattfaltyn commented May 24, 2022

mattfaltyn commented May 25, 2022

tnull commented May 25, 2022

mattfaltyn commented May 25, 2022

Document `lightning` crate features #1478

Document `lightning` crate features #1478

tnull left a comment •

edited

Loading

mattfaltyn May 19, 2022 •

edited

Loading

codecov-commenter commented May 19, 2022 •

edited

Loading

tnull commented May 24, 2022 •

edited

Loading