Skip to content
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

Backports for 0.1.0 #3536

Merged
merged 12 commits into from
Jan 16, 2025
Merged

Conversation

TheBlueMatt
Copy link
Collaborator

@TheBlueMatt TheBlueMatt commented Jan 14, 2025

This includes backports for #3518, #3520, #3521, #3522, #3524, #3528, #3530, #3533, #3535, and #3538.

jkczyz and others added 5 commits January 14, 2025 19:45
Lack of bindings support was because the method used to return a slice
of tuples, it seems. Now that it returns &[BlindedPaymentPath], bindings
should be possible given that they can be generated for
Bolt12Invoice::message_paths.
When either the amount or the `max_total_cltv_expiry_delta` are
set to max-value, `set_max_path_length` can trigger overflows in
`build_onion_payloads_callback`, leading to debug-panics.
With the `Confirm` interface, transaction confirmations can come
in at any time, so asserting that a confirmation is more recent
than the last time we broadcasted a transaction can lead to
spurious assertion failures.
.. and bump its MSRV to 1.75.

Recently, `rustls` bumped their MSRV to 1.71. As we depend on them and
don't want to continuously pin this security-critical dependency back,
we have no choice left but to bump the MSRV for
`lightning-transaction-sync` to a version >= 1.71, too.

Here, we hence move the `lightning-transaction-sync` tests to a
dedicated script and propose to introduce a secondary MSRV of 1.75.

We chose this particular version, because:

a) it's > 1 year old
b) it provides a buffer to 1.71, i.e., if some crate bumped to a version
 > 1.71, there is a chance we don't immediately have to react again
c) it
 stabilized `async fn`s in traits (see
 https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html),
 which might become handy for related (BDK) crates, which hopefully will
 adopt the same target.
@TheBlueMatt TheBlueMatt added this to the 0.1 milestone Jan 14, 2025
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

Cherry picks LGTM.

@valentinewallace
Copy link
Contributor

Note that we also tagged #3531 for backport

@TheBlueMatt
Copy link
Collaborator Author

Sure, but #3531 hasn't landed, and I'm not sure we need to hold 0.1.0 on it :).

When scanning confirmed transactions for spends that conflict with our
existing packages, we should continue scanning after detecting the first
conflicting package since a transaction can conflict with multiple
packages.

This ensures that we remove *all* inputs from our packages that have
already been spent by the counterparty so that valid claim transactions
are generated.

Fixes lightningdevkit#3537.
@dunxen
Copy link
Contributor

dunxen commented Jan 15, 2025

Ah, CI failure reminded me we need to backport this: #3518.

Just tagged it.

Rust 1.84.0 was recently released along with some new clippy lints, one
of which is `unnecessary_map_or`. Unfortunately this lint suggests using
`Option::is_some_and` as a fix, but this is only available in Rust
 version >= 1.70, while we still have an MSRV of 1.63. So we silence that
lint for now.

We'd still like our lint CI to use stable Rust so that we can benefit from
new lint checks which may be helpful and don't require an MSRV bump, but
sometimes new lints (like in this case) do.

See:
  https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_map_or
  https://doc.rust-lang.org/std/option/enum.Option.html#method.is_some_and
tnull and others added 3 commits January 15, 2025 14:13
To trigger message processing, we previously had the user set a callback
to `PeerManager::process_events` via an `Fn()` callback. This is however
not supported by `c_bindings`.

Here, we therefore introduce as `ProcessMesssagesCallback` trait that
can be used via `LiquidityManager::set_process_msgs_callback_fn`, which
is exposed in `c_bindings`.
Add a check to ensure that the amount_msats in an invoice matches the
amount_msats specified in the invoice_request or offer (or refund).
Reject the invoice as invalid if there is a mismatch between these
amounts. Otherwise, an invoice may be paid with an amount greater than
the requested amount.

Co-authored-by: Ian Slane <[email protected]>
Co-authored-by: Jeffrey Czyz <[email protected]>
When InvoiceRequest::amount_msats returns Some, it may have been
inferred from the Offer::amount and InvoiceRequest::quantity. Add a
method to InvoiceRequest for determining if the amount was explicitly
set.
@TheBlueMatt
Copy link
Collaborator Author

Okay, this should be good to land now (once #3530 lands).

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

Attention: Patch coverage is 97.89474% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.33%. Comparing base (c8ec8eb) to head (be1a3ab).

Files with missing lines Patch % Lines
lightning/src/offers/invoice.rs 93.93% 4 Missing ⚠️
lightning/src/ln/functional_tests.rs 98.61% 3 Missing ⚠️
lightning/src/offers/invoice_request.rs 98.27% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              0.1    #3536      +/-   ##
==========================================
+ Coverage   88.29%   88.33%   +0.04%     
==========================================
  Files         149      149              
  Lines      112930   113281     +351     
  Branches   112930   113281     +351     
==========================================
+ Hits        99707   100066     +359     
- Misses      10732    10733       +1     
+ Partials     2491     2482       -9     

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

Static invoices don't have an amount_msats field.
@TheBlueMatt
Copy link
Collaborator Author

Ugh, okay, now should be good.

@TheBlueMatt TheBlueMatt merged commit 2a642f8 into lightningdevkit:0.1 Jan 16, 2025
24 of 25 checks passed
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.

7 participants