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

Monero auto-confirm fails with AMOUNT_NOT_MATCHING or NO_MATCH_FOUND in certain cases #4557

Closed
wiz opened this issue Sep 23, 2020 · 11 comments · Fixed by #4560
Closed

Monero auto-confirm fails with AMOUNT_NOT_MATCHING or NO_MATCH_FOUND in certain cases #4557

wiz opened this issue Sep 23, 2020 · 11 comments · Fixed by #4560

Comments

@wiz
Copy link
Member

wiz commented Sep 23, 2020

Description

The new Monero trade auto-confirm feature may fail if the XMR transaction is not yet confirmed at the time the BTC buyer presses "payment started" and submits the XMR proof of transaction data to BTC seller

Version

v1.3.9

Steps to reproduce

  1. Begin an XMR/BTC trade
  2. After the buyer sends the XMR payment, submit the XMR proof of transaction before the XMR payment is confirmed by the Monero blockchain

Expected behaviour

Bisq should retry after a while if the XMR transaction is not yet confirmed

Actual behaviour

Bisq instantly fails the auto-confirm functionality, despite the XMR transaction getting confirmed shortly after

Additional info

Result{
     detail=Detail{
     numConfirmations=0,
     errorMsg='null'
} NO_MATCH_FOUND
} FAILED
This might not mean that the XMR transfer was invalid but you have to check yourself if the XMR transfer was correct. Result{
     detail=Detail{
     numConfirmations=0,
     errorMsg='null'
} AMOUNT_NOT_MATCHING
} FAILED
@jmacxx
Copy link
Contributor

jmacxx commented Sep 23, 2020

If the amount does not match, then it has to fail, that's a core tenet of the functionality.
If the amount did match, but the software says it does not, could you provide logs?

@chimp1984
Copy link
Contributor

Maybe it has to be fixed on the service side. If the tx is unknown yet it should not return AMOUNT_NOT_MATCHING but the other error we interpret already for that situation.

@jmacxx
Copy link
Contributor

jmacxx commented Sep 23, 2020

Amount not matching means we got the details of the tx and they did not match.

@wiz
Copy link
Member Author

wiz commented Sep 24, 2020

@jmacxx I don't think so, since the user who reported this bug says they were able to manually confirm the trade fine. I can send you logs privately if you want to investigate more.

@jmacxx
Copy link
Contributor

jmacxx commented Sep 24, 2020

Yes please, logs would help.

@jmacxx
Copy link
Contributor

jmacxx commented Sep 24, 2020

Thanks for the logs. The amount check between bisq and the explorer API returned a difference after the 8th decimal place and AMOUNT_NOT_MATCHING was the result. The difference was as follows:

xx.xxxxxxxx7000 XMR expected by bisq
xx.xxxxxxxx0000 XMR provided by explorer API

Looks like this should be fixed by truncating the last 4 digits of the amounts before comparing. Especially since Bisq displays the XMR amount to be sent using 8 decimal places.

I should have anticipated this during testing. Comparing numbers that have been achieved by converting one currency to another should always have a tolerance taken into account, and checking XMR amounts down to the 8th decimal should be adequate, even though Monero has 12.

@jmacxx
Copy link
Contributor

jmacxx commented Sep 24, 2020

Well, after doing some testing I think this problem is even more convoluted. Bisq already does truncate the amount expected to 8 decimal places, thanks to the Monetary class. My theory above was wrong.

The only other issue I can find is that there were different maker and taker prices at the time of the trade (confirmed by the logs). It is possible (though not yet confirmed) that the maker used 0.00848493 (X) price while the taker used 0.00848404 (Y). So the taker sending the XMR was told to send BTC/Y XMR while the maker was expecting BTC/X XMR. The resultant difference in amount would then be at the third decimal place. I need to try this out in a test.

@jmacxx
Copy link
Contributor

jmacxx commented Sep 24, 2020

Found the bug, XMR autoconfirm task is getting the amount from the offer; it should use the trade which can be different due to the taker's price having the chance to be slightly different.

Volume volume = checkNotNull(trade.getOffer()).getVolumeByAmount(tradeAmount);

Should be: Volume volume = trade.getTradeVolume();

This can be verified by doing an autoconfirm trade, forcing the taker's price to be different (L194 of TakeOfferDataModel).

jmacxx added a commit to jmacxx/bisq that referenced this issue Sep 24, 2020
Get the amount from the trade rather than the offer.

Log the expected amount when not matching the explorer result.
It would have made debugging this issue much more straightforward.

Fixes bisq-network#4557
@wiz
Copy link
Member Author

wiz commented Sep 24, 2020

@chimp1984 do you think we should ship a hotfix release for this bug?

@wiz
Copy link
Member Author

wiz commented Sep 25, 2020

@jmacxx great work investigating the AMOUNT_NOT_MATCHING, apparently this is actually two separate bugs - what about my original suspicion that the XMR transaction was not yet confirmed on Monero blockchain for the NO_MATCH_FOUND case?

@jmacxx
Copy link
Contributor

jmacxx commented Sep 25, 2020

There are two cases where it continues to poll the API: transaction not found, or all checks pass but not enough confirmations.
The transaction was found, the amount did not match and therefore the autoconf cannot go through. No need to wait for confirmations.

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 a pull request may close this issue.

3 participants