Invalid PayoutTxPublished message closes trade leaving funds locked

[ghost]

@pazza83 reports some trades are moved to closed having funds still locked in the multisig.

• Logs show payout tx paying a different address than was in the contract. Bitcoin core rejected the tx due to signature not matching the script. The funds remained locked.
• Bisq accepted the PayoutTxPublished P2P message and closed the trade.

Suggested resolution

Bisq should validate the received P2P message, and if any payout address does not match the trade contract, display an error and leave the trade open. Then the user will be able to get support from a mediator.

---

Question / Alternate resolution

Bisq already closes a trade when it detects a payout tx in its wallet. The additional path to close a trade upon receipt of a P2P message seems unnecessary -> would it make sense to remove the duplicate path?

[ghost]

After investigating a further batch of 22 closed trades matching this scenario, I noticed the reason for the payout address being different. The code called getOrCreateAddressEntry(id, TRADE_PAYOUT) which creates a new address if one is not found. The resulting new address would cause transaction failure because the buyer has signed their version of the payout to the addresses specified in the contract. The examples from @pazza83 show that sometimes AddressEntryList is missing data, presumably due to a catastrophe causing them to restore a backup / PendingTrades but not AddressEntryList. This is backed up by the following code comment:

https://github.com/bisq-network/bisq/blob/92283de42a34d3b26a2b92605d0c65467fc4ab36/core/src/main/java/bisq/core/trade/protocol/bisq_v1/tasks/seller/SellerSignAndFinalizePayoutTx.java#L75-L82

The code already attempts to handle this scenario by obtaining the signing key from the wallet:

https://github.com/bisq-network/bisq/blob/92283de42a34d3b26a2b92605d0c65467fc4ab36/core/src/main/java/bisq/core/trade/protocol/bisq_v1/tasks/seller/SellerSignAndFinalizePayoutTx.java#L83-L87

In addition to that it needs to use the payout address from the trade contract (instead of a fresh address), to ensure a successful payout:

sellerPayoutAddressString = checkNotNull(trade.getContract()).getSellerPayoutAddressString();

Either it could always use the contract's payout address, or it could fallback to the contract's payout address only if the AddressEntryList record is missing.

[pazza83]

Hi @jmacxx thanks for investigating this.

I think this issue is a serious as from both the buyer and sellers perspectives the trades have completed and no further action on their parts are needed. This makes it possible for users to not notice the fact they have locked funds. I think in the last few months there have been at least 3 sets of buyers and sellers that have had a similar issue.

If a user does notice the trade being in their history means it is often difficult to contact their trade peer to arrange a manual payout. This leads to arbitration being required to solve the issue and potentially one of the peers losing bitcoin despite not being aware of the issue.

@jmacxx would it be possible to run an update report to see how much BTC is locked in trades since the last report in September 20022

Also @jmacxx would it be possible to have something similar to https://github.com/bisq-network/bisq/pull/6998 where either buyer or seller can force the initiation on a mediation ticket from either their 'Failed' or 'History'? This would allow for the user with the 22+ trades to open mediation from their trade history and for the mediator to then have open communication channels to both the buyer and seller. It would also have helped the previous users in similar situations. From a support / mediation perspective they are difficult and time consuming cases to work on as the mediator has to rely on one of the users providing the information to them via Matrix (trade details screen, json file, logs, hex signatures etc). It would be so much easier to manage if the information was shared via Bisq in mediation. To prevent 'mediation spam' maybe the shortcut required for a user to open mediation on a historic or failed trade could be shared just with the mediators. That way the mediators could provide it to users only where necessary.

[ghost]

Found 3.93663465 BTC total locked value in the past 18 months. https://github.com/jmacxx/WIP/blob/master/bisqTxChecker/report.txt

I think not really feasible to do mediation from failed and history. The bug fix outlined above about using contract payout address is an obvious & simple fix to an obscure bug. I asked for @HenrikJannsen to indicate their approval of this concept.

final String sellerPayoutAddressString;
Optional<AddressEntry> x = walletService.getAddressEntry(id,AddressEntry.Context.TRADE_PAYOUT);
if (x.isPresent()) {
    sellerPayoutAddressString = x.get().getAddressString();
} else {
    sellerPayoutAddressString = checkNotNull(trade.getContract()).getSellerPayoutAddressString();
    log.warn("Payout address missing from AddressEntry, using contract instead: {}", sellerPayoutAddressString);
}

[pazza83]

Starting with the manual payouts now. For trade ID 7867234:

DepositTXId: 10d274c1058d616f934c9c6e88bb9f141d729cec6f916d4b5732f9dd40f6ecdb AmountInMultisig: 0.01370718 buyerPayoutAmount: 0.0118 buyerPayoutAddress: bc1qd8n9xa9c3a25z4wtu7cpx98pplu4l3rflrasgj sellerPayoutAmount: 0.0018 sellerPayoutAddress: bc1qxc4nmghhrdd0mqpvufeqdypeenmvqrapf6udse Txfee: Tx fee%: BuyerPubKeyasHex: 02cea3fc87c2a51a9993aa07b2c064616baa37e2fabb3509eaddfb11d7cea89f4e SellerPubKeyAsHex: 033859e0d6f384f94cb1e51cfa26252ddd3f8f0be876e3c13cb783e5b72d518784

Looks like it is a case of address reuse by the buyer and seller which would explain why the trade ended in both their histories

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[HenrikJannsen]

Still relevant

[pazza83]

Another case of this. Trade grwobcq

It is being paid out as an off Bisq reimbursement.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[HenrikJannsen]

Still relevant

[pazza83]

Yes another report of this issue again today. We get a couple a month