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

Add mempool lookup for trade transactions #5119

Closed
chimp1984 opened this issue Jan 26, 2021 · 8 comments · Fixed by #5160
Closed

Add mempool lookup for trade transactions #5119

chimp1984 opened this issue Jan 26, 2021 · 8 comments · Fixed by #5160

Comments

@chimp1984
Copy link
Contributor

chimp1984 commented Jan 26, 2021

Description

To avoid issues with invalid transactions in the trade process we should add a mempool lookup to detect early if a transaction did not get accepted in the mempool.

Beside the use case to verify if a tx is available and valid we can use that for verifying the trade fee payment of the peer.

Background context

BitocinJ only knows about own transactions not about the peers transaction. Also we do not get notified about an invalid transaction in case one of the inputs was already spent (TODO: check out if we receive an error back and if so if we can handle that).

Limitations

  • It takes a bit of time between the publishing of a transaction until is is found on an explorer. Depending on the use case that need to be considered.
  • There is no guarantee that every explorer node receives every transaction.

Those edge cases for false positives should be rather rare and the benefit will ourweight the risks of false positives.

Use cases:

Maker fee tx:

The maker can periodically check that their maker fee txs are found in the mempool. Once found they get removed from the list of transactions to check.

Taker verifies maker fee tx:

The maker fee tx ID is in the offer. At take offer the taker could request the mempool API to see if the maker fee transaction is found. If not, the taker receives a warning popup. They can manually lookup the tx as well to be sure it was not a false positive. For that case they can override the suggestion of the app to not take that offer.

Taker verifies maker fee payment:

When requesting the maker fee tx the taker can verify that the trade fee payment was as expected. There need to be used historical fee values (DAO parameters) in case the offer has been created in the past. There need to be a tolerance window as it might be that the offer was created at the last block of a cycle but the maker fee tx got into the block in the next cycle which could have changed the maker fee. One way would be to take the fee of the cycle of the maker fee tx and if that would fail to go one cycle back and try again with that previous value.

Verify that deposit tx inputs are matching the trade fee txs outputs.

This is not a mempool lookup issue but simply an additional check to detect bugs in case the deposit tx would be incorrect (there have been a past bug where the deposit tx was taken from another trade).

Maker verifies taker fee payment

We do not need to verify the taker fee transaction directly as the deposit transaction would never get confirmed if any of the inputs would be invalid. If the deposit transaction is not confirmed the trade protocol stays at step 1 and no fiat/altcoins will be transferred. To verify the fee payment of the taker we cannot do it reliably at the take offer phase in the trade protocol as the time is too short and the risks for false positives too high. But we can verify it later once the deposit transaction has been confirmed. In case the maker is the buyer they would never get to step 2 to start the payment in case the fee payment was incorrect. In case the maker is the seller, they would never get to step 3 for confirming receipt and releasing the BTC if the fee payment was not valid.

Arbitrator checks if delayed payout transaction is confirmed

When an arbitration ticket gets opened the arbitrator checks if the delayed payout transaction is confirmed. If not they need to wait until its confirmed.

Error handling

In case the lookup does not succeed, the app should use a second mempool API and try again. Only if that fails as well we consider the transaction as not found. The nodes are selected randomly.

Privacy

We use the mempool explorers operated by Bisq contributors. Those run as hidden service, so the only thing the explorer learns is that such a lookup request is likely a Bisq transaction, but as the trade transactions have some sort of "fingerprint" due their structure and connections that should not be a critical issue.

Preferences

We should allow users to setup their own mempool explorer (we only support one API type) in the preferences.
As the feature is not only a protection for the user but also for the peer and for the Bisq DAO (that trade fees are paid correctly) we should not make that feature optional.

@chimp1984
Copy link
Contributor Author

chimp1984 commented Jan 26, 2021

@jmacxx Would you have bandwidth to work on that?

@pazza83
Copy link

pazza83 commented Jan 28, 2021

Is there are way to only publish the offers once the transaction has been accepted in the mempool?

@chimp1984
Copy link
Contributor Author

Is there are way to only publish the offers once the transaction has been accepted in the mempool?

Good idea. Should be considered. Only issue I see that it adds a bit complexity to the offer creation part as the user get a delay which can take a few seconds or in the worst case longer and it has to be communicated to the user so he do not get surprised if it is not visible in the offer book. But at least we should check the state of the maker fee tx and if it is no found in the mempool after some time to alert the user.

@pazza83
Copy link

pazza83 commented Jan 28, 2021

I did not think this would be possible. I do not think the user experience aspect would be too much of a worry other than at the initial update.

If offers where only published when the transaction has been accepted in the mempool it would then enable the maker to place offers whilst being able to control the miner fee. I think this would result in more offers being made.

It would be great if a trader that wanted to make a large number of offers mid week could be on Bisq when the mempool was busy and place a number of offers that would then confirm at the weekend before appearing on Bisq as available for takers.

If the transactions did not get accepted in the mempool for any reason, trade would fail but no funds would be lost.

@chimp1984
Copy link
Contributor Author

chimp1984 commented Jan 28, 2021

Not sure I understand correctly:
Accepted in the mempool does not mean that the miner fee was high enough to get included in a block.
So we could publish the offer only once it is confirmed but that might come with considerable delays and I fear that has too much of a downside and it also does not mean the the other trade txs will get confirmed, so in times of volatile fees it still could lead to failed trades and the mess with required SPV resync to clean up the wallet.

If the transactions did not get accepted in the mempool for any reason, trade would fail but no funds would be lost.

The taker might lose the taker fee and it causes likely a support case.

I think what you aim for is to have a queue of new offers where you define the max tx fee you want to pay and those get published once the mempool is empty so that they have good chances to get confirmed. That could be done by keeping those offers locally in a queue and regularily check the recommended miner fee and once it is below your preferred miner fee they get published with the maker fee.
That is conceptually do-able but not a trivial dev effort.

@pazza83
Copy link

pazza83 commented Jan 28, 2021

Another use case might be when someone is happy to wait for few blocks.

For example yesterday there was a block that took ~1 hour to be confirmed. Fees where about 90 sat/vB. The blocks behind it where 4 sat/vB.

It would be great if a maker could place an offer for 10 sat/vB (9 fold saving) and know that there offer would likely confirm in 30 mins.

@pazza83
Copy link

pazza83 commented Jan 28, 2021

Accepted in the mempool does not mean that the miner fee was high enough to get included in a block.

Sorry I meant, wait till the maker tx id has at least one confirmation before making the the trade available to be taken on Bisq.

So we could publish the offer only once it is confirmed but that might come with considerable delays and I fear that has too much of a downside and it also does not mean the the other trade txs will get confirmed, so in times of volatile fees it still could lead to failed trades and the mess with required SPV resync to clean up the wallet.

My idea would be if the offer is only published once the maker fee is confirmed it would reduce the occurrence of failed trades. At least no active trades would fail due to maker tx id not being published.

Non active trades might still become failed trades, but as they are maker only they are are not live yet on Bisq so it would not really be an issue as no funds would be locked / lost, as they would not be available to take until confirmed.

I think what you aim for is to have a queue of new offers where you define the max tx fee you want to pay and those get published once the mempool is empty so that they have good chances to get confirmed. That could be done by keeping those offers locally in a queue and regularily check the recommended miner fee and once it is below your preferred miner fee they get published with the maker fee.

Yes that would be great if achievable.

That is conceptually do-able but not a trivial dev effort.

Ok, just thinking of ways to keep the miner fees as low as possible in relation to trade size. This will be come more important if initial trade limits are lowered as BTC price increases.

@chimp1984
Copy link
Contributor Author

At least no active trades would fail due to maker tx id not being published.

Yes. But I fear the trade off to always delay an offer to the risk that we get a congested mempool and an offer is never get confirmed is maybe not a good one. I think when we remove the risk for failed txs not included in the mempool we have solved the most critical problem.

Another option could be to add the maker fee (sat/vbyte) in the offer and handle it client side to filter/ignore offers with unconfirmed maker txs with a very low fee as those carry higher risk of delayed or never confirmed deposit txs. Though that would increase complexity for user how to interpret that data, so probably not a good idea...

We could also add the option for takers to only accpet offers where the maker fee is confirmed.

To make an explorer lookup for all offers might be a bit expensive as well, so probably better to limit it to offers the taker is intending to take.

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.

2 participants