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

Always use fresh address for MULTI_SIG context #5885

Merged
merged 1 commit into from Dec 4, 2021
Merged

Always use fresh address for MULTI_SIG context #5885

merged 1 commit into from Dec 4, 2021

Conversation

ghost
Copy link

@ghost ghost commented Dec 1, 2021

Fixes #5880, Fixes #5661

It is currently possible for the user to choose a receiving address from Funds -> Receive, request their exchange withdrawal to that address; then continue to take an offer on Bisq. If this happens, the funds deposited from the exchange will never show in Bisq, as the same address is used in the trade with a MULTI_SIG context meaning that it is forever excluded from balance calculations.

Solution is to always generate a fresh address when obtaining a MULTI_SIG reservation.

Testing (pre)

  1. Bob has 2 bech32 addresses shown in Funds -> Receive,
  2. Alice creates offer
  3. Bob presses Take Offer, then Next Step. One of the 2 addresses is reserved for trade funding.
  4. Bob takes note of the receiving address still shown in Funds -> Receive: "address_b".
  5. Bob completes the Take Offer process.
  6. Bob deposits to "address_b" (picked it from Funds -> Receive earlier).
  7. Bring the trade to normal completion.
  8. Notice that the deposited amount is missing, but is shown in Transaction list & the Emergency Wallet (ctrl-e).

Testing (post)

  1. Bob has 2 bech32 addresses shown in Funds -> Receive,
  2. Alice creates offer
  3. Bob presses Take Offer, then Next Step. One of the 2 addresses is reserved for trade funding.
  4. Bob takes note of the receiving address still shown in Funds -> Receive: "address_b".
  5. Bob completes the Take Offer process.
  6. Bob deposits to "address_b" (picked it from Funds -> Receive earlier).
  7. Bring the trade to normal completion.
  8. Notice that the deposited amount is OK.

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 - Code changes look fine to me, but I'd like to wait for an ACK from either @sqrrm or @chimp1984 who are more familiar with the wallet codebase.

@ripcurlx ripcurlx added this to the v1.8.0 milestone Dec 2, 2021
@chimp1984
Copy link
Contributor

Thanks @jmacxx for finding that bug!
I could reproduce it and confirm that your fix resolves the issue.

Here are some more info how to reproduce (as it took me a bit).

One cannot create a new address if one unused is there. To get over that click take offer, then the unused address is removed (as used for reserved for trade) and one can create a new address. This new address (A2) will later be used for the multisig. Take the offer and then send funds to that address A2. One can check also with cmd+j the wallet details and see if that address was used for the multisig.

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

@ripcurlx ripcurlx merged commit e576829 into bisq-network:master Dec 4, 2021
@ghost ghost mentioned this pull request Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants