0.0.123 Backports #3042
Merged
0.0.123 Backports #3042
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If an Offer contains a path, the blinded_node_id of the path's final hop can be used as the signing pubkey. Make Offer::signing_pubkey and OfferContents::signing_pubkey return an Option to support this. Upcoming commits will implement this behavior.
If an offer has at least one path, it may omit the signing pubkey and use the blinded node id of the last hop of a path to sign an invoice. Allow parsing such offers but not yet creating them.
Instead of reusing OfferTlvStream::paths, add a dedicated paths TLV to InvoiceRequestTlvStream such that it can be used in Refund. This allows for an Offer without a signing_pubkey and still be able to differentiate whether an invoice is for an offer or a refund.
When parsing a Bolt12Invoice use both the Offer's signing_pubkey and paths to determine if it is for an Offer or a Refund. Previously, an Offer was required to have a signing_pubkey. But now that it is optional, the Offers paths can be used to make the determination. Additionally, check that the invoice matches one of the blinded node ids from the paths' last hops.
Offers currently require a description, though this may change to be optional. Remove the description requirement from the API, setting and empty string by default.
Refunds currently require a description, though this may change to be optional. Remove the description requirement from the API, setting and empty string by default.
PaymentContext is already stored in ClaimablePayment via PaymentPurpose, so there is no need to repeat it in each ClaimableHTLC via OnionPayload. This avoids cloning the PaymentContext each time an HTLC is received, other than for the first HTLC for a payment.
Event::PaymentClaimable and Event::PaymentClaimed already contain the paid amount, so there's no need to included the requested amount in InvoiceRequestFields.
InvoiceRequestFeatures may contain a large, odd bit. Including this in InvoiceRequestFields, which is in each BlindedPath of a Bolt12Invoice, could cause the invoice's onion message to exceed the maximum size. The features are already checked before sending an invoice.
…followups Follow-ups to lightningdevkit#2970
…pubkey Sending to `Offer` without `signing_pubkey`
In `funding_transaction_generated_intern`, if `find_funding_output` fails (e.g. due to the require output not being present in the provided funding transaction) we'd previously not generated a `ChannelClosed` event which leaves users possibly in a confused state. Here we fix this, also fixing the relevant tests to check for the new event. Fixes lightningdevkit#2843.
If we fail to fund a batch open we'll force-close all channels in the batch, however would previously fail to send error messages to our peers for any channels we were due to test after the one that failed. This commit fixes that issue, sending the required error messages to allow our peers to clean up their state.
rustc now warns any time a `#[macro_export]` is used inside a
function as it generates surprising results. Because doctests are
run inside implicit test functions this means any use of
`#[macro_export]` inside a doctest will now warn. We do this in
`lightning-custom-message` to demonstrated how to use the crate,
which now fails to compile.
Here we fix this by adding an `fn main() {}` to the doctest, which
causes doctests to be compiled as freestanding code rather than
inside a test function.
Note that while discussing this upstream it came up that rustc is
also planning on changing the way doctests are compiled to compile
an entire crate's doctests in one go rather than in separate
crates, causing doctests to have a shared namespace which may
generate future surprising outcomes.
…rustc-warning Fix new rustc `#[macro_export]` warning in doctests
…-pico-btc-overflow Fix overflow in lightning-invoice `amount_pico_btc`
`onion::message::messenger::PeeledOnion` is a public enum which included the private enum `NextHop`, which is not acceptable. Thus, we here expose `NextHop` but rename it `NextMessageHop` to make clear that it is specific to messages.
We don't actually intend these to be public as they're just for docs but the bindings don't currently parse `#[doc(hidden)]` as "no-export" so we add manual no-export tags as well.
The `PaymentHash`, `PaymentSecret`, `PaymentPreimage`, and `ChannelId` types are all small wrappers around `[u8; 32]` and are used throughout the codebase but were defined in the top-level `ln/mod.rs` file and the relatively sparsely-populated `ln/channel_id.rs` file. Here we move them to a common `types` module and go ahead and update all our in-crate `use` statements to refer to the new module for bindings. We do, however, leave a `pub use` alias for the old paths to avoid upgrade hassle for users.
When we receive a new block we may generate `Event::SpendableOutputs` in `ChannelMonitor`s which then need to be processed by the background processor. While it will do so eventually when its normal loop goes around, this may cause user tests to be delayed in finding events, so we should notify the BP immediately to wake it on new blocks. We implement that here, unconditionally notifying the `background-processor` whenever we receive a new block or confirmed transactions.
…fy-bp-on-blocks Wake `background-processor` from `ChainMonitor` on new blocks
In most cases we already call both in a pair, and in fact always consolidate some of the returned values across both accessors, so there's not much reason to have them be separate methods. Here we merge them.
The previous commit left indentation in `get_pending_htlc_stats` deliberately wrong, to ease reviewability. This commit fixes the indentation.
As LDK changes, `UserConfig::default()` may imply marginally different behavior, whereas `test_default_channel_config` is intended to tweak defaults to provide a stable behavior for test contexts. This commit changes a few uses of `UserConfig::default()` to `test_default_channel_config` in cases that will fail over the coming commits due to dust changes.
The spec was changed to allow excluding an offer description if the offer doesn't have an amount. However, it is still required when the amount is set.
…batch-funding-failures Add error handling for channels which fail to be created in `funding_transaction_generated_intern`
…description Optional description for `Offer` and `Refund`
…ream-123-bindings
…followups Follow-ups to lightningdevkit#2970
…pubkey Sending to `Offer` without `signing_pubkey`
…rustc-warning Fix new rustc `#[macro_export]` warning in doctests
…-pico-btc-overflow Fix overflow in lightning-invoice `amount_pico_btc`
…fy-bp-on-blocks Wake `background-processor` from `ChainMonitor` on new blocks
…batch-funding-failures Add error handling for channels which fail to be created in `funding_transaction_generated_intern`
…description Optional description for `Offer` and `Refund`
…ream-123-bindings
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## 0.0.123 #3042 +/- ##
===========================================
+ Coverage 89.16% 89.82% +0.65%
===========================================
Files 118 116 -2
Lines 97726 96425 -1301
Branches 97726 96425 -1301
===========================================
- Hits 87142 86614 -528
+ Misses 8345 7235 -1110
- Partials 2239 2576 +337 ☔ View full report in Codecov by Sentry. |
…-invoice-amt-msats-overflow Fix overflow in invoice amount setter.
Transaction fees on counterparty commitment transactions are ultimately not our money and thus are really "dust" from our PoV - they're funds that may be ours during off-chain updates but are not ours once we go on-chain. Thus, here, we count any such fees in excess of our own fee estimates towards dust exposure. We don't bother to make an inbound/outbound channel distinction here as in most cases users will use `MaxDustExposure::FeeRateMultiplier` which will scale with the fee we set on outbound channels anyway. Note that this also enables the dust exposure checks on anchor channels during feerate updates. We'd previously elided these as increases in the channel feerates do not change the HTLC dust exposure, but now do for the fee dust exposure.
Now that we're including excess counterparty commitment transaction fees in our dust calculation, we need to update the docs accordingly. We do so here, describing some of the considerations and risks that come with the new changes. We also take this opportunity to double the default value, as users have regularly complained that non-anchor channels fail to send HTLCs with the default settings with some feerates. Fixes lightningdevkit#2922
…-are-dust Include excess counterparty commitment transaction fees in dust exposure
|
Okay, included 3045 which should be sufficient for an RC, let's land this! |
valentinewallace
approved these changes
May 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backports of everything merged to main since the fork.