lightning icon indicating copy to clipboard operation
lightning copied to clipboard

Changed: offers: offers that include offer_paths now omit the offer_issuer_id and sign with the last used blinded path alias' key by default

Open 21M4TW opened this issue 8 months ago • 10 comments

  • Removing the offer_issuer_id from created offers when there's an offer_path, as explained in the Bolt12 specifications. This saves space and enhance privacy when it's not needed because we already have a blinded path.
  • Allowing signing an invoice referencing the above offer by signing with the last used blinded node id, as per the Bolt12 specifications.
  • Adding a force_issuer_id optional field to the offer command so the offer_issuer_id is included even when not required due the presence of blinded paths.
  • [ x] The changelog has been updated in the relevant commit(s) according to the guidelines.
  • [ x] Tests have been added or modified to reflect the changes.
  • [ x] Documentation has been reviewed and updated as needed.
  • [ x] Related issues have been listed and linked, including any that this PR closes.

21M4TW avatar Apr 15 '25 13:04 21M4TW

As requested by @vincenzopalazzo

21M4TW avatar Apr 15 '25 13:04 21M4TW

This invoice is invalid: let's break it down by type, length, value (hex)

  • 00 10 fc84667539e41bd6564fbbdb87d3375e
  • 02 20 43497fd7f826957108f4a30fd9cec3aeba79972084e90ead01ea330900000000
  • 0a 07 6e657774657374
  • 10 ee 03e5c9e190e93af5f757e533ad12ca2da9daef99f337a7af0320e70feae9a7890003b224fa465507bf714641c64a1018ccc05bbcda60d3aa15212e577dfd8e0fb9ea020202a9628f99168a0b09c8ddc71bd9191e31f871075cd8854968dbb72d47c3912100338139ad02276007ea918fa7208765a306c200c7af814c0284deca7ced13b36863c84d03393572ef44d44d78cce9b0bf58a6ff8c02c1a165fe61b4fc6c2097bee34d538fbc84902df62e0bf8eac81495971d491b3600324a46184f1c237f594b5cd4797f794c0b3f2267b8ff51e55376aa7053a5b708ad7c7056dd0b537494580af8f872b9972cebba
  • 50 20 43497fd7f826957108f4a30fd9cec3aeba79972084e90ead01ea330900000000
  • 52 01 0b
  • 54 00
  • 58 21 037d942d1f385d68bf600ed3ce14d1e0b84f99eaa594196e60dedc4a6eae3064fe
  • a0 fd00fd 03e5c9e190e93af5f757e533ad12ca2da9daef99f337a7af0320e70feae9a789000304e360cbffb0e81e6489baad6b24762b28c1f98bec4fcd7771db32906baababd02038e48152e6f2388282153b34bd136a812b71648f3c15c894d1c50afc45ac154cd0042f6ace0f6e49219b7dbca6f38e0f53a2f70b06403692dfa04d6ba0734fb2084b1c4cf1d7d42567c59715cdbef764fe63440ddb41e7fe67e34be6bdccac4a59f7d596002d7759d4d3b3d3a2fe68bffbbcad4b68456f60fe8c0877c92990e7ed402939cc000323fd7cf52b11be81729bcf3a0dcf3b9aae68351b3f1bd40c29db363573aebefa38d44988c0929ae3c1ad3052b48d38327ab6b
  • a2 1c 000000010000000a0010000000000000000000000000002380000000
  • a4 04 68ac799e
  • a8 20 d6e635a2de2108f755ac860e2a6e80ab9798dc73ab55e6b6c54eda1025e6dadf
  • aa 01 0b
  • ae 03 020000
  • f0 40 9558b15edc9a8b402eb1a54512caae3b82d31f61c04e849a5491d075d5d0ebf7b3f0ae39e8541496ec374a38ec8ea947de2a74ba4b22efb34a859e55506726cc

This has no "b0" field (i.e. type: 176 (invoice_node_id)).

To quote bolt12:

A writer of an invoice:
...
  - if `offer_issuer_id` is present:
    - MUST set `invoice_node_id` to the `offer_issuer_id`
  - otherwise, if `offer_paths` is present:
    - MUST set `invoice_node_id` to the final `blinded_node_id` on the path it received the invoice request

And:

A reader of an invoice:
  - MUST reject the invoice if `invoice_amount` is not present.
  - MUST reject the invoice if `invoice_created_at` is not present.
  - MUST reject the invoice if `invoice_payment_hash` is not present.
  - MUST reject the invoice if `invoice_node_id` is not present.

So, who is producing this invalid invoice?

rustyrussell avatar Sep 16 '25 01:09 rustyrussell

Thank you Rusty, and sorry for the late reply.

I just updated this PR with what I think fixes the issue. It now does the following: -When offer_paths are present and offer_issuer_id is no longer needed, set offer_issuer_id to NULL in found_best_peer -In listoffers_done, when there is no offer_issuer_id, set invoice_node_id to the final blinded_node_id from the used offer path (as described in the Bolt12 specs) -handle_sign_bolt12 was only used to sign invoices, but was not able to sign using the blinded node id. I modified the function to support using a path_pubkey (I created the function node_blinded_privkey as well).

So as far as I know, this code is now properly working and addresses the issues I had identified. The invoice signature is recognized as valid by fetchinvoice, and it works whether or not an offer contains an issuer_id. Thank you!

21M4TW avatar Nov 11 '25 03:11 21M4TW

This solves the issuer_id issue identified in https://github.com/ElementsProject/lightning/issues/4689

21M4TW avatar Nov 11 '25 23:11 21M4TW

It seems this one slipped through the 25.12 release crack because the issue it was linked to had been closed. will request and prompt for review ahead of early 2026 release. @21M4TW can you please rebase?

madelinevibes avatar Dec 08 '25 05:12 madelinevibes

It seems this one slipped through the 25.12 release crack because the issue it was linked to had been closed. will request and prompt for review ahead of early 2026 release. @21M4TW can you please rebase?

Thanks. I just rebased it to the latest commit from master.

21M4TW avatar Dec 08 '25 13:12 21M4TW

thanks! now over to @endothermicdev for review please.

madelinevibes avatar Dec 08 '25 23:12 madelinevibes

I'm just catching up with this PR, but the commit message is unclear. I'm not convinced anything is currently broken - if so, please describe the issue and preferably reproduce it with a test so we can make sure to check for it in the future.

I think this is doing two things:

* Removing the `offer_issuer_id` from created offers when there's an `offer_path`.  I presume this is to save space and/or enhance privacy when it's not needed because we already have a blinded path.

* Allowing signing an invoice referencing the above offer by signing with the blinded node id.

In that case, it would be nice to document the rationale for the change in the commit, and the changelog should be changed, because it changes behavior rather than fixing a broken feature. The commit message would then look something like:

offers: omit `offer_issuer_id` from new offers when using blinded `offer_path`s

describe the rationale and changes...

Changelog-Changed: offers: offers that include offer_paths now omit the offer_issuer_id and sign with the blinded path alias' key

This is a nice improvement! The only other thing is I wonder if it's always correct to omit offer_issuer_id if we can. Would a vendor want to force inclusion here so that customers can verify they're dealing with the right node? In that case, an optional parameter to the offer rpc to force inclusion of the offer_issuer_id might be appropriate.

I agree with you that it was not "broken" in the sense that it was not preventing a payment, but the offer from a node with only private channels contained the unannounced node ID of that node. I can tag it as "changed". I will look into adding an optional parameter to the offer rpc. Thanks!

21M4TW avatar Dec 14 '25 15:12 21M4TW

@endothermicdev I added an optional force_issuer_id field to the offer rpc as you suggested (hopefully I changed all the files I had to change regarding this).

21M4TW avatar Dec 15 '25 04:12 21M4TW

I still need to review the core part of this PR because it requires me to look at the code that I haven't been looking at for a long time, but I left some comments during my light review.

BTW review the commit is hard because you are putting all together, unfortunately, and probably splitting this in the right way is hard too, because it's like a chicken and eggs problem, but I would love to see the gen file stripped out and add them in a separate commit.

What I would do if I were writing the commit from 0 is:

1. Core functionality

2. auto gen files

3. tests

But if the CLN team is fine with the current organisation, I am fine too!

If I was putting the new force_issuer_id part in a separate commit it would separate the core functionalities quite well I think. Do you want me to do that?

For the core functionalities, it is mostly divided in two, that is the more trivial part where I simply set the offer_issuer_id to NULL when offer_paths is present (and the issuer ID is not enforced through force_issuer_id), and the part where I pass down the last blinded_node_id and the associated path_pubkey starting at the onion message level, and where I use these to tweak the private key and sign the invoice.

21M4TW avatar Dec 15 '25 14:12 21M4TW