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:

bisq/core/src/main/java/bisq/core/trade/protocol/bisq_v1/tasks/seller/SellerSignAndFinalizePayoutTx.java

Lines 75 to 82 in 92283de

	// In some error edge cases it can be that the address entry is not marked (or was unmarked).
	// We do not want to fail in that case and only report a warning.
	// One case where that helped to avoid a failed payout attempt was when the taker had a power failure
	// at the moment when the offer was taken. This caused first to not see step 1 in the trade process
	// (all greyed out) but after the deposit tx was confirmed the trade process was on step 2 and
	// everything looked ok. At the payout multiSigAddressEntryOptional was not present and payout
	// could not be done. By changing the previous behaviour from fail if multiSigAddressEntryOptional
	// is not present to only log a warning the payout worked.

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

bisq/core/src/main/java/bisq/core/trade/protocol/bisq_v1/tasks/seller/SellerSignAndFinalizePayoutTx.java

Lines 83 to 87 in 92283de

	log.warn("sellerMultiSigPubKey from AddressEntry does not match the one from the trade data. " +
	"Trade id ={}, multiSigAddressEntryOptional={}", id, multiSigAddressEntryOptional);
	}
	DeterministicKey multiSigKeyPair = walletService.getMultiSigKeyPair(id, sellerMultiSigPubKey);

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 #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