-
Notifications
You must be signed in to change notification settings - Fork 26
Adding support for compact offers #840
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
base: master
Are you sure you want to change the base?
Conversation
The default offer also uses an empty `path_id` in the blinded path (because it doesn't need to store any randomness), so an empty `path_id` doesn't necessarily means that we're using a compact offer.
|
I've fixed the This looks ok to me, we could bikeshed what the wallet exactly stores (storing the nonce instead of the blinded public key would be slightly more consistent with deterministic offers), but we probably don't care. @thomash-acinq what do you think about this change? |
| * This offer will stay valid after restoring the seed on a different device: we will | ||
| * automatically keep accepting payments for this offer. |
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.
This doesn't seem compatible with the requirement to store the key in compactOfferKeys which will not be restored from the seed.
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.
Good catch. That was not accurate. Fixed in 1226ecc
| * The caller must store the returned `OfferAndKey.privateKey.publicKey`, | ||
| * and set/update the NodeParams.compactOfferKeys with the value. |
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.
Why does the caller need to do it, it seems that the function already does it.
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.
Good point. Lightning-kmp does automatically update the value. But those values aren't persisted to disk. So the next time the app restarts, it's the callers responsibility to restore the set. I've updated the text to be more specific in 1226ecc
|
A question to you @robbiehanson: how many such compact offers do you expect to create? If it's a small amount, we could derive them all deterministically based on a counter between |
Good question. Basically, a new offer would get created for each linked card. In theory this means only a handful of cards. In practice... users lose cards. And if the cards only cost a few dollars, and you can choose from hundreds of thousands of designs (holiday designs, your favorite sports teams, ...) people might buy several because it's fun (and their non-fun bank never let them do it). There was also a discussion about merchant tracking. In the lnurl version, every card using the same HTTP sever had the same URL. This had the nice property that it provided an anonymity set, and prevented any kind of useful merchant tracking. With offers, since each offer is unique, it opens the door to merchant tracking. And it was pointed out that this problem could be solved with blinded paths. That is, you can simply update the card with a newly generated offer (and new random nonce). You don't even have to change the keys on the card. So from the app's perspective it's the same card (same keys, same card UID). But from the merchant's perspective, it now looks like a completely different card. This is an advanced feature... but would be nice to have the support in lightning-kmp should we decide to include it. Which means, possibly, multiple offers per card.
This is an interesting idea. Say a user loses a card, and they mark it as lost within the app. I suppose we would want to stop accepting InvoiceRequest's for this offer? Maybe not right away, but maybe after 90 days? (Since InvoiceRequest from card offer == merchant refund attempt) Or maybe we don't worry about this?
Right now, each card gets backed up. So if you lose your phone, for example, and restore your phoenix wallet, then your cards are restored as well. Using the current code, we'd have to also backup the compactOfferPubKey in this dataset. With this new idea, I would just need to backup & restore the |
But here, the backup is a cloud backup, which users can disable and which could be lost (if access to the cloud account is lost), this is a weaker backup than just restoring from seed (which is sufficient for other types of offers).
It looks like it's desirable to be able to generate new compact offers whenever wanted: I think my proposal of brute-forcing a small number of compact offers based on a counter doesn't work really well then. And being able to revoke an offer (in case the card is lost) is IMO an important feature, which is hard to handle with a counter-based set of offers. I'm thinking of a slightly different way to backup/revoke these compact offers then. What do you think of the following:
I think the ability to use the cards themselves as a backup would be really nice, let me know if that's feasible (for example by making sure the "randomly generated card keys" you mention are actually deterministically derived based on the seed and something that's hard-wired into the card or the offer). |
thomash-acinq
left a comment
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've made some changes in #841 so that compact routes are only accepted when we need it.
| // Compact offers are randomly generated, but they don't store the nonce in the path_id to save space. | ||
| // It is instead the wallet's responsibility to store the corresponding blinded public key(s). | ||
| pathId == null && nodeParams.compactOfferKeys.value.contains(blindedPrivateKey.publicKey()) -> 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.
"compact offers" are not real offers in the sense that we won't accept invoice requests for them, so they don't need to be handled here.
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.
Actually they are real offers, we do want to accept payments for them (for example when merchant does a refund or cashback). It's also handy to use that card simply to provide our offer that anyone can pay, so I think it's worth handling them here?
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, then we should at least check that it has only a blinded route and no other field (no description that we would commit to).
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.
By tapping the card, we give permission to the other party to collect a (small) payment. It seems weird and unsafe to reuse this interaction to also mean "pay me".
| } catch (_: Throwable) { | ||
| Either.Left(CannotDecodeTlv(OnionPaymentPayloadTlv.EncryptedRecipientData.tag)) | ||
| val nextPathKey = Sphinx.blind(pathKey, Sphinx.computeBlindingFactor(pathKey, sharedSecret)) | ||
| if (encryptedPayload.isEmpty()) { |
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.
The compact format should still be disallowed for payments and for standard offers. We should enable it explicitly only when we need it.
That's totally feasible. And a great idea. When creating/linking the card, the steps would be:
Restoring the card via a tap would simply require:
|
|
Nice, that sounds like a good plan to ensure that those offers can be restored from the cards. In that case we only need Phoenix to store the set of @robbiehanson can you take a look at #841 which introduces the |
Yeah, we had a conversation about it, and we're going to bump that to a future PR. The specs for the CardPaymentRequest are still a work-in-progress. |
In the Bolt Card community, there is a new proposed protocol that uses Bolt 12 offers.
One of the problems with the current bolt card protocol is that is uses lnurl-withdraw (which creates various problems, especially for self-custodial wallets). So there's a push to switch to offers & onion messages (i.e. direct communication over the lightning network, and no need for an HTTP server).
Background
A short summary of how the new protocol works is like this:
CardPaymentRequestmessage to Bob that includes:CardPaymentErrormessage back to AliceThis new protocol will be a dramatic improvement over the old protocol for a number of reasons:
The Problem
There's one hiccup: the physical card only contains 256 bytes of memory. Plus there are several reserved bytes for the NDEF message header and the dynamically generated values. So the actual max size of an offer is 200 bytes. And an offer can be larger than this, especially when a blinded path is included.
Our default offer (on mainnet) is currently 206 bytes.
In our default offer, we have the following encryptedData sizes:
encryptedData for trampoline node = 51 bytes
encryptedData for final recipient = 16 bytes
I was very curious as to why we bother to include any encryptedData for the final recipient, when it's actually an empty TLV. And I found the following rationale in Bolt 4:
Challenging this requirement: For an incoming payment for our default offer, and for a CardPaymentRequest, we have no incentive to "verify that the blinded route is used in the right context".
In the case of our defaultOffer, there's nothing to verify about the payment because there's no amount or description or anything. And I don't think we care, since the assumption about our defaultOffer is that it's "shared widely".
In the case of a CardPaymentRequest, the verification is actually separate from the blinded path. That is, the CardPaymentRequest contains the encryptedData directly from the card, and it's really this data we need to verify.
If the encryptedData for the final recipient was dropped, the size drops to 190 bytes, and the offer would fit on the card.
In discussing the issue internally, it was decided that:
and:
However, we really don't want to change the current default offer. Doing so could affect contacts. Luckily we don't need to.
Proposed Solution
This PR proposes a new function in NodeParams:
It will return an offer with null encryptedData for the finalRecipient. I.e. an offer of size 190 bytes.
The existing functions we're currently using (
NodeParams.defaultOffer&NodeParams.randomOffer) are unchanged.Technical Discussion
A user would be allowed to link multiple cards to their wallet. (e.g. create cards for your children and set spending limits on them) Ideally each card has a different offer. Otherwise merchants can associate them. For this reason, the
compactOfferuses a random nonce. And each generated compact offer is unique. (i.e given 2 offers, merchant wouldn't be able to tell if they were for 2 different wallets or the same wallet)There's a function
OfferManager.isOurOffer()which is used when receiving an InvoiceRequest. It continues to function as expected, even when presented with a compact offer.It now looks like this:
Which uses a new value in NodeParams:
Since
compactOfferis only designed for a narrow set of use cases, it's the caller's responsibility to store & restore the set of keys. In my use case, this will be simple because the user will only create a few cards.