-
Notifications
You must be signed in to change notification settings - Fork 364
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
BOLT 12 Offers utilities #2578
BOLT 12 Offers utilities #2578
Conversation
be8bf45
to
0ff5708
Compare
/// The message contents to send in an [`OnionMessage`]. | ||
/// | ||
/// [`OnionMessage`]: crate::ln::msgs::OnionMessage | ||
pub contents: T, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, if we leave a totally unbounded type that's gonna be a problem for bindings. Can we bound this by CustomOnionMessageContents
(or if we need a general bound we could create an OnionMessageContents
trait that we impl for BOLT12 messages as well)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need a more general bound since OffersMessage
is used, too.
lightning/src/ln/channelmanager.rs
Outdated
/// [`InvoiceRequest`]: crate::offers::invoice_request::InvoiceRequest | ||
/// [`InvoiceRequestBuilder`]: crate::offers::invoice_request::InvoiceRequestBuilder | ||
/// [`Bolt12Invoice`]: crate::offers::invoice::Bolt12Invoice | ||
pub fn request_invoice( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be pub? I'm not sure what the use is for this outside of simply paying an offer, which is presumably a different method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this is the "paying for an offer" method. I guess that I can rename it to be more user- rather than implementation-focused.
0ff5708
to
ff2702c
Compare
@@ -118,6 +118,13 @@ pub enum OnionMessageContents<T: CustomOnionMessageContents> { | |||
Custom(T), | |||
} | |||
|
|||
/// A type used within a variant of [`OnionMessageContents`]. | |||
pub trait OnionMessageContentsType {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than an empty trait, should we just rename CustomOnionMessageContents
to OnionMessageContents
and impl it for Bolt12/etc? Its already basically what we want - extends Write and has a tlv_type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... we already have an OnionMessageContents
enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, then lets use that in PendingOnionMessage
instead of a generic type T?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also seems arbitrary that a custom message handler isn't allowed to send BOLT12 messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we don't want a BOLT 12 handler to be able to send arbitrary ones. A custom handler can alway have their type wrap OffersMessage
in a variant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean...why not? We could also drop OnionMessageContents
or make it priv, and replace most usages with a trait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline and made the following changes:
- Renamed
OnionMessageContents
toParsedOnionMessageContents
and gave itpub(super)
visibility - Renamed
CustomOnionMessageContents
toOnionMessageContents
and used it as a trait bound on most methods - Changed Payload to hold
T
instead ofParsedOnionMessageContents<T>
whereT: OnionMessageContents
- Implemented
OnionMessageContents
forOffersMessage
andParsedOnionMessageContents
. The latter is needed so that arbitraryPayload
contents can be read and specific contents can be written
087941d
to
8116d6e
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2578 +/- ##
==========================================
- Coverage 89.04% 88.96% -0.09%
==========================================
Files 112 112
Lines 87527 87632 +105
Branches 87527 87632 +105
==========================================
+ Hits 77942 77963 +21
- Misses 7345 7435 +90
+ Partials 2240 2234 -6
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial look, just nits
@@ -208,19 +211,6 @@ mod tests { | |||
|
|||
#[test] | |||
fn test_no_onion_message_breakage() { | |||
let one_hop_om = "020000000000000000000000000000000000000000000000000000000000000e01055600020000000000000000000000000000000000000000000000000000000000000e01ae0276020000000000000000000000000000000000000000000000000000000000000002020000000000000000000000000000000000000000000000000000000000000e0101022a0000000000000000000000000000014551231950b75fc4402da1732fc9bebf00109500000000000000000000000000000004106d000000000000000000000000000000fd1092202a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005600000000000000000000000000000000000000000000000000000000000000"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind too much, but why remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restored it but now the test needs to check against InvalidFirstHop
, which I'm not sure if that is intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mm that shouldn't be intended. It looks like that's because the first-hop peer in the reply path isn't connected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will reexamine this but want to get the other changes pushed for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'd also be fine with saving this for follow-up if you want to note it in #1970.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, other than the introduction node being wrong, when hardcoding a fix I'm also seeing this logging:
Received an onion message with path_id Some([]) and a reply_path
Instead of the expected:
Received an onion message with path_id None and a reply_path
But I don't understand how my change would affect the path_id
. Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, nevermind on this. That was just because I had been building using some unpublished commits instead of during the rebase. 🤦♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #2671
8116d6e
to
2508211
Compare
2508211
to
7ddbd57
Compare
Rebased as there were a lot of merge conflicts from the last release. |
@@ -208,19 +211,6 @@ mod tests { | |||
|
|||
#[test] | |||
fn test_no_onion_message_breakage() { | |||
let one_hop_om = "020000000000000000000000000000000000000000000000000000000000000e01055600020000000000000000000000000000000000000000000000000000000000000e01ae0276020000000000000000000000000000000000000000000000000000000000000002020000000000000000000000000000000000000000000000000000000000000e0101022a0000000000000000000000000000014551231950b75fc4402da1732fc9bebf00109500000000000000000000000000000004106d000000000000000000000000000000fd1092202a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005600000000000000000000000000000000000000000000000000000000000000"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mm that shouldn't be intended. It looks like that's because the first-hop peer in the reply path isn't connected.
7ddbd57
to
bd42eb0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to look into addressing the fuzz test comment still.
I think this LGTM, I guess its now based on #2662, but once that lands feel free to squash. |
There's probably gonna be some conflicts between this and #2599 |
4f520cc
to
6b96866
Compare
e4e9fd7
to
7bf0469
Compare
Discussed offline. It would be better to keep the retry strategy separate from when to expire waiting for an invoice. |
Only comment left form my end is #2578 (comment), which you can fix or not, but either way lets squash and I'll ack :) |
Sorry needs a small-ish rebase after #2599. |
7bf0469
to
7a198a8
Compare
OnionMessageProvider is a super-trait of OnionMessageHandler, but they don't need to be used separately. Additionally, the former is misplaced in the events module. Remove OnionMessageProvider and add it's only method, next_onion_message_for_peer, into OnionMessageHandler.
OnionMessenger can send onion message responses from its handlers using respond_with_onion_message, which finds a path to the destination and enqueues the response for sending. Generalize this as it can be used not only for responses but for initial sends as well.
In preparation for needing the name OnionMessageContents for a trait to bound methods, rename it to ParsedOnionMessageContents. In the next commit, it's use will be limited to reading only, and the new trait will be a bound on method parameters instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after rebase
Rename CustomOnionMessageContents to OnionMessageContents and use it as a trait bound on messages passed to OnionMessenger methods. This allows using the trait in an upcoming commit as a bound on the contents of PendingOnionMessage. Also, make ParsedOnionMessageContent implement OnionMessageContents so that Payload can be bounded by OnionMessageContents directly, but used when either reading a ParsedOnionMessageContent or writing a specific type of OnionMessageContents (e.g., OffersMessage).
7a198a8
to
bf980c9
Compare
When constructing onion messages to send initially (opposed to replying to one from a handler), the user must construct an OnionMessagePath first before calling OnionMessener::send_onion_message. Additionally, having a reference to OnionMessener isn't always desirable. For instance, in an upcoming commit, ChannelManager will implement OffersMessageHandler, which OnionMessenger needs a reference to. If ChannelManager had a reference to OnionMessenger, too, there would be a dependency cycle. Instead, modify OffersMessageHandler and CustomOnionMessageHandler's interfaces to include a method for releasing pending onion messages. That way, ChannelManager may, for instance, construct and enqueue an InvoiceRequest for sending without needing a reference to OnionMessenger. Additionally, OnionMessenger has responsibility for path finding just as it does when replying to messages from a handler. It performs this when extracting messages from the handlers before returning the next message to send to a peer.
This allows for specifying the introduction node as the message recipient.
PendingOutboundPayment::AwaitingInvoice counts the number of timer ticks that have passed awaiting a Bolt12Invoice for an InvoiceRequest. When a constant INVOICE_REQUEST_TIMEOUT_TICKS has passed, the payment is forgotten. However, this mechanism is insufficient for the Refund scenario, where the Refund's expiration should be used instead. Change AwaitingInvoice to store an absolute expiry instead. When removing stale payments, pass the `SystemTime` in `std` and the highest block time minus two hours in `no-std`.
Add utility functions to ChannelManager for creating OfferBuilder, and RefundBuilder such that derived keys are used for the signing pubkey and payer id, respectively. This allows for stateless verification of any InvoiceRequest and Invoice messages. Later, blinded paths can be included in the returned builders. Also tracks future payments using the given PaymentId such that the corresponding Invoice is paid only once.
While this doesn't add much privacy over not including any blinded paths, it allows us to exercise code for receiving on blinded paths.
For consistency with other functions and doc cleanliness.
bf980c9
to
905028b
Compare
Rebased. Needed to expose |
/// Creates a [`RefundBuilder`] such that the [`Refund`] it builds is recognized by the | ||
/// [`ChannelManager`] when handling [`Bolt12Invoice`] messages for the refund. The builder will | ||
/// have the provided expiration set. Any changes to the expiration on the returned builder will | ||
/// not be honored by [`ChannelManager`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to mention the two-hour thing here, its a bit awkward its buried in the timer_tick_occurred
docs. Can happen in a followup, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in #2039.
/// [`ChannelManager`] when handling [`Bolt12Invoice`] messages for the refund. The builder will | ||
/// have the provided expiration set. Any changes to the expiration on the returned builder will | ||
/// not be honored by [`ChannelManager`]. | ||
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh also we should probably say something about how you can use abandon_payment
to revoke a refund once issued (if it hasn't been paid). Its somewhat non-intuitive to me that that is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in #2039.
0.0.118 - Oct 23, 2023 - "Just the Twelve Sinks" API Updates =========== * BOLT12 sending and receiving is now supported as an alpha feature. You may run into unexpected issues and will need to have a direct connection with the offer's blinded path introduction points as messages are not yet routed. We are seeking feedback from early testers (lightningdevkit#2578, lightningdevkit#2039). * `ConfirmationTarget` has been rewritten to provide information about the specific use LDK needs the feerate estimate for, rather than the generic low-, medium-, and high-priority estimates. This allows LDK users to more accurately target their feerate estimates (lightningdevkit#2660). For those wishing to retain their existing behavior, see the table below for conversion. * `ChainHash` is now used in place of `BlockHash` where it represents the genesis block (lightningdevkit#2662). * `lightning-invoice` payment utilities now take a `Deref` to `AChannelManager` (lightningdevkit#2652). * `peel_onion` is provided to statelessly decode an `OnionMessage` (lightningdevkit#2599). * `ToSocketAddrs` + `Display` are now impl'd for `SocketAddress` (lightningdevkit#2636, lightningdevkit#2670) * `Display` is now implemented for `OutPoint` (lightningdevkit#2649). * `Features::from_be_bytes` is now provided (lightningdevkit#2640). For those moving to the new `ConfirmationTarget`, the new variants in terms of the old mempool/low/medium/high priorities are as follows: * `OnChainSweep` = `HighPriority` * `MaxAllowedNonAnchorChannelRemoteFee` = `max(25 * 250, HighPriority * 10)` * `MinAllowedAnchorChannelRemoteFee` = `MempoolMinimum` * `MinAllowedNonAnchorChannelRemoteFee` = `Background - 250` * `AnchorChannelFee` = `Background` * `NonAnchorChannelFee` = `Normal` * `ChannelCloseMinimum` = `Background` Bug Fixes ========= * Calling `ChannelManager::close_channel[_with_feerate_and_script]` on a channel which did not exist would immediately hang holding several key `ChannelManager`-internal locks (lightningdevkit#2657). * Channel information updates received from a failing HTLC are no longer applied to our `NetworkGraph`. This prevents a node which we attempted to route a payment through from being able to learn the sender of the payment. In some rare cases, this may result in marginally reduced payment success rates (lightningdevkit#2666). * Anchor outputs are now properly considered when calculating the amount available to send in HTLCs. This can prevent force-closes in anchor channels when sending payments which overflow the available balance (lightningdevkit#2674). * A peer that sends an `update_fulfill_htlc` message for a forwarded HTLC, then reconnects prior to sending a `commitment_signed` (thus retransmitting their `update_fulfill_htlc`) may result in the channel stalling and being unable to make progress (lightningdevkit#2661). * In exceedingly rare circumstances, messages intended to be sent to a peer prior to reconnection can be sent after reconnection. This could result in undefined channel state and force-closes (lightningdevkit#2663). Backwards Compatibility ======================= * Creating a blinded path to receive a payment then downgrading to LDK prior to 0.0.117 may result in failure to receive the payment (lightningdevkit#2413). * Calling `ChannelManager::pay_for_offer` or `ChannelManager::create_refund_builder` may prevent downgrading to LDK prior to 0.0.118 until the payment times out and has been removed (lightningdevkit#2039). Node Compatibility ================== * LDK now sends a bogus `channel_reestablish` message to peers when they ask to resume an unknown channel. This should cause LND nodes to force-close and broadcast the latest channel state to the chain. In order to trigger this when we wish to force-close a channel, LDK now disconnects immediately after sending a channel-closing `error` message. This should result in cooperative peers also working to confirm the latest commitment transaction when we wish to force-close (lightningdevkit#2658). Security ======== 0.0.118 expands mitigations against transaction cycling attacks to non-anchor channels, though note that no mitigations which exist today are considered robust to prevent the class of attacks. * In order to mitigate against transaction cycling attacks, non-anchor HTLC transactions are now properly re-signed before broadcasting (lightningdevkit#2667). In total, this release features 61 files changed, 3470 insertions, 1503 deletions in 85 commits from 12 authors, in alphabetical order: * Antonio Yang * Elias Rohrer * Evan Feenstra * Fedeparma74 * Gursharan Singh * Jeffrey Czyz * Matt Corallo * Sergi Delgado Segura * Vladimir Fomene * Wilmer Paulino * benthecarman * slanesuke
Add utility methods to
ChannelManager
for:OfferBuilder
such that derived keys are used for the signing pubkey,RefundBuilder
such that derived keys are used for the payer id, andInvoiceRequest
for anOffer
such that derived keys are used for the payer idWhen the
ChannelManager
is the payer (i.e., when creatingRefund
andInvoiceRequest
), aPaymentId
is required and used for tracking the outbound payment once theBolt12Invoice
is received.One-hop blinded paths are used throughout until multi-hop blinded paths are supported.
Offer message handling and sending a
Bolt12Invoice
for aRefund
such that derived keys are used for the signing pubkey are in #2039, which is based on this PR.