Verify maker & taker fee transactions via Mempool lookup #5160

ghost · 2021-02-06T20:05:29Z

Closes #5119

Queries mempool at checkOfferAvailability stage (take offer).
Validates maker transaction and trading fee.
Indicates status at UI level (see screenshots below)
Queries mempool at trade initiation (step 1&2) before fiat payment initiated.
Maker validates taker transaction and trading fee.
Informs user to open a dispute if the trading fees were not paid
Arbitrator ticket informs about the confirmation status of delayed payout

Screenshots

https://imgur.com/a/tSaUgRL

Safeguards

Can be disabled universally via a filter, and individually via config setting.

In regtest mode the API calls are bypassed, returning success status.

Accuracy

The fee schedule is set via DAO parameters. Therefore checking that the correct fee was paid involves comparing the block height of an offer Tx to the DAO parameters which were relevant at that time.

I observed some variance in fee payments, perhaps because different users nodes may have been out of consensus with the DAO state.

The validation code has a threshold at which it considers fees paid to be acceptable. Currently this is set at 95%, i.e. if the fee paid is >= 95% of what was expected, the validation passes.

Very old offers (of which there are a handful still out there), use fee addresses which are not known but perhaps were used prior to the DAO. So a threshold of blockheight 599999 is used such that the unknown fee addresses are allowed to pass when the offer Tx is older than the threshold. Alternatively we could add 1PUXU1M... and 18GzH11... to the validator's list of accepted fee addresses.

private final double feeTolerance = 0.95;     // we expect fees to be at least 95% of target
private final long blockTolerance = 599999L;  // allow really old offers with weird fee addresses

Out of ~450 current offers ~9 are flagged as having underpaid the trading fee.

Checks own open offers

To implement the requirement that one's own offers be checked I integrated a validation in the TriggerPriceService so that if an offer is found to be bad it gets automatically disabled. This can help prevent the issues we've been seeing of offers that do not get published to the mempool for hours/days sometimes. Now if an offer Tx is not found, it will get disabled and the user will be motivated to investigate.

Testing

Tests were written in TxValidatorTest to check the various error scenarios, be it underpaid BSQ, BTC, unknown fee address, or unknown tx. Most of the tests use data from real-world scenarios like the dust-fee scam.

PR Checklist:

Validate offer maker tx and fees.
Arbitrator checks that delayed payout tx is confirmed.
Verify deposit tx inputs match maker/taker outputs.
Maker verify taker fee payment.
Validates own open offers, disables ones that are bad.

Conza88 · 2021-02-06T21:17:10Z

I thought it did this already 😳

ripcurlx · 2021-02-10T09:03:52Z

I thought it did this already 😳

Only for transactions that are linked to your wallet (e.g. Bisq fee tx of the trading peer wasn't checked)

Conza88 · 2021-02-17T22:42:33Z

If there is a problem it warns user to back out (user can still continue).

Would there be a scenario when that was ever advisable? Or worth doing?

chimp1984 · 2021-03-13T15:29:42Z

core/src/main/java/bisq/core/provider/mempool/TxValidator.java

+        long feeValue = jsonVIn0Value.getAsLong() - jsonFeeValue.getAsLong();
+        // if the first output (BSQ) is greater than the first input (BSQ) include the second input (presumably BSQ)
+        if (jsonFeeValue.getAsLong() > jsonVIn0Value.getAsLong()) {
+            // in this case 2 or more UTXOs were spent to pay the fee:


The code handles only the case of 1 or 2 BSQ inputs. Maybe also add a comment that we building on the concention that BSQ inputs are the before BTC inputs, which is not a consensus rule (for outputs it is) but our code handles it like that, so for the fee check that is good enough as we do not expect that ppl use custom txs for those txs.
It could be also a rare case that there is no BSQ change output. That can be if the input is exactly the amount to be burned or if the change is < dust, then it gets added to miner fee.

Another issue is that we do not check if the burned satoshis are BSQ or BTC. That would require that the tx is confirmed as only at confirmation the BSQ explorers recognize a BSQ tx. As confirmation will happen later we need to do that check after the deposit tx is confirmed in the trade protocol. So before the buyer sends the fiat he will check the takers BSQ fee it the BSQ explorer returns it as a valid BSQ tx and then before the seller confirms receipt to check the makers fee tx.

chimp1984 · 2021-03-13T16:40:51Z

core/src/main/java/bisq/core/provider/mempool/TxValidator.java

+        JsonObject jsonVin0 = jsonVin.get(0).getAsJsonObject();
+        JsonObject jsonVout0 = jsonVout.get(0).getAsJsonObject();
+        JsonElement jsonVIn0Value = jsonVin0.getAsJsonObject("prevout").get("value");
+        JsonElement jsonFeeValue = jsonVout0.get("value");


The BSQ fee check is tricky. Maybe a better approach is to postpone it directly to the trade protocol states when the deposit tx is confirmed and use the BSQ explorer for delivering the fee.
In BsqWalletService.completeBsqTradingFeeTx there have been added a while ago a change which made the input 0 a BTC output in case there is no BSQ output. I think that change is not needed (at least I tested it without and it worked, but don't want to touch that at the moment...). So to take that additional option into account all gets more complicated....

core/src/main/java/bisq/core/trade/TradeDataValidation.java

core/src/main/java/bisq/core/provider/mempool/MempoolService.java

desktop/src/main/java/bisq/desktop/main/offer/takeoffer/TakeOfferView.java

desktop/src/main/java/bisq/desktop/main/offer/takeoffer/TakeOfferViewModel.java

ghost · 2021-03-14T03:28:18Z

👍 Thanks @chimp1984 for the review & suggestions. I applied your changes, and will think about the suggestion of checking BSQ fees using a BSQ explorer.

ripcurlx · 2021-03-17T08:44:35Z

@jmacxx I'm reviewing this PR right now and think it would be great to get it into v1.6.0. Could you please resolve the conflicts mentioned and I'm happy to merge it after a positive review.

core/src/main/java/bisq/core/offer/TriggerPriceService.java

ripcurlx · 2021-03-17T14:39:20Z

I just got following error while trying to open a dispute as mediator:

März-17 15:33:03.377 [JavaFX Application Thread] ERROR bisq.common.setup.CommonSetup: Uncaught Exception from thread JavaFX Application Thread 
März-17 15:33:03.377 [JavaFX Application Thread] ERROR bisq.common.setup.CommonSetup: throwableMessage= null 
März-17 15:33:03.378 [JavaFX Application Thread] ERROR bisq.common.setup.CommonSetup: throwableClass= class java.lang.NullPointerException 
März-17 15:33:03.381 [JavaFX Application Thread] ERROR bisq.common.setup.CommonSetup: Stack trace:
java.lang.NullPointerException
	at bisq.core.provider.mempool.TxValidator.initialSanityChecks(TxValidator.java:260)
	at bisq.core.provider.mempool.TxValidator.parseJsonValidateTx(TxValidator.java:131)

This happens as jsonTxt is "" on Regtest.

desktop/src/main/java/bisq/desktop/main/offer/takeoffer/TakeOfferDataModel.java

ripcurlx · 2021-03-17T15:08:44Z

It wasn't changed in this PR, but I missed it in the other one.
Please don't use version-new as class to highlight this text, as there might be a problem in the future if someone changes this class. It also shows a hand icon which indicates that this underlined label is clickable.

tradePeriodEnd.getStyleClass().add("version-new"); // highlight field when the trade period is still active

Better would be e.g. to create a new class .alert without underline and hand, so it doesn't look like a link.

.alert {
    -fx-text-fill: -bs-rd-error-red;
}

ripcurlx · 2021-03-17T15:12:53Z

And one more thing I missed in the other PR and feels a little bit weird for the user. If a dispute is closed you get the notification popup. When you click on it, it opens the support section and the issue is selected. But it is greyed out and users might not know what to do. Maybe it is better to open in this case automatically the same action as a double click would trigger. WDYT?

ripcurlx · 2021-03-17T15:21:01Z

Also it would be good to show a (1) badge over the chat icon for the system message, so people are looking into it. In the past the chat was open by default in-line. So at least a notification would be good.

desktop/src/main/java/bisq/desktop/main/offer/takeoffer/TakeOfferDataModel.java

@chimp1984

apply @chimp1984 patch.txt code review suggestions taker tx check moved to trade step 2, after confirmation solve issue with calculating expected fees for unconfirmed tx resolve conflict with PR5207 (Disputes UI) check new offers after 1 block; check Json string not null; warn -> info remove unused parameter remove debugging log.warn message

ripcurlx

ACK - tested it on Regtest and Mainnet. So far everything is looking fine besides the minor issues that are fixed now.

ghost marked this pull request as draft February 14, 2021 04:51

ghost closed this Feb 17, 2021

ghost reopened this Feb 17, 2021

ghost changed the title ~~Verify maker fee transactions~~ Verify maker & taker fee transactions via Mempool lookup Feb 17, 2021

ghost mentioned this pull request Feb 28, 2021

For Cycle 22 bisq-network/compensation#791

Closed

ghost marked this pull request as ready for review March 12, 2021 23:55

chimp1984 suggested changes Mar 13, 2021

View reviewed changes

ripcurlx reviewed Mar 17, 2021

View reviewed changes

core/src/main/java/bisq/core/offer/TriggerPriceService.java Show resolved Hide resolved

ripcurlx reviewed Mar 17, 2021

View reviewed changes

desktop/src/main/java/bisq/desktop/main/offer/takeoffer/TakeOfferDataModel.java Outdated Show resolved Hide resolved

ripcurlx added this to the v1.6.0 milestone Mar 17, 2021

ripcurlx reviewed Mar 17, 2021

View reviewed changes

desktop/src/main/java/bisq/desktop/main/offer/takeoffer/TakeOfferDataModel.java Outdated Show resolved Hide resolved

jmacxx added 2 commits March 17, 2021 16:23

Merge branch 'master' into mempool_check_maker_tx

47ad421

ripcurlx approved these changes Mar 17, 2021

View reviewed changes

ripcurlx merged commit 87d8f5f into bisq-network:master Mar 17, 2021

ghost mentioned this pull request Mar 24, 2021

Review changes for Disputes GUI #5349

Merged

ghost mentioned this pull request Apr 2, 2021

For Cycle 23 bisq-network/compensation#814

Closed

ripcurlx mentioned this pull request Apr 5, 2021

For Cycle 23 bisq-network/compensation#813

Closed

ripcurlx mentioned this pull request May 10, 2021

For Cycle 24 bisq-network/compensation#846

Closed

ghost mentioned this pull request Sep 17, 2021

L1 Support Agent bisq-network/roles#64

Open

JeremyRand mentioned this pull request Apr 2, 2022

"Deactivating open offer due to mempool validation" does not produce any GUI explanation #6127

Closed

ghost deleted the mempool_check_maker_tx branch May 29, 2022 22:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify maker & taker fee transactions via Mempool lookup #5160

Verify maker & taker fee transactions via Mempool lookup #5160

ghost commented Feb 6, 2021 •

edited by ghost

Loading

Conza88 commented Feb 6, 2021

ripcurlx commented Feb 10, 2021

Conza88 commented Feb 17, 2021

chimp1984 Mar 13, 2021

chimp1984 Mar 13, 2021

ghost commented Mar 14, 2021

ripcurlx commented Mar 17, 2021

ripcurlx commented Mar 17, 2021 •

edited

Loading

ripcurlx commented Mar 17, 2021 •

edited

Loading

ripcurlx commented Mar 17, 2021

ripcurlx commented Mar 17, 2021

ripcurlx left a comment

Verify maker & taker fee transactions via Mempool lookup #5160

Verify maker & taker fee transactions via Mempool lookup #5160

Conversation

ghost commented Feb 6, 2021 • edited by ghost Loading

PR Checklist:

Conza88 commented Feb 6, 2021

ripcurlx commented Feb 10, 2021

Conza88 commented Feb 17, 2021

chimp1984 Mar 13, 2021

Choose a reason for hiding this comment

chimp1984 Mar 13, 2021

Choose a reason for hiding this comment

ghost commented Mar 14, 2021

ripcurlx commented Mar 17, 2021

ripcurlx commented Mar 17, 2021 • edited Loading

ripcurlx commented Mar 17, 2021 • edited Loading

ripcurlx commented Mar 17, 2021

ripcurlx commented Mar 17, 2021

ripcurlx left a comment

Choose a reason for hiding this comment

ghost commented Feb 6, 2021 •

edited by ghost

Loading

ripcurlx commented Mar 17, 2021 •

edited

Loading

ripcurlx commented Mar 17, 2021 •

edited

Loading