Swap the dep order between `lightning` and `lightning-invoice` #3234

TheBlueMatt · 2024-08-09T14:57:25Z

`lightning-invoice` currently has a dependency on the entire
`lightning` crate just because it wants to use some of the useful
types from it. This is obviously backwards and leads to some
awkwardness like the BOLT 11 invoice signing API in the `lightning`
crate taking a `[u5]` rather than a `Bolt11Invoice`.

Instead, this simplifies the interface and avoids a
serialization-deserialization roundtrip when signing invoices in a
validating signer.

Fixes #3227

TheBlueMatt · 2024-08-09T15:26:09Z

Note that the first commit can/should be dropped after #3020, but I needed it to get things to compile at all until then.

apoelstra · 2024-08-09T15:38:56Z

ut/concept ACK 7d52254

Thanks!

TheBlueMatt · 2024-08-09T15:46:08Z

Oops, forgot to rustfmt, pushed that.

codecov · 2024-08-10T20:51:55Z

Codecov Report

Attention: Patch coverage is 93.06519% with 50 lines in your changes missing coverage. Please review.

Project coverage is 89.75%. Comparing base (795887a) to head (2350921).
Report is 4 commits behind head on main.

Files	Patch %	Lines
lightning-types/src/features.rs	95.62%	12 Missing and 12 partials ⚠️
lightning-types/src/payment.rs	73.07%	6 Missing and 1 partial ⚠️
lightning/src/util/ser.rs	0.00%	7 Missing ⚠️
lightning/src/ln/msgs.rs	60.00%	0 Missing and 4 partials ⚠️
lightning/src/ln/features.rs	80.00%	0 Missing and 3 partials ⚠️
lightning/src/sign/mod.rs	50.00%	3 Missing ⚠️
lightning/src/ln/bolt11_payment.rs	96.87%	1 Missing ⚠️
lightning/src/util/test_utils.rs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3234      +/-   ##
==========================================
- Coverage   89.75%   89.75%   -0.01%     
==========================================
  Files         122      123       +1     
  Lines      101903   101887      -16     
  Branches   101903   101887      -16     
==========================================
- Hits        91465    91445      -20     
+ Misses       7753     7751       -2     
- Partials     2685     2691       +6

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

tnull

Unsurprisingly, strong concept ACK from my side for this.

rustfmt_excluded_files

tnull · 2024-08-12T08:01:59Z

lightning/src/ln/mod.rs

+pub mod invoice_utils;
+pub mod bolt11_payment;
+
+pub use lightning_types::{PaymentHash, PaymentPreimage, PaymentSecret};


Hmm, re-exporting under lightning::ln and lightning::ln::types is a bit confusing. Can't we just switch to either use a re-export at the top-level (i.e., lightning::types or similar) or using lightning_types::{..}?

I agree, though we have a lot of code that references these as lightning::ln::... that I'm not sure what to do about, and it seems like something to fix in a followup rather than here.

Yeah, doing it in a follow-up (possibly still landing for the same release?) would be fine I think

lightning-types/src/lib.rs

lightning/src/util/mod.rs

tnull · 2024-08-12T08:32:40Z

Note that the first commit can/should be dropped after #3020, but I needed it to get things to compile at all until then.

FWIW, not sure which first commit you're referring to. Seems you might have already dropped it?

optout21 · 2024-08-12T09:30:08Z

lightning-types/Cargo.toml

+[package.metadata.docs.rs]
+rustdoc-args = ["--cfg", "docsrs"]
+
+[features]


When building lightning-types by itself, I found that the std/no-std features are missing:

Suggested change

[features]

[features]

default = ["std"]

no-std = ["bitcoin/no-std"]

std = ["bitcoin/std", "bech32/std"]

Should be resolved when we upgrade to rust-bitcoin 32 in the same release.

optout21 · 2024-08-12T09:32:32Z

LGTM!

This change straightens the lightning-invoice dependency.
So the criteria to move types to lightning-types is that if they are needed in lightning-invoice. I suppose in the future it may be the case that other types will be moved as needed, they crate will be already there.

TheBlueMatt · 2024-08-12T13:14:46Z

FWIW, not sure which first commit you're referring to. Seems you might have already dropped it?

I did.

So the criteria to move types to lightning-types is that if they are needed in lightning-invoice. I suppose in the future it may be the case that other types will be moved as needed, they crate will be already there.

Yea, for now. We can add other stuff that may be useful for crates that want some additional types but don't want to depend on all of lightning.

tnull · 2024-08-13T09:02:23Z

lightning-types/src/routing.rs

+	/// The difference in CLTV values between this node and the next node.
+	pub cltv_expiry_delta: u16,
+	/// The minimum value, in msat, which must be relayed to the next hop.
+	pub htlc_minimum_msat: Option<u64>,


FWIW, to keep this new crate as generic Lightning as possible, we could eventually consider a way to drop/move these fields which are somewhat proprietary to LDK/leak LDK internals as they don't exist in the actual BOLT11 r field.

Yea, I dunno, I don't mind it. These aren't really BOLT 11 fields but rather a generic concept of what a route hint hop should be.

tnull · 2024-08-13T09:05:15Z

Feel free to squash fixups.

`lightning-invoice` currently has a dependency on the entire `lightning` crate just because it wants to use some of the useful types from it. This is obviously backwards and leads to some awkwardness like the BOLT 11 invoice signing API in the `lightning` crate taking a `[u5]` rather than a `Bolt11Invoice`. This is the first step towards fixing that - moving the common types we need into a new `lightning-types` crate which both can depend on. Since we're using a new crate and can't depend on the existing `lightning` hex utility to implement `Display`, we also take this opportunity to switch to the new `Display` impl macro in `hex_conservative`.

`lightning-invoice` currently has a dependency on the entire `lightning` crate just because it wants to use some of the useful types from it. This is obviously backwards and leads to some awkwardness like the BOLT 11 invoice signing API in the `lightning` crate taking a `[u5]` rather than a `Bolt11Invoice`. This takes one more step, moving the routing types `lightning-invoice` uses into `lightning-types`.

It turns out all the places we use `Features::is_subset` we could as well be using `Features::requires_unknown_bits_from`. Further, in the next commit `Features` will move to a different crate so any methods which the `lightning` crate uses will need to be public. As the `is_subset` API is prety confusing (it doesn't consider optional/required bits, only whether the bits themselves are strictly a subset) it'd be nice to not have to expose it, which is enabled here.

`lightning-invoice` currently has a dependency on the entire `lightning` crate just because it wants to use some of the useful types from it. This is obviously backwards and leads to some awkwardness like the BOLT 11 invoice signing API in the `lightning` crate taking a `[u5]` rather than a `Bolt11Invoice`. This takes one more step, moving the `Features` types from `lightning` to `lightning-types`.

`lightning-invoice` currently has a dependency on the entire `lightning` crate just because it wants to use some of the useful types from it. This is obviously backwards and leads to some awkwardness like the BOLT 11 invoice signing API in the `lightning` crate taking a `[u5]` rather than a `Bolt11Invoice`. This takes one more step, moving the `UntrustedString` and `PrintableString` types to `lightning-types`.

In a coming commit, the `lightning-invoice::utils` module will move to the `lightning` crate, causing its tests to be included in the global lockorder tests done in that crate. This should be fine, except that the `lightning-invoice::utils` module currently holds the `added_monitors` lock too long causing lockorder violations. Instead, this commit replaces the legacy monitors-added test with the `check_added_monitors` test utility.

`lightning-invoice` currently has a dependency on the entire `lightning` crate just because it wants to use some of the useful types from it. This is obviously backwards and leads to some awkwardness like the BOLT 11 invoice signing API in the `lightning` crate taking a `[u5]` rather than a `Bolt11Invoice`. This takes tees us up for the final step, adding a `lightning-types` dependency to `lightning-invoice` and using it for imports rather than the `lightning` crate.

`lightning-invoice` previously had a dependency on the entire `lightning` crate just because it wants to use some of the useful types from it. This is obviously backwards and leads to some awkwardness like the BOLT 11 invoice signing API in the `lightning` crate taking a `[u5]` rather than a `Bolt11Invoice`. Here we finally rectify this issue, swapping the dependency order and making `lightning` depend on `lightning-invoice` rather than the other way around. This moves various utilities which were in `lightning-invoice` but relied on `lightning` payment types to make payments to where they belong (the `lightning` crate), but doesn't bother with integrating them well in their new home.

Now that the `lightning` crate depends on the `lightning-invoice` crate, there's no reason to have the `sign_invoice` method take raw base32 field elements as we can now give it a real `RawBolt11Invoice`, which we do here. This simplifies the interface and avoids a serialization-deserialization roundtrip when signing invoices in a validating signer. FIxes lightningdevkit#3227

In the next commit we'll `rustfmt` newly-added files, but before we do so we clean up some code so that the resulting files won't be quite as absurd. We also exclude the new `invoice_utils.rs` file, as it needs quite substantial cleanups.

The past handful of commits were mostly moving code around, so to aid reviewers violated our `rustfmt` rules. Here we rectify that by `rustfmt`'ing the newly-added files.

TheBlueMatt · 2024-08-13T12:55:28Z

Rebased to address trivial conflict and squashed without other changes.

tnull

LGTM

While this is mostly moving code, I think this needs a second or third set of eyes given the ~ +1800/-1600 change set.

vincenzopalazzo

LGTM

vincenzopalazzo · 2024-08-14T14:11:48Z

lightning-types/Cargo.toml

@@ -0,0 +1,24 @@
+[package]
+name = "lightning-types"


Reading the PR description I was expecting a name like lightning-common, but types is fine too I guess

*-common seems a bit vague as it might imply actual "core" lightning functionality. I've seen the suffix be used a bit loosely to not only refer to types. *-types seems more appropriate IMO :)

Its also not really intended to be used by downstream crates, so I'm not sure it matters that much :)

dunxen

Quite a chonky one, but thanks for leaving the formatting until the final commit. I've confirmed it's mostly moves.

LGMT.

dunxen · 2024-08-14T14:04:19Z

lightning/src/chain/package.rs

@@ -93,7 +93,7 @@ pub(crate) fn verify_channel_type_features(channel_type_features: &Option<Channe
 			supported_feature_set |= additional_permitted_features;
 		}

-		if !features.is_subset(&supported_feature_set) {
+		if features.requires_unknown_bits_from(&supported_feature_set) {


Way clearer.

TheBlueMatt force-pushed the 2024-08-lightning-dep-inv branch from 08671c8 to 7d52254 Compare August 9, 2024 15:25

apoelstra mentioned this pull request Aug 9, 2024

Cleanup Bech32 logic, isolate or make away with bech32 dependency #3195

Open

TheBlueMatt force-pushed the 2024-08-lightning-dep-inv branch from 7d52254 to 7716d79 Compare August 9, 2024 15:45

TheBlueMatt force-pushed the 2024-08-lightning-dep-inv branch from 7716d79 to f7ff707 Compare August 10, 2024 20:43

TheBlueMatt force-pushed the 2024-08-lightning-dep-inv branch from f7ff707 to 66748fe Compare August 12, 2024 00:37

tnull reviewed Aug 12, 2024

View reviewed changes

optout21 mentioned this pull request Aug 12, 2024

Own implementation of Bech32::u5 type, upgrade of bech32 dependency #3201

Closed

3 tasks

optout21 reviewed Aug 12, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-08-lightning-dep-inv branch 2 times, most recently from 8d53b79 to 2350921 Compare August 12, 2024 14:17

tnull previously approved these changes Aug 13, 2024

View reviewed changes

TheBlueMatt force-pushed the 2024-08-lightning-dep-inv branch from 2350921 to fff0ef3 Compare August 13, 2024 12:54

TheBlueMatt added 10 commits August 13, 2024 12:54

Prepare to rustfmt newly added files

30879ed

In the next commit we'll `rustfmt` newly-added files, but before we do so we clean up some code so that the resulting files won't be quite as absurd. We also exclude the new `invoice_utils.rs` file, as it needs quite substantial cleanups.

rustfmt new files added in the past few commits

ae59d1d

The past handful of commits were mostly moving code around, so to aid reviewers violated our `rustfmt` rules. Here we rectify that by `rustfmt`'ing the newly-added files.

TheBlueMatt dismissed tnull’s stale review via ae59d1d August 13, 2024 12:55

TheBlueMatt force-pushed the 2024-08-lightning-dep-inv branch from fff0ef3 to ae59d1d Compare August 13, 2024 12:55

tnull approved these changes Aug 13, 2024

View reviewed changes

vincenzopalazzo approved these changes Aug 14, 2024

View reviewed changes

dunxen approved these changes Aug 14, 2024

View reviewed changes

TheBlueMatt merged commit 398314b into lightningdevkit:main Aug 14, 2024
13 of 17 checks passed

optout21 mentioned this pull request Aug 15, 2024

Upgrade bech32 dependency #3181

Closed

3 tasks

tnull mentioned this pull request Aug 19, 2024

Move lightning::ln::types::* to lightning-types #3251

Open

apoelstra mentioned this pull request Sep 6, 2024

Option for Fe32->bytes conversion (fes_to_bytes) to not drop last partial values rust-bitcoin/rust-bech32#198

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swap the dep order between `lightning` and `lightning-invoice` #3234

Swap the dep order between `lightning` and `lightning-invoice` #3234

TheBlueMatt commented Aug 9, 2024

TheBlueMatt commented Aug 9, 2024

apoelstra commented Aug 9, 2024

TheBlueMatt commented Aug 9, 2024

codecov bot commented Aug 10, 2024 •

edited

Loading

tnull left a comment

tnull Aug 12, 2024

TheBlueMatt Aug 12, 2024

tnull Aug 13, 2024

tnull commented Aug 12, 2024

optout21 Aug 12, 2024

TheBlueMatt Aug 12, 2024

optout21 commented Aug 12, 2024

TheBlueMatt commented Aug 12, 2024

tnull Aug 13, 2024

TheBlueMatt Aug 13, 2024

tnull commented Aug 13, 2024

TheBlueMatt commented Aug 13, 2024 •

edited

Loading

tnull left a comment

vincenzopalazzo left a comment

vincenzopalazzo Aug 14, 2024

dunxen Aug 14, 2024

TheBlueMatt Aug 14, 2024 •

edited

Loading

dunxen left a comment

dunxen Aug 14, 2024

-[features]
+[features]
+default = ["std"]
+no-std = ["bitcoin/no-std"]
+std = ["bitcoin/std", "bech32/std"]

Swap the dep order between lightning and lightning-invoice #3234

Swap the dep order between lightning and lightning-invoice #3234

Conversation

TheBlueMatt commented Aug 9, 2024

TheBlueMatt commented Aug 9, 2024

apoelstra commented Aug 9, 2024

TheBlueMatt commented Aug 9, 2024

codecov bot commented Aug 10, 2024 • edited Loading

Codecov Report

tnull 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 commented Aug 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

optout21 commented Aug 12, 2024

TheBlueMatt commented Aug 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Aug 13, 2024

TheBlueMatt commented Aug 13, 2024 • edited Loading

tnull left a comment

Choose a reason for hiding this comment

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

dunxen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swap the dep order between `lightning` and `lightning-invoice` #3234

Swap the dep order between `lightning` and `lightning-invoice` #3234

codecov bot commented Aug 10, 2024 •

edited

Loading

TheBlueMatt commented Aug 13, 2024 •

edited

Loading

TheBlueMatt Aug 14, 2024 •

edited

Loading