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

Wait for AckMessage before completing BuyerSendCounterCurrencyTransferStartedMessage task #5401

Merged
merged 1 commit into from Apr 12, 2021

Conversation

ghost
Copy link

@ghost ghost commented Apr 9, 2021

Fixes seller-cant-confirm issue by transitioning past BuyerSendCounterCurrencyTransferStartedMessage only after an AckMessage response. Previously it would transition upon mailbox message delivery.

Scenario from log of issue aTPPev (and others) :

  • Alice creates an offer to buy. Bob takes the offer. Blockchain confirms.
  • Alice initiates fiat payment and clicks on Payment sent.
  • 12 seconds later Alice clicks Payment sent.
  • 11 seconds later Alice clicks Payment sent.
  • 12 seconds later Alice clicks Payment sent.
  • 9 seconds later Alice log receives "Message arrived at peer" (4 times).
  • Alice transitions to the next step and no longer has the option to click Payment sent.
  • [10 hours passes], Ack message has not been received from Bob.
  • Bob is still waiting for BuyerSendCounterCurrencyTransferStartedMessage, so he can not click on "Payment received".
  • Alice and Bob exchange chat messages and eventually open a Dispute.

In the scenario above, Alice would keep being prompted to "send confirmation again" (until she receives AckMessage) and eventually DM with Bob to ask what the issue is.

  • Bob might say he has not received payment notification, he could restart Bisq and maybe the message would go through,
  • or Bob might have already transitioned to showing the "Confirm Payment Receipt" button, and therefore could pay out the trade.
  • The trade can be paid out normally even if Alice has the Payment started button showing. As long as Bob processes a BuyerSendCounterCurrencyTransferStartedMessage => can click "Confirm Payment Receipt".
  • This fix gives more chance for BuyerSendCounterCurrencyTransferStartedMessage to be re-sent.

image

tag @chimp1984 pls 👀

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

@chimp1984
Copy link
Contributor

Ah great that tackle that issue! Your suggested solution looks good to me.

Seems there are some edge cases where the message arrives but the peer does not process the message (maybe just shuts down app or some edge case bugs). Maybe related with hibernate behaviour at some OS. I saw sometimes offers I could never take because the peer did not respond (in time), so that looked like the network throttling behaviour for apps in hibernate state. Unfortunately hibernate strategies getting more and more complex and instransparant with each new OS version, at least on OSX and Windows will be worse for sure...

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 #5401 (review)

@ripcurlx ripcurlx merged commit 2c3c936 into bisq-network:master Apr 12, 2021
@ripcurlx ripcurlx added this to the v1.6.3 milestone Apr 12, 2021
@ghost ghost mentioned this pull request May 4, 2021
@ghost ghost deleted the buyer_wait_for_ack branch May 29, 2022 22:46
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.

2 participants