Skip to content
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

Privacy improvements for manual payout #4899

Merged
merged 2 commits into from Dec 17, 2020
Merged

Privacy improvements for manual payout #4899

merged 2 commits into from Dec 17, 2020

Conversation

ghost
Copy link

@ghost ghost commented Dec 4, 2020

The goal of this PR is to improve privacy, by not requiring mediators to ask for users private keys.

  • Redesign the UI
  • Add import/export of payout settings
  • Mediator does not need private key
  • User can sign using own wallet or private key
  • Validation of input fields
  • Calculate the tx fee based on inputs
  • Display of the generated txid & hex so it can be checked

The emergency multisig payout tool has been redesigned so that the unsigned payout transaction can be built, exported to the users for them to sign, and then the two signatures applied by the mediator before broadcasting the payout.

The screen is split into tabs (or a menu) allowing choices for Inputs, Import/Export, Sign and Build. See screenshots below.

Inputs would be filled out by the mediator. Most of the info can be obtained from the trade's contract. The amountInMultisig can be obtained from checking an explorer - amountInMultisig is necessary because this value is part of the hashed signature (in segwit), so the TX will not be valid unless this amount is specified correctly.

update 2020-12-14 : added the option to import data from a mediation ticket (see last screenshot)

The mediator would then export the settings and paste the exported string to both users in mediation chat. e.g.

c2Vnd2l0OmE5MGJlY2Q3NGFlY2Y3ZGNhYzcyYzYyY2U3ZjRlZGMzNmNhYjQ0MTEwNmMxZWNhNDgzMDFlZjNiNGU1MmRhN2Y6MC4wMzIzMzM6MC4wMjY6MC4wMDY6YmNydDFxbXR0bjVwMnMwdnVjdnRwenMwd2oydXJtbW16bjhrMGRmMzBuZjU6YmNydDFxNTcwa2wzNGNzdndrbGt6MDQwNTk1ejg2MHRjd3lwemprdnB2czA6MDJiYTI5YmE0MWY2MDhmZWE4YWI3ZTQ2NzRkNDRjYWYxY2NiNjg1NDVjNzlmNjUxNGZiOTc4ZjNlZGVlZGZhZDQyOjAzMmU3M2RmOGQyYTEzNGYwZGY0MTcxZjU5ZjNkNDMxNjExNmZmMDAwYzViN2IyYzdiOGNiYWMyYWUzYjliODNjMw==

Users would open the tool (ctrl+g) click import and paste in the supplied string. Then they would click sign and press "LOCATE KEY IN WALLET" followed by "GENERATE SIGNATURE". Then they would paste the signature string back to the mediator.

(The mediator would give the user some guidance on how to open the tool and sign).

The mediator, upon receiving the two signature strings would click "BUILD" and paste the buyer and seller signatures into the fields, followed by "BUILD" and/or "BROADCAST". The txId and txHex are displayed in a text box (similar to the UI of coinbin), so the txId could by copied into an explorer to check the TX status.

Entering the parameters:
image

Exporting:
image

Signing (by users):
image

Building:
image

Update 2020-12-14 added ability to import data from mediation ticket:
image

Fixes #4061

@huey735, mediators are invited to review/comment on this proposed solution.

@Bisq-knight
Copy link

Sounds good! I'm gonna try it out with a case I currently have that requires that. So the steps are:

  1. Mediator inputs the payout details
  2. Mediator exports the unsigned tx
  3. Mediator sends both users the transaction to sign, which
  4. user opens the tool (also with ctrl+g)
  5. user signs
  6. user sends mediator the signature
  7. Mediaor combines the two signatures
  8. Mediator broadcasts the tx

@ghost
Copy link
Author

ghost commented Dec 7, 2020

Yes, those are the steps. I'm here & on keybase if you have any issues.

@Bisq-knight
Copy link

Like I said in our discussion in Keybase, this does look very promising and certainly would make users' lives easier as they do feel a like intimidated when they read the tutorial on how to get the private keys.

Plus it removes our reliance on Coinb.in as part of the current "private" process, a service that could be taken down at any time

@chimp1984
Copy link
Contributor

Thanks @jmacxx for your initiative!

Maybe we can combine that for bisq-network/proposals#287 ?

E.g. If we make it a bit more automated then the users could use that tool to make an alternative payout with arbitrary outputs and fees so it can serve both the idea to boost stuck tx chains by CPFP and to be used as emergency tool if normal payout options do not work. Of course adding too much complexity to an emergency tool is not a good idea...

Redesign the UI
Add import/export of payout settings
Add ability to import from mediation ticket
Mediator does not need private key
User can sign using own wallet or private key
Validation of input fields
Calculate the tx fee based on inputs
Display of the generated txid & hex so it can be checked
@ghost ghost marked this pull request as ready for review December 15, 2020 04:25
@ghost
Copy link
Author

ghost commented Dec 15, 2020

E.g. If we make it a bit more automated then the users could use that tool to make an alternative payout with arbitrary outputs and fees so it can serve both the idea to boost stuck tx chains by CPFP and to be used as emergency tool if normal payout options do not work. Of course adding too much complexity to an emergency tool is not a good idea...

@chimp1984 Its a good idea and I'd like to take some time to think about a UI design for it.

@chimp1984
Copy link
Contributor

chimp1984 commented Dec 15, 2020

E.g. If we make it a bit more automated then the users could use that tool to make an alternative payout with arbitrary outputs and fees so it can serve both the idea to boost stuck tx chains by CPFP and to be used as emergency tool if normal payout options do not work. Of course adding too much complexity to an emergency tool is not a good idea...

@chimp1984 Its a good idea and I'd like to take some time to think about a UI design for it.

Actually after more thought, I found that it might be ok to do it manually. It is exceptional anyway and the manual process via mediator brings some safety. Otherwise there might be risky situations (e.g. buyer sending signature but then later it gets confirmed and he starts the payment, but seller takes signature and gets back his trade amount with it...). I started a while back a "cancel trade" sub-protocol and it turned out that its more tricky and risky as it seemed first. Mainly because it introduced a parallel execution stream and that can cause problems.

So if your tool supports to change the miner fee, it would fulfill the basic requirements for a CPFP. The calculation how much fee is required can also be done manually by a explorer look up. I think its not worth the effort for that edge case.... Just would be good that we have a tool at hand to un-block stuck txs in times when the fees spike and sustain high.

Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Will test it tomorrow... Would be good to make the fee editable as well so it can be used for CPFP use case as well.

@ghost
Copy link
Author

ghost commented Dec 16, 2020

To adjust the fee, adjust one or both payout amounts.

The fee adjusts to the leftover.
No matter what is entered, the fee is input - output1 - output2.
e.g.

INPUT = 0.02202  OUTPUTS 0.00600 0.01600 FEE 0.000020
INPUT = 0.02202  OUTPUTS 0.00599 0.01590 FEE 0.000040

chimp1984
chimp1984 previously approved these changes Dec 16, 2020
Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK
Tested now and worked all! Great tool!
Small nit with catching invalid inputs at parseCoin...

@ripcurlx
Copy link
Contributor

Just re-triggered Travis, seems to be quite unreliable recently. I'll cherry-pick this into v1.5.2.

@ripcurlx ripcurlx added this to the v1.5.2 milestone Dec 16, 2020
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK based on #4899 (review)

@ripcurlx ripcurlx merged commit ceecb40 into bisq-network:master Dec 17, 2020
@ghost ghost mentioned this pull request Dec 27, 2020
@ghost ghost mentioned this pull request Jan 24, 2021
@ghost ghost deleted the 4061_privacy_improvement branch May 29, 2022 22:48
@ghost ghost mentioned this pull request Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stop requesting traders private keys for Manual payout
3 participants