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

Fix failed trade / missing payment information #5679

Merged
merged 1 commit into from Sep 6, 2021
Merged

Fix failed trade / missing payment information #5679

merged 1 commit into from Sep 6, 2021

Conversation

ghost
Copy link

@ghost ghost commented Aug 30, 2021

Problem description:
A trade will fail if the deposit transaction is received via the mempool before it is received via P2P messaging protocols.

During initial trade protocol messaging, the deposit transaction is received at the buyer via two possible paths: a P2P message DepositTxAndDelayedPayoutTxMessage, or received in the mempool.
Receiving via P2P message is much more likely, but in rare cases when received via the mempool first, we get the following error:

Log of broken version:

Aug-30 07:52:20.383 [JavaFX Application Thread] INFO  b.c.btc.wallet.WalletService: depositTx received from network:
Aug-30 07:52:36.318 [JavaFX Application Thread] INFO  b.c.t.p.FluentProtocol$Condition: We received a DepositTxAndDelayedPayoutTxMessage at phase DEPOSIT_PUBLISHED and state BUYER_SAW_DEPOSIT_TX_IN_NETWORK
Aug-30 07:52:36.319 [JavaFX Application Thread] WARN  b.c.t.protocol.BuyerProtocol: We with a DepositTxAndDelayedPayoutTxMessage but we have already processed the deposit and delayed payout tx so we ignore the message. This can happen if the ACK message to the peer did not arrive and the peer repeats sending us the message. We send another ACK msg. 
Aug-30 07:52:36.319 [JavaFX Application Thread] INFO  b.c.t.protocol.TradeProtocol: Send AckMessage for DepositTxAndDelayedPayoutTxMessage to peer 
Aug-30 07:52:36.321 [JavaFX Application Thread] INFO  b.c.t.protocol.TradeProtocol: Remove DepositTxAndDelayedPayoutTxMessage from the P2P network. 
Aug-30 07:52:36.321 [JavaFX Application Thread] WARN  b.c.t.protocol.TradeProtocol: PreConditions not met. preConditions=[false]
Aug-30 07:52:36.321 [JavaFX Application Thread] ERROR b.c.t.protocol.TradeProtocol: Task runner failed with error PreConditions not met. preConditions=[false],
... followed by missing payment instructions and the trade cannot be completed.
Aug-30 07:59:02.876 [JavaFX Application Thread] ERROR b.d.m.p.p.s.b.BuyerStep2View: Not supported PaymentMethod:

The handling of the P2P message is not carried out because it erroneously thinks it has been handled before. The preconditions are incorrect.

Preconditions are changed via this PR, checking if the payment account payload has already been received. If not, then the P2P message will be processed.

Log of fixed version:

Aug-30 08:45:44.418 [JavaFX Application Thread] INFO  b.c.btc.wallet.WalletService: depositTx received from network:
Aug-30 08:46:04.643 [JavaFX Application Thread] INFO  b.c.t.p.FluentProtocol$Condition: We received a DepositTxAndDelayedPayoutTxMessage at phase DEPOSIT_PUBLISHED and state BUYER_SAW_DEPOSIT_TX_IN_NETWORK
Aug-30 08:46:04.643 [JavaFX Application Thread] INFO  b.c.btc.wallet.WalletService: depositTx received from peer:
Aug-30 08:46:04.648 [JavaFX Application Thread] INFO  b.common.taskrunner.TaskRunner: Run task: ApplyFilter 
Aug-30 08:46:04.648 [JavaFX Application Thread] INFO  b.common.taskrunner.TaskRunner: Run task: VerifyPeersAccountAgeWitness 
Aug-30 08:46:04.649 [JavaFX Application Thread] INFO  b.common.taskrunner.TaskRunner: Run task: BuyerSendsShareBuyerPaymentAccountMessage 
Aug-30 08:46:04.649 [JavaFX Application Thread] INFO  b.c.t.p.t.SendMailboxMessageTask: Send ShareBuyerPaymentAccountMessage to peer 

This PR was a collaboration between myself (analysis & test), and @chimp1984 (suggested code fix).
The problem has been replicated on mainnet, and the fix tested on mainnet.

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

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

@ripcurlx ripcurlx merged commit e8f417f into bisq-network:master Sep 6, 2021
@ripcurlx ripcurlx added this to the v1.7.4 milestone Sep 6, 2021
@ghost ghost deleted the fix_deposit_received_from_network_issue branch May 29, 2022 22:51
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