Rename `Offer::signing_pubkey` to `Offer::issuer_id` #3218

jkczyz · 2024-08-01T22:50:54Z

The spec was recently changed to use offer_issuer_id instead of offer_node_id. LDK always used signing_pubkey to avoid confusion with a node_id. Rename it to issuer_id now that the spec is cleared up. Continue to use signing_pubkey in signing contexts.

Also, add issuer_id methods to invoice structs. Update test vectors from earlier spec changes.

codecov · 2024-08-01T23:01:24Z

Codecov Report

Attention: Patch coverage is 93.59606% with 13 lines in your changes missing coverage. Please review.

Project coverage is 89.62%. Comparing base (6662c5c) to head (e11025f).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/offers/invoice.rs	70.96%	9 Missing ⚠️
lightning/src/ln/channelmanager.rs	71.42%	1 Missing and 1 partial ⚠️
lightning/src/offers/invoice_request.rs	96.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3218      +/-   ##
==========================================
- Coverage   89.65%   89.62%   -0.04%     
==========================================
  Files         126      126              
  Lines      102546   102564      +18     
  Branches   102546   102564      +18     
==========================================
- Hits        91935    91918      -17     
- Misses       7890     7916      +26     
- Partials     2721     2730       +9

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

valentinewallace · 2024-08-01T23:02:21Z

rustfmt check is failing

vincenzopalazzo

LGTM, modulo CI.

vincenzopalazzo · 2024-08-02T07:08:19Z

lightning/src/ln/channelmanager.rs

-		let builder = OfferBuilder::deriving_signing_pubkey(node_id, expanded_key, nonce, secp_ctx)
+		let builder = OfferBuilder::deriving_issuer_id(node_id, expanded_key, nonce, secp_ctx)


I assume that this is a would-like-to-have review on Bolts lightning/bolts#798 (comment) ?

Anyway, I like the bolt idea invoice_signing_pubkey to make all less confusing.

Yeah, though we already use signing_pubkey in LDK. Updating the spec would just mean we update the name in InvoiceTlvStream and InvoiceTlvStreamRef.

TheBlueMatt · 2024-08-05T18:30:49Z

Needs rebase.

lightning/src/offers/invoice_macros.rs

TheBlueMatt · 2024-08-13T13:47:19Z

Feel free to squash.

TheBlueMatt · 2024-08-13T13:49:19Z

lightning/src/ln/channelmanager.rs

@@ -8973,7 +8973,7 @@ where
 	/// # Limitations
 	///
 	/// Requires a direct connection to an introduction node in [`Offer::paths`] or to
-	/// [`Offer::signing_pubkey`], if empty. A similar restriction applies to the responding
+	/// [`Offer::issuer_id`], if empty. A similar restriction applies to the responding


I strongly prefer the signing_pubkey term, as issuer_id implies some kind of persistent ID and this absolutely should not be persistent. I don't think we should change it in the public API.

Don't we have the same problem with payer_id?

Yea, and we should probably rename that IMO :).

Hmm... I thought the reason the spec was changed from node_id to issuer_id was because node_id implied it was some persistent id. Maybe issuer_signing_pubkey and payer_signing_pubkey?

Hmm... I thought the reason the spec was changed from node_id to issuer_id was because node_id implied it was some persistent id.

Right, which is why we never did node_id :) But IMO _id is also a problem.

Maybe issuer_signing_pubkey and payer_signing_pubkey?

Or invoice_signing_pubkey and invreq_signing_pubkey to indicate that these are keys specific to this inv/req, and not specific to the issuer/payer.

Or invoice_signing_pubkey and invreq_signing_pubkey to indicate that these are keys specific to this inv/req, and not specific to the issuer/payer.

I'm not so keen on those. InvoiceRequest::invreq_signing_pubkey is redundant and Offer::invoice_signing_pubkey would make it such that we have both Bolt12Invoice::invoice_signing_pubkey -- also redundant and unclear where it is coming from -- and Bolt12Invoice::signing_pubkey.

Using "issuer" and "payer" is clearer, IMO. It's not that the signing pubkey is specific to the issuer/payer but rather belongs to them.

Or thought of another way, it is specific to the issuer/payer and thus it actually is an id. It's just that they can have multiple identities -- and thus ids -- that cannot be correlated. But happy to use issuer_signing_pubkey and payer_signing_pubkey if you prefer to not use "id".

Sure, lets just do *_signing_pubkey. I think the "its an ID but they have multiple" is the wrong thing we should be communicating - we really need to communicate clearly and repeatedly that these are keys that sign the objects and no other objects, cause we've already seen lots of confusion on this.

PTAL, the latest push renamed both issuer_id and payer_id in the public API.

shaavan

Looks good to me, with just a few suggestions!

shaavan · 2024-08-29T13:03:59Z

lightning/src/offers/invoice.rs

+	/// The public key used by the recipient to sign invoices.
+	///
+	/// From [`Offer::issuer_signing_pubkey`] and may be `None`; also `None` if the invoice was
+	/// created in response to a [`Refund`].
+	///
+	/// [`Offer::issuer_signing_pubkey`]: crate::offers::offer::Offer::issuer_signing_pubkey


Maybe we can reword it a little bit to make it more clear. How about?

Suggested change

/// The public key used by the recipient to sign invoices.

///

/// From [`Offer::issuer_signing_pubkey`] and may be `None`; also `None` if the invoice was

/// created in response to a [`Refund`].

///

/// [`Offer::issuer_signing_pubkey`]: crate::offers::offer::Offer::issuer_signing_pubkey

/// Returns the public key used by the recipient to sign invoices.

/// The returned value varies based on the context in which the invoice was created:

/// If the invoice was created in response to an [`Offer`] then it returns the [`Offer::issuer_signing_pubkey`]

/// If the invoice was created in response to a [`Refund`] then it returns `None`.

///

/// [`Offer::issuer_signing_pubkey`]: crate::offers::offer::Offer::issuer_signing_pubkey

This isn't correct. It may return None when created in response to an Offer as detailed in Offer::issuer_signing_pubkey. Rather than repeat those docs, I'd rather just link to them. Added some clarification in those docs for when it is None, though.

shaavan · 2024-08-29T13:11:15Z

lightning/src/offers/static_invoice.rs

+	/// The public key corresponding to the key used to sign the invoice.
+	///
+	/// This will be:
+	/// - [`Offer::issuer_signing_pubkey`] if `Some`, otherwise
+	/// - the final blinded node id from a [`BlindedPath`] in [`Offer::paths`] if `None`.
+	///
+	/// [`Offer::issuer_signing_pubkey`]: crate::offers::offer::Offer::issuer_signing_pubkey
+	/// [`Offer::paths`]: crate::offers::offer::Offer::paths


Slight improvement suggestion.

Suggested change

/// The public key corresponding to the key used to sign the invoice.

///

/// This will be:

/// - [`Offer::issuer_signing_pubkey`] if `Some`, otherwise

/// - the final blinded node id from a [`BlindedPath`] in [`Offer::paths`] if `None`.

///

/// [`Offer::issuer_signing_pubkey`]: crate::offers::offer::Offer::issuer_signing_pubkey

/// [`Offer::paths`]: crate::offers::offer::Offer::paths

/// The public key corresponding to the key used to sign the invoice.

///

/// - If the invoice was created in response to an [`Offer`]:

/// - The public key will be [`Offer::issuer_signing_pubkey`] if it is `Some`.

/// - If [`Offer::issuer_signing_pubkey`] is `None`, the key will be the final

/// blinded node ID from a [`BlindedPath`] in [`Offer::paths`].

///

/// [`Offer`]: crate::offers::offer::Offer

/// [`Offer::issuer_signing_pubkey`]: crate::offers::offer::Offer::issuer_signing_pubkey

/// [`Offer::paths`]: crate::offers::offer::Offer::paths

This is always in response to an offer, so no need to have a leading statement here. Re-worded slightly for clarity.

shaavan · 2024-08-29T13:15:21Z

lightning/src/offers/invoice.rs

@@ -762,13 +762,37 @@ macro_rules! invoice_accessors { ($self: ident, $contents: expr) => {
 	}
 } }

+
+macro_rules! invoice_accessors_signing_pubkey {


We have separated the macros for invoice, and static invoice. However, the macro name, structure, docs, and variable names are identical. Maybe we can update their docs the have information specific to each type?

These macros are private to the module, so I think it is fine to keep them as is.

TheBlueMatt · 2024-09-05T19:23:50Z

Needs rebase :/

jkczyz · 2024-09-05T23:29:33Z

Needs rebase :/

Rebased 😮‍💨

TheBlueMatt · 2024-09-09T18:10:41Z

LGTM, basically, feel free to squash. I do find the issuer terminology to be super confusing, though - after this PR I can do InvoiceRequest.issuer_signing_pubkey() and get back a pubkey for the Offer, not the issuer of the InvoiceRequest as the method seems to imply (and before this PR there's still InvoiceRequest.issuer() itself).

jkczyz · 2024-09-09T18:32:36Z

LGTM, basically, feel free to squash. I do find the issuer terminology to be super confusing, though - after this PR I can do InvoiceRequest.issuer_signing_pubkey() and get back a pubkey for the Offer, not the issuer of the InvoiceRequest as the method seems to imply (and before this PR there's still InvoiceRequest.issuer() itself).

We could introduce an OfferRef<'a> type (wrapping a reference to the InvoiceRequest, actually) and have those methods on it instead. Then we can still define them with the macro. Caller would do something like invoice_request.offer().signing_pubkey(). Would such a type be a problem for bindings?

TheBlueMatt · 2024-09-09T18:42:04Z

Why not just use method names that make sense in both contexts? That seems like a bunch of effort to avoid picking a more descriptive name :).

jkczyz · 2024-09-09T21:50:56Z

Why not just use method names that make sense in both contexts? That seems like a bunch of effort to avoid picking a more descriptive name :).

Some reason given in #3218 (comment). It's also inconsistent with other offer field names. Could use an offer_ and invreq_ prefixes everywhere, but arguably having a different code separator between a logical "struct" and "field" is more readable than smashing everything together with underscores. The former is also more difficult to do with macros without leaving the separator on where it isn't need (e.g., Offer::offer_issuer_id).

TheBlueMatt · 2024-09-09T23:56:54Z

Some reason given in #3218 (comment).

I agree with the comments there, but I don't think it specifically argues for "issuer".

It's also inconsistent with other offer field names. Could use an offer_ and invreq_ prefixes everywhere, but arguably having a different code separator between a logical "struct" and "field" is more readable than smashing everything together with underscores. The former is also more difficult to do with macros without leaving the separator on where it isn't need (e.g., Offer::offer_issuer_id).

I agree with this, but to be clear my main complaint isn't that we need Offer-issuer, but rather that issuer is ambiguous in a general sense. Using "payer" for the entity paying makes sense, but "issuer" isn't the opposite of that (I know we probably don't want to use "payee" cause that confuses things for Refunds), but there should be something better, IMO.

TheBlueMatt · 2024-09-09T23:57:41Z

To be clear, we also don't necessarily need to figure it out now - we already use "issuer" to describe the Offer creator, so using it again is fine, I just think we should stop using it :)

valentinewallace · 2024-09-11T17:19:43Z

2c -- I agree that InvoiceRequest::issuer_signing_pubkey is a bit confusing. Tbh, I don't see an issue with using payee or recipient terminology, if we want to stick to just renames and not go for Jeff's invoice_request.offer().offer_field() solution. But I don't think it's too critical to figure out right now so fine to ACK to unblock the follow-ups if Matt is on board.

lightning/src/offers/offer.rs

jkczyz · 2024-09-11T20:26:54Z

2c -- I agree that InvoiceRequest::issuer_signing_pubkey is a bit confusing. Tbh, I don't see an issue with using payee or recipient terminology, if we want to stick to just renames and not go for Jeff's invoice_request.offer().offer_field() solution. But I don't think it's too critical to figure out right now so fine to ACK to unblock the follow-ups if Matt is on board.

FWIW, the offer issuer may not be the payment recipient.

valentinewallace · 2024-09-11T20:28:00Z

2c -- I agree that InvoiceRequest::issuer_signing_pubkey is a bit confusing. Tbh, I don't see an issue with using payee or recipient terminology, if we want to stick to just renames and not go for Jeff's invoice_request.offer().offer_field() solution. But I don't think it's too critical to figure out right now so fine to ACK to unblock the follow-ups if Matt is on board.

FWIW, the offer issuer may not be the payment recipient.

Ah, okay. Is this the payment notifications case?

jkczyz · 2024-09-11T20:33:58Z

Ah, okay. Is this the payment notifications case?

Yup, that's what I had in mind.

TheBlueMatt · 2024-09-16T15:04:53Z

Needs rebase.

lightning/src/offers/offer.rs

lightning/src/offers/invoice.rs

The spec was recently changed to use offer_issuer_id instead of offer_node_id. LDK always used signing_pubkey to avoid confusion with a node_id. Rename it to issuer_signing_pubkey now as InvoiceRequest and Bolt12Invoice will have similarly named methods in upcoming commits.

Useful for determining if the signing_pubkey is the issuer_signing_pubkey or is from a blinded path.

For consistency with Offer::issuer_signing_pubkey, rename InvoiceRequest::payer_id to use "signing_pubkey" instead of "id".

For consistency with Offer::issuer_signing_pubkey, rename Refund::payer_id to use "signing_pubkey" instead of "id".

vincenzopalazzo reviewed Aug 2, 2024

View reviewed changes

jkczyz force-pushed the 2024-07-offers-spec-update branch from b82f893 to 0695031 Compare August 2, 2024 14:18

jkczyz force-pushed the 2024-07-offers-spec-update branch from 0695031 to 05d2edb Compare August 5, 2024 23:56

valentinewallace reviewed Aug 7, 2024

View reviewed changes

lightning/src/offers/invoice_macros.rs Outdated Show resolved Hide resolved

jkczyz force-pushed the 2024-07-offers-spec-update branch from 05d2edb to 500af8a Compare August 8, 2024 17:38

jkczyz mentioned this pull request Aug 12, 2024

Experimental offer TLVs #3237

Merged

TheBlueMatt reviewed Aug 13, 2024

View reviewed changes

jkczyz force-pushed the 2024-07-offers-spec-update branch 2 times, most recently from ef5aff8 to eb43a3c Compare August 15, 2024 00:19

shaavan reviewed Aug 29, 2024

View reviewed changes

jkczyz force-pushed the 2024-07-offers-spec-update branch from eb43a3c to a4f0d8f Compare September 5, 2024 23:28

jkczyz force-pushed the 2024-07-offers-spec-update branch from a4f0d8f to 7da88f0 Compare September 9, 2024 16:53

jkczyz force-pushed the 2024-07-offers-spec-update branch from 7da88f0 to 9ce3187 Compare September 11, 2024 17:00

valentinewallace reviewed Sep 11, 2024

View reviewed changes

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved

jkczyz force-pushed the 2024-07-offers-spec-update branch from 9ce3187 to ce74ceb Compare September 11, 2024 20:24

jkczyz force-pushed the 2024-07-offers-spec-update branch from ce74ceb to b670d90 Compare September 16, 2024 15:52

TheBlueMatt previously approved these changes Sep 16, 2024

View reviewed changes

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved

lightning/src/offers/invoice.rs Outdated Show resolved Hide resolved

jkczyz added 7 commits September 16, 2024 13:56

Re-write Offer::issuer_signing_pubkey documentation

7351270

Add issuer_signing_pubkey to Bolt12Invoice

a466f2e

Useful for determining if the signing_pubkey is the issuer_signing_pubkey or is from a blinded path.

Use unsigned invoice types in docs

cfe8430

Update offer test vectors from latest spec changes

de29295

Rename InvoiceRequest::payer_id

dd8f744

For consistency with Offer::issuer_signing_pubkey, rename InvoiceRequest::payer_id to use "signing_pubkey" instead of "id".

Rename Refund::payer_id

e11025f

For consistency with Offer::issuer_signing_pubkey, rename Refund::payer_id to use "signing_pubkey" instead of "id".

jkczyz dismissed TheBlueMatt’s stale review via e11025f September 16, 2024 18:56

jkczyz force-pushed the 2024-07-offers-spec-update branch from b670d90 to e11025f Compare September 16, 2024 18:56

TheBlueMatt approved these changes Sep 16, 2024

View reviewed changes

valentinewallace approved these changes Sep 17, 2024

View reviewed changes

TheBlueMatt merged commit 429cbe1 into lightningdevkit:main Sep 17, 2024
18 of 21 checks passed

		let builder = OfferBuilder::deriving_signing_pubkey(node_id, expanded_key, nonce, secp_ctx)
		let builder = OfferBuilder::deriving_issuer_id(node_id, expanded_key, nonce, secp_ctx)

Rename Offer::signing_pubkey to Offer::issuer_id #3218

Rename Offer::signing_pubkey to Offer::issuer_id #3218

Conversation

jkczyz commented Aug 1, 2024

codecov bot commented Aug 1, 2024 • edited Loading

Codecov Report

valentinewallace commented Aug 1, 2024

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

TheBlueMatt commented Aug 13, 2024

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

TheBlueMatt Aug 13, 2024 • edited Loading

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

shaavan 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Sep 5, 2024

jkczyz commented Sep 5, 2024

TheBlueMatt commented Sep 9, 2024

jkczyz commented Sep 9, 2024

TheBlueMatt commented Sep 9, 2024

jkczyz commented Sep 9, 2024

TheBlueMatt commented Sep 9, 2024

TheBlueMatt commented Sep 9, 2024

valentinewallace commented Sep 11, 2024

jkczyz commented Sep 11, 2024

valentinewallace commented Sep 11, 2024

jkczyz commented Sep 11, 2024

TheBlueMatt commented Sep 16, 2024

Rename `Offer::signing_pubkey` to `Offer::issuer_id` #3218

Rename `Offer::signing_pubkey` to `Offer::issuer_id` #3218

codecov bot commented Aug 1, 2024 •

edited

Loading

TheBlueMatt Aug 13, 2024 •

edited

Loading