[bisq-network/bisq] Verify maker & taker fee transactions via Mempool lookup (#5160)

[chimp1984]

@chimp1984 requested changes on this pull request.



> +        jsonVin = getVinAndVout(jsonTxt).first;
+        jsonVout = getVinAndVout(jsonTxt).second;
+        JsonObject jsonVin0 = jsonVin.get(0).getAsJsonObject();
+        JsonObject jsonVout0 = jsonVout.get(0).getAsJsonObject();
+        JsonElement jsonVIn0Value = jsonVin0.getAsJsonObject("prevout").get("value");
+        JsonElement jsonFeeValue = jsonVout0.get("value");
+        if (jsonVIn0Value == null || jsonFeeValue == null) {
+            throw new JsonSyntaxException("vin/vout missing data");
+        }
+        Coin expectedFee = isMaker ?
+                getMakerFeeHistorical(false, tradeAmount, blockHeight) :
+                getTakerFeeHistorical(false, tradeAmount, blockHeight);
+        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. 

> +            String error = "UNDERPAID. " + description;
+            errorList.add(error);
+            log.warn(error);
+        }
+        return false;
+    }
+
+    private boolean checkFeeAmountBSQ(String jsonTxt, Coin tradeAmount, boolean isMaker, long blockHeight) {
+        JsonArray jsonVin;
+        JsonArray jsonVout;
+        jsonVin = getVinAndVout(jsonTxt).first;
+        jsonVout = getVinAndVout(jsonTxt).second;
+        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.... 

> @@ -355,6 +355,27 @@ public static void validatePayoutTxInput(Transaction depositTx,
         }
     }
 
+    public static void validateDepositInputs(Trade trade) throws InvalidTxException {
+        // assumption: deposit tx always has 2 inputs, the maker and taker

Correct. 

> @@ -355,6 +355,27 @@ public static void validatePayoutTxInput(Transaction depositTx,
         }
     }
 
+    public static void validateDepositInputs(Trade trade) throws InvalidTxException {
+        // assumption: deposit tx always has 2 inputs, the maker and taker
+        if (trade == null || trade.getDepositTx() == null || trade.getDepositTx().getInputs().size() != 2) {
+            throw new InvalidTxException("Deposit transaction is null or has unexpected input count");
+        }
+        Transaction depositTx = trade.getDepositTx();
+        String txIdInput0 = depositTx.getInput(0).getOutpoint().getHash().toString();
+        String txIdInput1 = depositTx.getInput(1).getOutpoint().getHash().toString();
+        String contractMakerTxId = trade.getContract().getOfferPayload().getOfferFeePaymentTxId();
+        String contractTakerTxId = trade.getContract().getTakerFeeTxID();
+        boolean makerFirstMatch = contractMakerTxId.equalsIgnoreCase(txIdInput0) && contractTakerTxId.equalsIgnoreCase(txIdInput1);
+        boolean takerFirstMatch = contractMakerTxId.equalsIgnoreCase(txIdInput1) && contractTakerTxId.equalsIgnoreCase(txIdInput0);
+        if (!makerFirstMatch && !takerFirstMatch) {

The order is determined by buyer/seller role (not maker/taker) but your check here is sufficient...

> +import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.MoreExecutors;
+import com.google.common.util.concurrent.SettableFuture;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import java.util.function.Consumer;
+
+import lombok.Getter;
+import lombok.extern.slf4j.Slf4j;
+
+import javax.annotation.Nullable;
+
+ at Slf4j
+public class MempoolService {

I assume it should be a @Singleton . At the moment we get created 2 instances.

> +    private int outstandingRequests = 0;
+
+    @Inject
+    public MempoolService(Socks5ProxyProvider socks5ProxyProvider,
+                          Config config,
+                          Preferences preferences,
+                          FilterManager filterManager,
+                          DaoFacade daoFacade,
+                          DaoStateService daoStateService) {
+        this.socks5ProxyProvider = socks5ProxyProvider;
+        this.config = config;
+        this.preferences = preferences;
+        this.filterManager = filterManager;
+        this.daoFacade = daoFacade;
+        this.daoStateService = daoStateService;
+        this.btcFeeReceivers = getAllBtcFeeReceivers();

As we rely here on the daostate we need to fill btcFeeReceivers via a onAllServicesInitialized method.

> +        mempoolRequest.getTxStatus(future, txValidator.getTxId(), null);
+    }
+
+    private void retryValidateOfferTakerTx(MempoolRequest mempoolRequest, TxValidator txValidator, Consumer<TxValidator> resultHandler) {
+        SettableFuture<String> future = SettableFuture.create();
+        Futures.addCallback(future, callbackForTakerTxValidation(mempoolRequest, txValidator, resultHandler), MoreExecutors.directExecutor());
+        mempoolRequest.getTxStatus(future, txValidator.getTxId(), null);
+    }
+
+    private FutureCallback<String> callbackForMakerTxValidation(MempoolRequest theRequest, TxValidator txValidator, Consumer<TxValidator> resultHandler) {
+        outstandingRequests++;
+        FutureCallback<String> myCallback = new FutureCallback<>() {
+            @Override
+            public void onSuccess(@Nullable String jsonTxt) {
+                outstandingRequests--;
+                UserThread.execute(() -> {

As we get called from another thread here its more safe to move the outstandingRequests update into the userthread mapped runnable as well.

> +        FutureCallback<String> myCallback = new FutureCallback<>() {
+            @Override
+            public void onSuccess(@Nullable String jsonTxt) {
+                outstandingRequests--;
+                UserThread.execute(() -> {
+                    resultHandler.accept(txValidator.parseJsonValidateMakerFeeTx(jsonTxt, btcFeeReceivers));
+                });
+            }
+            @Override
+            public void onFailure(Throwable throwable) {
+                outstandingRequests--;
+                log.warn("onFailure - {}", throwable.toString());
+                if (theRequest.switchToAnotherProvider()) {
+                    retryValidateOfferMakerTx(theRequest, txValidator, resultHandler);
+                } else {
+                    UserThread.execute(() -> {

I think here as well its more safe to move all code into the runnable to avoid potential threading issues.

> @@ -898,6 +899,21 @@ private void addButtons() {
         });
     }
 
+    private void nextStepCheckMakerTx() {
+        // the tx validation check has had plenty of time to complete, but if for some reason it has not returned
+        // we continue anyway since the check is not crucial.
+        // note, it would be great if there was a real tri-state boolean we could use here, instead of -1, 0, and 1

Using Optional<Boolean> would be an option to handle that. Another to use boxed Boolean type and check for null but Optional is more clear IMO.

> @@ -462,6 +463,7 @@ private void updateButtonDisableState() {
         boolean inputDataValid = isBtcInputValid(amount.get()).isValid
                 && dataModel.isMinAmountLessOrEqualAmount()
                 && !dataModel.isAmountLargerThanOfferAmount()
+                && dataModel.mempoolStatus.get() >= 0

Do we want to block in case response is slow (tor can be slow)?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/5160#pullrequestreview-611605723
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20210313/77c577da/attachment.htm>