Bisq does not verify taker's funding TX is broadcast to Bitcoin P2P network

[wiz]

Description

Bisq allows its offers to be taken without verifying the taker's funding TX was actually broadcast to the Bitcoin P2P network.

Version

1.1.7

Steps to reproduce

1. Configure Alice's Bisq node to only connect to a single a Bitcoin node that does not relay transactions to the Bitcoin P2P network (i.e. configure it to have zero peers)
2. Create an offer on Bob's node
3. Take the offer on Alice's node

Expected behaviour

Alice's node should not attempt to take the offer if it cannot broadcast a valid funding TX to the Bitcoin P2P network.

Bob's node should not allow Alice to accept his offer if Bob cannot verify that Alice's funding TXID has been seen by at least 1 of his Bitcoin peers.

Actual behaviour

Bob's offer gets accepted state, but the trade can never begin because Alice's funding TX was not broadcast to the Bitcoin P2P network. Additionally, Bob's funds are now stuck in limbo.

Screenshots

[chimp1984]

The peer's trade fee tx is not knows to BitcoinJ as it is not related to the wallet. Beside that it is a valid situation that the peer's trade fee is not confirmed or not even found in the mempool, specailly for the taker fee tx that is likely the case as the time between publishing taker fee tx and deposit tx is very short and the tx is likely not propagated much in the network.

We have to find out what causes those bugs with not published txs. It might be related to the AddressEntry management, that there is a bug in some edge cases.

[brincobt]

@wiz: I have put my log at a web URL. See bisq chat, please.

[wiz]

@chimp1984 since BitcoinJ / SPV cannot verify this TX, we are discussing having all the btcnode operators start running a block explorer API like electrs on their servers - this will allow Bisq to query the full node if a TX exists in the mempool or not, which means the fullnode would have verified it. Users will be able to choose which node(s) to verify this TX using, including their own full node if they have one.

[wiz]

Bisq can be modified to query the full node REST API with the TXID like this, if it exists in mempool then taker can be allowed to accept the trade offer. http://jiuuuislm7ooesic.onion:3000/tx/44d675e87d667d89c471a24269489a7a37eb631bde8e75439c42d036f6f7de54

[wiz]

Actually I found this TODO item which seems to be to add the missing verification without needing electrs https://github.com/bisq-network/bisq/blob/master/core/src/main/java/bisq/core/trade/protocol/tasks/maker/MakerVerifyTakerFeePayment.java#L41

[wiz]

So far 2 btcnode operators are online running electrs nodes:
@Emzy http://ldv2izp7qs3rpsxv.onion:3000/address/1wizSAYSbuyXbt9d8JV8ytm5acqq2TorC
@wiz http://jiuuuislm7ooesic.onion:3000/address/1wizSAYSbuyXbt9d8JV8ytm5acqq2TorC
@devinbileck is working on his now and I will ask @mrosseel next

[chimp1984]

Please be aware that this approach likely is not feasible (take offer process takes 1-2 sec. not enough time to enure a tx is visible to all nodes). Please don't rush in a wrong direction before we have a clear picture whats the cause of the problem.

[wiz]

Well to be brutally honest, the approach of trusting 0-conf TXs to accept Bisq trade offers isn't feasible either. How else can we solve this fundamental design flaw in the Bisq trade protocol, if not to delay accepting the offer by a few seconds until Bisq can verify the 0-conf TX is actually in the Bitcoin mempool? It seems to fix this properly would require re-architecting the trade protocol quite a bit.

[wiz]

Moving discussion to bisq-network/proposals#132

[wiz]

@ripcurlx I am going to re-open this issue because while the concept of "failed trades" was introduced, which reduced the severity of this issue so that it does not cause wallet corruption, it still inconveniences the user and Bisq should verify the taker's funding TX. I proposed using a REST API electrs backend on our federated btcnodes for this purpose

[huey735]

To add to that. Given the issues Bisq has been having, I'm vehemently against relaying offers with not-broadcast or unconfirmed maker fee tx. Thinking of it it seems like a possible point of attack to flood the network, strand takers funds and strain support personnel.

[wiz]

Yes, that is another missing part of the trade protocol from the original design, but since Bisq is SPV it cannot verify transactions that are outside of its wallet without using some API to a trusted Bitcoin fullnode

[ripcurlx]

Yes, that is another missing part of the trade protocol from the original design, but since Bisq is SPV it cannot verify transactions that are outside of its wallet without using some API to a trusted Bitcoin fullnode

There is actually a task (TakerVerifyMakerFeePayment, MakerVerifyTakerFeePayment) within the trade protocol that wasn't implemented because of the SPV limitations.

//TODO impl. missing

[wiz]

Right, so we want to use a REST API electrs backend for this verification, to our federated btcnodes

[wiz]

Query all of our federated nodes, if all of them return 404 for this API call, then consider the TX to be invalid
https://blockstream.info/api/tx/bd0c2270af24be1c6ef399c1b99e3f66481c3cfd98ce205985d3102907c765eb

[wiz]

@sqrrm @freimair @ripcurlx
I thought of a better workaround than using electrs backend - take the following case:

depositTx received from peer: 
  29f9666b64df1b162f64bfa2bdf9ef510d65b02e36a8e9e1b3bebb3b5487e7fe
  updated: 2020-03-20T03:10:42Z
     in   PUSHDATA(72)[3045022100f3a09edfa44027e035d2d5ffe3100d5df24a2737e16342be04907bb6d072ee0d022068eb82020b18151877e541e2040c02cdbb4011f992e73ab4c6309f984fa0f1ba01] PUSHDATA(33)[03b251a0397992ad03577d0cedd95aca5d562e7e5190887bacb8c68ff21e4d0b3a] 0.013485 BTC (1348500)
          outpoint:92f708c6c317b7293ac360a7e0525d968f0c423b407f725121b39c5e7be9e9eb:1 hash160:53ebe7e1ec28e89ed5326fc179c9ab37d865217d
     in   PUSHDATA(72)[304502210080aab58d142d47bcfa70fac45cf549f47d077b30176fbfaad075aba41b76b196022057fdede2135016197aa14a079a2b7a916b3a59a909162ddbd2a92d293237be2901] PUSHDATA(33)[02e080be8d5ba65e74fd167063533cca2bd74e8c8af473b923acaa43cb76f5917c]
          outpoint:1f51b6a116726799a18bb82943a1b92683785650e4b58c96d7dddf5bcc54e76a:1
     out  HASH160 PUSHDATA(20)[38abc168a78caba347e1a63623479c08a33b970c] EQUAL 0.117136 BTC (11713600) ScriptPubKey: a91438abc168a78caba347e1a63623479c08a33b970c87 Address:36rfX4tS12TPvUqRRj8yrhvMiNxb7dwjMh 
     out  RETURN PUSHDATA(32)[c072d153c502af690eb95a909b492ce22d4d4068957c134164358d3597ac94eb] 0.00 BTC (0) ScriptPubKey: 6a20c072d153c502af690eb95a909b492ce22d4d4068957c134164358d3597ac94eb Address:[exception: Cannot cast this script to a pay-to-address type]
     prps UNKNOWN

In this deposit TX, the taker's funding TX 1f51b6a116726799a18bb82943a1b92683785650e4b58c96d7dddf5bcc54e76a does not exist in mempool or blockchain, so the deposit TX is invalid because it spends an invalid UTXO.

My suggested workaround, is to have both trade parties re-broadcast the deposit TX to their Bitcoin peers. They should expect either a successful broadcast, or a "already in mempool" error, which also indicates a valid TX. If they receive any other error when re-broadcasting the TX, such as "spends invalid UTXO", etc. the maker can assume the taker's funding TX is not valid, and it can reject the peer attempting to take the offer.

[chimp1984]

The trade protocol must not be delayed by the API call as that would just introduce more problems like network disconnections or timeouts. It would also decrease UX.

I recommend to do that API lookup after a certain period and then display a waring to the user.

The cases if "too long unconfirmed tx chain" can be handled differently by limiting the offer to be created or offers taken by the number of unconfirmed transactions in the users wallet. That is not perfect and does not handle all possible cases but I think that will reduce those issues by > 90% and is relatively easy to implement and does not cause UX issues for most users (only those who create many offers in a row which is not a standard behaviour).

Also note that trades with "too long unconfirmed tx chain" still can be confirmed later and the trade can be completed normally.

SPV resync have to been taken carefully. It can cause more harm as it helps, for instance if a tx does not get confirmed fast but would have been confirmed in a few hours a SPV resync of one trader will render the deposit tx invalid if that user makes a double spend in the meantime. Then the other user has a failed deposit tx and require a SPV resync as well.

The seller is doing the publishing of the deposit tx. If the buyer would do it as well it has to be taken care that the wallet does not add 2 times the same tx (it will hear from the network once the seller broadcasts) as that cause problems in BitcoinJ. It is tricky to test as the timing is not guaranteed if the tx comes first or the P2P message which triggers the re-broadcast of the buyer. I don't recommend to add here more complexity and fragility without having clear evidence that this is causing a problem.

[stale]

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.

[stale]

This issue has been automatically closed because of inactivity. Feel free to reopen it if you think it is still relevant.