Optional description for `Offer` and `Refund` by jkczyz · Pull Request #3018 · lightningdevkit/rust-lightning

jkczyz · 2024-04-24T21:15:10Z

Currently, Offer::description and Refund::description are required in the spec, but this may be relaxed. Instead of requiring these in the public API, default to using an empty string and expose a method in the builders to set them.

Based on #3017

codecov-commenter · 2024-04-24T21:24:37Z

Codecov Report

Attention: Patch coverage is 95.18600% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 90.54%. Comparing base (2c0fcf2) to head (e61001f).
Report is 45 commits behind head on main.

Files	Patch %	Lines
lightning/src/offers/invoice.rs	95.12%	2 Missing and 4 partials ⚠️
lightning/src/offers/offer.rs	95.65%	4 Missing and 2 partials ⚠️
lightning/src/offers/refund.rs	90.90%	5 Missing ⚠️
lightning/src/offers/invoice_request.rs	96.70%	3 Missing ⚠️
lightning/src/ln/channelmanager.rs	87.50%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #3018 +/- ##
==========================================
+ Coverage 89.13% 90.54% +1.40% 
==========================================
Files 118 117 -1 Lines 97492 100470 +2978 Branches 97492 100470 +2978 ==========================================
+ Hits 86903 90971 +4068 + Misses 8349 7009 -1340 - Partials 2240 2490 +250

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

TheBlueMatt · 2024-04-25T21:13:42Z

Dont we need to update to the new spec that allows an actually-empty description with no amount?

jkczyz · 2024-04-26T15:00:52Z

Dont we need to update to the new spec that allows an actually-empty description with no amount?

Guessing no since a zero-length TLV is valid. The spec says a writer "MUST set offer_description to a complete description of the purpose of the payment." So it may depend on how strict you want to interpret that.

jkczyz · 2024-04-26T15:39:30Z

Dont we need to update to the new spec that allows an actually-empty description with no amount?
Guessing no since a zero-length TLV is valid. The spec says a writer "MUST set offer_description to a complete description of the purpose of the payment." So it may depend on how strict you want to interpret that.

Err... may have misinterpreted your comment. Spec has been updated as, AFAICT: lightning/bolts@f150782

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.

TheBlueMatt · 2024-04-29T22:10:46Z

Don't we need to drop this failure?

rust-lightning/lightning/src/offers/offer.rs

Line 1087 in 2b14cc4

None => returnErr(Bolt12SemanticError::MissingDescription),

jkczyz · 2024-04-29T23:12:32Z

Don't we need to drop this failure?
rust-lightning/lightning/src/offers/offer.rs
Line 1087 in 2b14cc4
None => returnErr(Bolt12SemanticError::MissingDescription),

This PR just changes the API. We still require at least a non-empty string. Though in the latest spec it seems we can drop the error if the amount is also empty.

TheBlueMatt · 2024-04-30T00:48:02Z

Right, it seems like we should go ahead and do that here?

valentinewallace

LGTM after Matt's feedback

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.

jkczyz · 2024-05-02T21:45:08Z

Alright, description is now an Option and will be set to Some(String::new()) in OfferBuilder if an amount is not set. Parsing also updated.

valentinewallace · 2024-05-03T16:27:04Z

LGTM once Matt takes a look!

TheBlueMatt

LGTM. Treating @valentinewallace's "LGTM" as an ACK and merging.

The merge-base changed after approval.

TheBlueMatt

Not sure why github is insistent that my review doesn't count...

…description Optional description for `Offer` and `Refund`

v0.0.123 - May 08, 2024 - "BOLT12 Dust Sweeping" API Updates =========== * To reduce risk of force-closures and improve HTLC reliability the default dust exposure limit has been increased to `MaxDustHTLCExposure::FeeRateMultiplier(10_000)`. Users with existing channels might want to consider using `ChannelManager::update_channel_config` to apply the new default (lightningdevkit#3045). * `ChainMonitor::archive_fully_resolved_channel_monitors` is now provided to remove from memory `ChannelMonitor`s that have been fully resolved on-chain and are now not needed. It uses the new `Persist::archive_persisted_channel` to inform the storage layer that such a monitor should be archived (lightningdevkit#2964). * An `OutputSweeper` is now provided which will automatically sweep `SpendableOutputDescriptor`s, retrying until the sweep confirms (lightningdevkit#2825). * After initiating an outbound channel, a peer disconnection no longer results in immediate channel closure. Rather, if the peer is reconnected before the channel times out LDK will automatically retry opening it (lightningdevkit#2725). * `PaymentPurpose` now has separate variants for BOLT12 payments, which include fields from the `invoice_request` as well as the `OfferId` (lightningdevkit#2970). * `ChannelDetails` now includes a list of in-flight HTLCs (lightningdevkit#2442). * `Event::PaymentForwarded` now includes `skimmed_fee_msat` (lightningdevkit#2858). * The `hashbrown` dependency has been upgraded and the use of `ahash` as the no-std hash table hash function has been removed. As a consequence, LDK's `Hash{Map,Set}`s no longer feature several constructors when LDK is built with no-std; see the `util::hash_tables` module instead. On platforms that `getrandom` supports, setting the `possiblyrandom/getrandom` feature flag will ensure hash tables are resistant to HashDoS attacks, though the `possiblyrandom` crate should detect most common platforms (lightningdevkit#2810, lightningdevkit#2891). * `ChannelMonitor`-originated requests to the `ChannelSigner` can now fail and be retried using `ChannelMonitor::signer_unblocked` (lightningdevkit#2816). * `SpendableOutputDescriptor::to_psbt_input` now includes the `witness_script` where available as well as new proprietary data which can be used to re-derive some spending keys from the base key (lightningdevkit#2761, lightningdevkit#3004). * `OutPoint::to_channel_id` has been removed in favor of `ChannelId::v1_from_funding_outpoint` in preparation for v2 channels with a different `ChannelId` derivation scheme (lightningdevkit#2797). * `PeerManager::get_peer_node_ids` has been replaced with `list_peers` and `peer_by_node_id`, which provide more details (lightningdevkit#2905). * `Bolt11Invoice::get_payee_pub_key` is now provided (lightningdevkit#2909). * `Default[Message]Router` now take an `entropy_source` argument (lightningdevkit#2847). * `ClosureReason::HTLCsTimedOut` has been separated out from `ClosureReason::HolderForceClosed` as it is the most common case (lightningdevkit#2887). * `ClosureReason::CooperativeClosure` is now split into `{Counterparty,Locally}Initiated` variants (lightningdevkit#2863). * `Event::ChannelPending::channel_type` is now provided (lightningdevkit#2872). * `PaymentForwarded::{prev,next}_user_channel_id` are now provided (lightningdevkit#2924). * Channel init messages have been refactored towards V2 channels (lightningdevkit#2871). * `BumpTransactionEvent` now contains the channel and counterparty (lightningdevkit#2873). * `util::scid_utils` is now public, with some trivial utilities to examine short channel ids (lightningdevkit#2694). * `DirectedChannelInfo::{source,target}` are now public (lightningdevkit#2870). * Bounds in `lightning-background-processor` were simplified by using `AChannelManager` (lightningdevkit#2963). * The `Persist` impl for `KVStore` no longer requires `Sized`, allowing for the use of `dyn KVStore` as `Persist` (lightningdevkit#2883, lightningdevkit#2976). * `From<PaymentPreimage>` is now implemented for `PaymentHash` (lightningdevkit#2918). * `NodeId::from_slice` is now provided (lightningdevkit#2942). * `ChannelManager` deserialization may now fail with `DangerousValue` when LDK's persistence API was violated (lightningdevkit#2974). Bug Fixes ========= * Excess fees on counterparty commitment transactions are now included in the dust exposure calculation. This lines behavior up with some cases where transaction fees can be burnt, making them effectively dust exposure (lightningdevkit#3045). * `Future`s used as an `std::...::Future` could grow in size unbounded if it was never woken. For those not using async persistence and using the async `lightning-background-processor`, this could cause a memory leak in the `ChainMonitor` (lightningdevkit#2894). * Inbound channel requests that fail in `ChannelManager::accept_inbound_channel` would previously have stalled from the peer's perspective as no `error` message was sent (lightningdevkit#2953). * Blinded path construction has been tuned to select paths more likely to succeed, improving BOLT12 payment reliability (lightningdevkit#2911, lightningdevkit#2912). * After a reorg, `lightning-transaction-sync` could have failed to follow a transaction that LDK needed information about (lightningdevkit#2946). * `RecipientOnionFields`' `custom_tlvs` are now propagated to recipients when paying with blinded paths (lightningdevkit#2975). * `Event::ChannelClosed` is now properly generated and peers are properly notified for all channels that as a part of a batch channel open fail to be funded (lightningdevkit#3029). * In cases where user event processing is substantially delayed such that we complete multiple round-trips with our peers before a `PaymentSent` event is handled and then restart without persisting the `ChannelManager` after having persisted a `ChannelMonitor[Update]`, on startup we may have `Err`d trying to deserialize the `ChannelManager` (lightningdevkit#3021). * If a peer has relatively high latency, `PeerManager` may have failed to establish a connection (lightningdevkit#2993). * `ChannelUpdate` messages broadcasted for our own channel closures are now slightly more robust (lightningdevkit#2731). * Deserializing malformed BOLT11 invoices may have resulted in an integer overflow panic in debug builds (lightningdevkit#3032). * In exceedingly rare cases (no cases of this are known), LDK may have created an invalid serialization for a `ChannelManager` (lightningdevkit#2998). * Message processing latency handling BOLT12 payments has been reduced (lightningdevkit#2881). * Latency in processing `Event::SpendableOutputs` may be reduced (lightningdevkit#3033). Node Compatibility ================== * LDK's blinded paths were inconsistent with other implementations in several ways, which have been addressed (lightningdevkit#2856, lightningdevkit#2936, lightningdevkit#2945). * LDK's messaging blinded paths now support the latest features which some nodes may begin relying on soon (lightningdevkit#2961). * LDK's BOLT12 structs have been updated to support some last-minute changes to the spec (lightningdevkit#3017, lightningdevkit#3018). * CLN v24.02 requires the `gossip_queries` feature for all peers, however LDK by default does not set it for those not using a `P2PGossipSync` (e.g. those using RGS). This change was reverted in CLN v24.02.2 however for now LDK always sets the `gossip_queries` feature. This change is expected to be reverted in a future LDK release (lightningdevkit#2959). Security ======== 0.0.123 fixes a denial-of-service vulnerability which we believe to be reachable from untrusted input when parsing invalid BOLT11 invoices containing non-ASCII characters. * BOLT11 invoices with non-ASCII characters in the human-readable-part may cause an out-of-bounds read attempt leading to a panic (lightningdevkit#3054). Note that all BOLT11 invoices containing non-ASCII characters are invalid. In total, this release features 150 files changed, 19307 insertions, 6306 deletions in 360 commits since 0.0.121 from 17 authors, in alphabetical order: * Arik Sosman * Duncan Dean * Elias Rohrer * Evan Feenstra * Jeffrey Czyz * Keyue Bao * Matt Corallo * Orbital * Sergi Delgado Segura * Valentine Wallace * Willem Van Lint * Wilmer Paulino * benthecarman * jbesraa * olegkubrakov * optout * shaavan

jkczyz marked this pull request as ready for review April 24, 2024 22:42

jkczyz requested a review from TheBlueMatt April 25, 2024 14:08

TheBlueMatt added this to the 0.0.123 milestone Apr 25, 2024

jkczyz added 6 commits April 26, 2024 18:14

Optional OfferContents::signing_pubkey
520fecf
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.

Allow parsing Offer without signing_pubkey
94d5af6
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.

Don't require Offer::description in API
48cba29
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.

Don't require Refund::description in API
db7e696
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.

jkczyz force-pushed the 2024-04-optional-description branch from 1814a22 to db7e696Compare April 26, 2024 23:14

valentinewallace reviewed Apr 30, 2024
View reviewed changes

Comment threadlightning/src/offers/offer.rs Outdated

Only require description when offer has an amount
e61001f
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.

TheBlueMatt previously approved these changes May 3, 2024
View reviewed changes

TheBlueMatt approved these changes May 3, 2024
View reviewed changes

TheBlueMatt merged commit a27088d into lightningdevkit:mainMay 3, 2024

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request May 6, 2024
Merge pull requestlightningdevkit#3018from jkczyz/2024-04-optional-…
72870aa
…description Optional description for `Offer` and `Refund`

Navigation Menu

Uh oh!

Optional description for `Offer` and `Refund` - #3018

Optional description for `Offer` and `Refund`#3018
TheBlueMatt merged 7 commits into
lightningdevkit:mainfrom
jkczyz:2024-04-optional-description

jkczyz commented Apr 24, 2024

Uh oh!

codecov-commenter commented Apr 24, 2024•
edited
Loading

Uh oh!

TheBlueMatt commented Apr 25, 2024

Uh oh!

jkczyz commented Apr 26, 2024

Uh oh!

jkczyz commented Apr 26, 2024

Uh oh!

TheBlueMatt commented Apr 29, 2024

Uh oh!

jkczyz commented Apr 29, 2024

Uh oh!

TheBlueMatt commented Apr 30, 2024

Uh oh!

valentinewallace left a comment

Uh oh!

Uh oh!

jkczyz commented May 2, 2024

Uh oh!

valentinewallace commented May 3, 2024

Uh oh!

TheBlueMatt left a comment

Uh oh!

TheBlueMatt left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

jkczyz commented Apr 24, 2024

Uh oh!

codecov-commenter commented Apr 24, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt commented Apr 25, 2024

Uh oh!

jkczyz commented Apr 26, 2024

Uh oh!

jkczyz commented Apr 26, 2024

Uh oh!

TheBlueMatt commented Apr 29, 2024

Uh oh!

jkczyz commented Apr 29, 2024

Uh oh!

TheBlueMatt commented Apr 30, 2024

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkczyz commented May 2, 2024

Uh oh!

valentinewallace commented May 3, 2024

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Apr 24, 2024•
edited
Loading