RoboSats / robosats

A simple and private bitcoin exchange
https://learn.robosats.com
GNU Affero General Public License v3.0
754 stars 146 forks source link

Add 'stealth invoices': an On/Off toggle for lightning invoice description #168

Closed alaznem closed 2 years ago

alaznem commented 2 years ago

the privacy issue When paying lightning invoices for the bond or the maker trade amount, the super descriptive description in the lightning invoice is handy, BUT there are a lot of use cases where users doxing itself severely.

As an example, if an user pays the bond or trade amount from a custodian exchange (Kraken for example), Kraken is getting undeniable facts about the withdrawal. This is also true for all other custodial lightning wallets like Wallet of Satoshi, ...

This is probably true for invoices as a taker, but I didn't research into this side of the trade concerning this feature request so far.

proposed solution Make adding a description to lightning invoices toggable for each user and default to NO description.

alternatives I can't think of any alternatives to maintain privacy when interacting with robosats from custodial wallets.

Reckless-Satoshi commented 2 years ago

Hey @alaznem , thanks for bringing up this issue.

Indeed, in some scenarios (custodial wallets) this is a big privacy concern. Let's brainstorm for a solution.

It is unavoidable that they know you are interacting with RoboSats's node (since the invoice must encode node_id), however the invoice usually also encodes order_id, currency and amount. I like the suggested toggling on/off of the description, however that means the invoice has to be generated more than once by the backend if the user toggles on/off: not too elegant since it exacerbates our problem with garbage invoices.

I am also unsure whether the default should be off: the information encoded in the description is often critical. Sometimes in case of catastrophic loss and need of support, many users only can recover the information about their order from the description of these invoices. It also explains how a hodl invoice behaves.

Currently a maker bond reads:

RoboSats - Publishing {order_id - buy/sell - order_currency - order_amount} - Maker bond - This payment WILL FREEZE IN YOUR WALLET, check on the website if it was successful. It will automatically return unless you cheat or cancel unilaterally.

Probably an invoice that minimizes new information gained by the custodial wallet (given that they already know you are dealing with RoboSats) could read:

You are committing to Order {order_id}. This payment WILL FREEZE IN YOUR WALLET, check on the website if it was successful. It will automatically return unless you cheat or cancel unilaterally.

It does not tell the custodial wallet anything about buy/sell, currency or amount. Yet still gives a few critical hints of info to the user.

Ideas?

alaznem commented 2 years ago

Thank you for taking my privacy concern serious, @Reckless-Satoshi .

I like the suggested toggling on/off of the description, however that means the invoice has to be generated more than once by the backend if the user toggles on/off: not too elegant since it exacerbates our problem with garbage invoices.

I can think of two options:

  1. I lack the understanding of lightning invoice creation, because this option might not be possible without invalidating the signature of the lightning invoice: can the lightning invoice be generated with the description field populated and the client alters the invoice locally in the browser when toggling? I'd expect when pressing the toggle button to off, the description field will be populated with blank/null and the qr code and invoice string updates accordingly.
  2. Make the user decide on the start of the order making or taking workflow to decide if the user want the invoices to be in stealth/ private/ incognito/ non descriptive mode or with the full information in the invoice description.

I am also unsure whether the default should be off: the information encoded in the description is often critical. Sometimes in case of catastrophic loss and need of support, many users only can recover the information about their order from the description of these invoices. It also explains how a hodl invoice behaves.

Thanks for the context. I change my mind and agree on defaulting to description on.

Probably an invoice that minimizes new information gained by the custodial wallet (given that they already know you are dealing with RoboSats) could read:

You are committing to Order {order_id}. This payment WILL FREEZE IN YOUR WALLET, check on the website if it was successful. It will automatically return unless you cheat or cancel unilaterally.

It does not tell the custodial wallet anything about buy/sell, currency or amount. Yet still gives a few critical hints of info to the user.

My preference is to implement toggling and keep the super descriptive mode as default and not stripe away some information from the invoice description. But display proper explanation of the toggling choice to the user during the workflow.

As long as the order_id is in the description, taking away information is just obscurification in times when robosats has become a big fish in p2p. The robosats order book is public and the custodian can observe the orderbook and make educated guesses about the type of lightning invoice because of the robosats order lifecycle.

On the other hand to switch to this not so descriptive invoice text could make it impossible for custodians to mine their data for historic robosats invoices, when they start to track interactions with robosats.

However if we could remove the order_id as well, and replace it with an order_reference field which is a non public field, then I feel comfortable without toggling and altering invoice description globally. This way the custodian could not connect the lightning invoice with an order (and in the worst case recurring robot identity), but in case of catastrophic loss, robosats support could trace the order_reference back to the actual order_id.

Reckless-Satoshi commented 2 years ago
  1. can the lightning invoice be generated with the description field populated and the client alters the invoice locally in the browser when toggling?

Actually, I believe this is technically possible! We would need a Javascript Bolt 11 invoice decoder and encoder. I've quickly check online and there seems to be a perfect package for it. So it might be relatively easy to implement a client-side stealth invoice.

It is definitely the most elegant solution, so hoping it works out well :)

Reckless-Satoshi commented 2 years ago

Implementing "Stealth invoices" (On/Off switch for invoice descriptions) is rewarded with ⚡ 120,000 Sats ⚡ (80K from dev fund and 40K by @alaznem)

These are just some route hints of how this task could look like. Please be mindful it might not be the right way to solve it:

Risks: it is unclear to me whether a bolt11 invoice can be stripped out some data and re-encoded and still be functional.

Reply to this comment if you want to be assigned this task.

fmitjans commented 2 years ago

Hi, I would like to be assigned this task

Reckless-Satoshi commented 2 years ago

Hi, I would like to be assigned this task

Sorry for the delay. Have fun! Let us know if you find any road block :)

Reckless-Satoshi commented 2 years ago

Reward for this feature has been bumped to 120,000 Sats via donation of @alaznem . Thank you! https://twitter.com/alazmonerales/status/1552099491985203208

fmitjans commented 2 years ago

Apparently the package doesn't allow to encode a different invoice without a new signature (it requires a private key). Further, looking at Bolt#11 it seems that any change to the description of an invoice would require a new signature, as the description is part of the data to be signed. Taking this into account, what would be the best way to implement this feature? Is generating two invoices for every transaction a big problem?

Reckless-Satoshi commented 2 years ago

Apparently the package doesn't allow to encode a different invoice Thanks for work. Very unfortunate finding since this was the absolute preferred way for us to handle the removal of the description. Is generating two invoices for every transaction a big problem?

It is certainly very inconvenient and risks the scalability of the app.

For further context: Every order has 4 LNpayment foreignkeys, three of them are hodl invoices. Each has a different functionality: maker bond, taker bond and trade escrow. Once one of these objects is created, it has the status "Generated". While this is the status, a backend service will be asking LND constantly whether the invoice has been locked or not. Once the invoice is locked, the order advances to the next status (e.g. maker bond is locked -> order goes public). Creating 2 invoices for each of these hodl payment increases the complexity of RoboSats substantially (e.g., backend has to attend two invoices simultaneously, each order has 4->7 LNpayments). It will make the DB grow almost twice as fast (it is already a problem at current rate!). In addition, it requires rewriting a lot of logics that are sensible and we risk introducing bugs on parts of the app that are already very battle tested. All in all: a terrible idea.

Another possible approach... A robot can set under their profile if they want Stealth Invoices. How the implementation would look:

For dispute solving and debugging, it would be best if the generated stealth invoice does actually have a unique description, but it can be a random string of characters "h2837ry98hwe9".

Cons: none really, but this is now 75% a python/backend task. Pros: no risks, this approach is guaranteed to work.

What do you guys think about it?

fmitjans commented 2 years ago

It will be harder to implement, but I think it is a great long-term solution.

Would the whole description look like this?

Transaction h2837ry98hwe9. This payment WILL FREEZE IN YOUR WALLET, check on the website if it was successful. It will automatically return unless you cheat or cancel unilaterally.

Or just the random string?

Reckless-Satoshi commented 2 years ago

Transaction h2837ry98hwe9. This payment WILL FREEZE IN YOUR WALLET, check on the website if it was successful. It will automatically return unless you cheat or cancel unilaterally.

This looks good to me as there is not sensitive data but helps the user with an explanation. Does it look good to you @alaznem ?

alaznem commented 2 years ago

It will be harder to implement, but I think it is a great long-term solution.

Is there a need to bump the reward even more? And do you still want to keep being assigned to this task?

Transaction h2837ry98hwe9. This payment WILL FREEZE IN YOUR WALLET, check on the website if it was successful. It will automatically return unless you cheat or cancel unilaterally.

Or just the random string?

I like the idea of using a word which is not connected to the idea of an "order" to describe the purpose of the random string, to indicate to the user that this random string doesn't show up anywhere else in the UI. Maybe we could be even more explicit about this and put it to the end of the description and don't introduce a new word other than payment like this:

This payment WILL FREEZE IN YOUR WALLET, check on the website if it was successful. It will automatically return unless you cheat or cancel unilaterally. Payment reference: h2837ry98hwe9

As an alternative we could put the reference inline like this:

This payment h2837ry98hwe9 WILL FREEZE IN YOUR WALLET, check on the website if it was successful. It will automatically return unless you cheat or cancel unilaterally.

I'd go for the version of putting the payment reference to the end for readability and accept to add another sentence to the description. But maybe I oversee other important tradeoffs here?

Reckless-Satoshi commented 2 years ago

This payment WILL FREEZE IN YOUR WALLET, check on the website if it was successful. It will automatically return unless you cheat or cancel unilaterally. Payment reference: h2837ry98hwe9

+1

fmitjans commented 2 years ago

Is there a need to bump the reward even more? And do you still want to keep being assigned to this task?

I'll be happy to do the assignment for the current reward, but it might take a while to complete (maybe 2-4 weeks). Is time an issue?

Reckless-Satoshi commented 2 years ago

I'll be happy to do the assignment for the current reward, but it might take a while to complete (maybe 2-4 weeks). Is time an issue?

I would deem it a low priority task (... in the context of everything that RoboSats needs as of now :D). No problem if it takes some time IMO.

I am also up for implementing all the backend stuff. I think it might be a large overhead for you to run a development full stack. However, I might not find time to implement this backend feature in the next ~2-3 weeks.

fmitjans commented 2 years ago

Unfortunately it seems I'm out of my depth doing this task. I no longer want the assignment

Reckless-Satoshi commented 2 years ago

I no longer want the assignment

I see, it became a different kind of task since invoices cannot be modified after the fact.

I am up for implementing the API endpoint during next week, if you want to take the part of adding the frontend switch for Stealth Invoice would be awesome.

Otherwise, please allow me to tip you 40K Sats, you did valuable research figuring out the original plan was not viable.

PD: This one might also be fitting for you ? https://github.com/Reckless-Satoshi/robosats/issues/195

fmitjans commented 2 years ago

I'll look into helping with the switch or other tasks, but I'm not sure I will get results so I'd rather not have any assignments yet. That's very kind of you. Is it ok to talk on Telegram about the payment?

alaznem commented 2 years ago

Unfortunately it seems I'm out of my depth doing this task. I no longer want the assignment

@fmitjans, thank you for being so responsive!

if you want to take the part of adding the frontend switch for Stealth Invoice would be awesome.

@Reckless-Satoshi : do you think it is worth it to sum up this issue for potential new assignees to avoid TL;DR? I feel that this issue has grown over a comfortable quick read and that the result is not really worth a full read (, but the path and space we took was necessary to come up with a good solution for sure). I could sum up before you start doing the backend part, if you think it's worth the time. I would also need some guidance on how to get this done, since I never did such sum up thing in a github issue. Probably by editing the initial post and maybe change the title to point to a TL;DR which shows recurring developers that the state of this issue has changed from heavy discussion to coding? Or should I do something with a PR tied to this issue?

Reckless-Satoshi commented 2 years ago

Is it ok to talk on Telegram about the payment?

Yes, just write a private. Although, as always, it would be great if we have a way to make these rewards to developers transparently in the open (always minding not revealing a sensitive node pubkey).

Probably by editing the initial post and maybe change the title to point to a TL;DR

This would be great!

To be honest, I could simply get this task done fairly quick and save a lot of future back and forth, but then we fail with the mission of the rewarded tasks: to incentive more developers to play with the insides of RoboSats and bring different perspectives.

ShatteredBunny commented 2 years ago

Hi, I'd like to do the task