verify payout & dispute payout tx fees are within range of recreated tx

core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java

@@ -698,6 +698,7 @@ public class TradeWalletService {
      * @throws TransactionVerificationException if there was an unexpected problem with the payout tx or its signatures
      * @throws WalletException if the seller's wallet is null or structurally inconsistent
      */
+    @Deprecated
     public Transaction sellerSignsAndFinalizesPayoutTx(Transaction depositTx,
                                                        byte[] buyerSignature,
                                                        Coin buyerPayoutAmount,

core/src/main/java/bisq/core/support/dispute/DisputeManager.java

@@ -701,7 +701,7 @@ public abstract class DisputeManager<T extends DisputeList<Dispute>> extends Sup
             if (trade == null) throw new RuntimeException("Dispute trade " + dispute.getTradeId() + " does not exist");
 
             // create dispute payout tx if not given
-            if (payoutTx == null) payoutTx = createDisputePayoutTx(trade, dispute, disputeResult); // can be null if already published or we don't have receiver's multisig hex
+            if (payoutTx == null) payoutTx = createDisputePayoutTx(trade, dispute, disputeResult, false); // can be null if already published or we don't have receiver's multisig hex
 
             // persist result in dispute's chat message
             ChatMessage chatMessage = new ChatMessage(
@@ -804,7 +804,7 @@ public abstract class DisputeManager<T extends DisputeList<Dispute>> extends Sup
     // Utils
     ///////////////////////////////////////////////////////////////////////////////////////////
 
-    public MoneroTxWallet createDisputePayoutTx(Trade trade, Dispute dispute, DisputeResult disputeResult) {
+    public MoneroTxWallet createDisputePayoutTx(Trade trade, Dispute dispute, DisputeResult disputeResult, boolean skipMultisigImport) {
 
         // sync and save wallet
         trade.syncWallet();
@@ -813,14 +813,16 @@ public abstract class DisputeManager<T extends DisputeList<Dispute>> extends Sup
         // create unsigned dispute payout tx if not already published and arbitrator has trader's updated multisig info
         TradingPeer receiver = trade.getTradingPeer(dispute.getTraderPubKeyRing());
         if (!trade.isPayoutPublished() && receiver.getUpdatedMultisigHex() != null) {
+            MoneroWallet multisigWallet = trade.getWallet();
 
             // import multisig hex
-            MoneroWallet multisigWallet = trade.getWallet();
+            if (!skipMultisigImport) {
-            List<String> updatedMultisigHexes = new ArrayList<String>();
+                List<String> updatedMultisigHexes = new ArrayList<String>();
-            if (trade.getBuyer().getUpdatedMultisigHex() != null) updatedMultisigHexes.add(trade.getBuyer().getUpdatedMultisigHex());
+                if (trade.getBuyer().getUpdatedMultisigHex() != null) updatedMultisigHexes.add(trade.getBuyer().getUpdatedMultisigHex());
-            if (trade.getSeller().getUpdatedMultisigHex() != null) updatedMultisigHexes.add(trade.getSeller().getUpdatedMultisigHex());
+                if (trade.getSeller().getUpdatedMultisigHex() != null) updatedMultisigHexes.add(trade.getSeller().getUpdatedMultisigHex());
-            if (!updatedMultisigHexes.isEmpty()) {
+                if (!updatedMultisigHexes.isEmpty()) {
-                multisigWallet.importMultisigHex(updatedMultisigHexes.toArray(new String[0])); // TODO (monero-project): fails if multisig hex imported individually
+                    multisigWallet.importMultisigHex(updatedMultisigHexes.toArray(new String[0])); // TODO (monero-project): fails if multisig hex imported individually
+                }
             }
 
             // sync and save wallet

core/src/main/java/bisq/core/support/dispute/arbitration/ArbitrationManager.java

@@ -329,8 +329,6 @@ public final class ArbitrationManager extends DisputeManager<ArbitrationDisputeL
         BigInteger destinationSum = (buyerPayoutDestination == null ? BigInteger.ZERO : buyerPayoutDestination.getAmount()).add(sellerPayoutDestination == null ? BigInteger.ZERO : sellerPayoutDestination.getAmount());
         if (!arbitratorSignedPayoutTx.getOutputSum().equals(destinationSum.add(arbitratorSignedPayoutTx.getChangeAmount()))) throw new RuntimeException("Sum of outputs != destination amounts + change amount");
 
-        // TODO: verify miner fee is within expected range
-
         // verify winner and loser payout amounts
         BigInteger txCost = arbitratorSignedPayoutTx.getFee().add(arbitratorSignedPayoutTx.getChangeAmount()); // fee + lost dust change
         BigInteger expectedWinnerAmount = HavenoUtils.coinToAtomicUnits(disputeResult.getWinner() == Winner.BUYER ? disputeResult.getBuyerPayoutAmount() : disputeResult.getSellerPayoutAmount());
@@ -348,6 +346,21 @@ public final class ArbitrationManager extends DisputeManager<ArbitrationDisputeL
         String signedMultisigTxHex = result.getSignedMultisigTxHex();
         signedTxSet.setMultisigTxHex(signedMultisigTxHex);
 
+        // verify mining fee is within tolerance by recreating payout tx
+        // TODO (monero-project): creating tx will require exchanging updated multisig hex if message needs reprocessed. provide weight with describe_transfer so fee can be estimated?
+        MoneroTxWallet feeEstimateTx = null;
+        try {
+            feeEstimateTx = createDisputePayoutTx(trade, dispute, disputeResult, true);
+        } catch (Exception e) {
+            log.warn("Could not recreate dispute payout tx to verify fee: " + e.getMessage());
+        }
+        if (feeEstimateTx != null) {
+            BigInteger feeEstimate = feeEstimateTx.getFee();
+            double feeDiff = arbitratorSignedPayoutTx.getFee().subtract(feeEstimate).abs().doubleValue() / feeEstimate.doubleValue(); // TODO: use BigDecimal?
+            if (feeDiff > XmrWalletService.MINER_FEE_TOLERANCE) throw new RuntimeException("Miner fee is not within " + (XmrWalletService.MINER_FEE_TOLERANCE * 100) + "% of estimated fee, expected " + feeEstimate + " but was " + arbitratorSignedPayoutTx.getFee());
+            log.info("Payout tx fee {} is within tolerance, diff %={}", arbitratorSignedPayoutTx.getFee(), feeDiff);
+        }
+
         // submit fully signed payout tx to the network
         List<String> txHashes = multisigWallet.submitMultisigTxHex(signedTxSet.getMultisigTxHex());
         signedTxSet.getTxs().get(0).setHash(txHashes.get(0)); // manually update hash which is known after signed

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

@@ -716,7 +716,6 @@ public abstract class Trade implements Tradable, Model {
         BigInteger sellerPayoutAmount = sellerDepositAmount.subtract(tradeAmount);
 
         // create transaction to get fee estimate
-        if (multisigWallet.isMultisigImportNeeded()) throw new RuntimeException("Cannot create payout tx because multisig import is needed");
         MoneroTxWallet feeEstimateTx = multisigWallet.createTx(new MoneroTxConfig()
                 .setAccountIndex(0)
                 .addDestination(buyerPayoutAddress, buyerPayoutAmount.multiply(BigInteger.valueOf(9)).divide(BigInteger.valueOf(10))) // reduce payment amount to compute fee of similar tx
@@ -798,17 +797,34 @@ public abstract class Trade implements Tradable, Model {
         BigInteger expectedSellerPayout = sellerDepositAmount.subtract(tradeAmount).subtract(txCost.divide(BigInteger.valueOf(2)));
         if (!sellerPayoutDestination.getAmount().equals(expectedSellerPayout)) throw new RuntimeException("Seller destination amount is not deposit amount - trade amount - 1/2 tx costs, " + sellerPayoutDestination.getAmount() + " vs " + expectedSellerPayout);
 
-        // TODO (woodser): verify fee is reasonable (e.g. within 2x of fee estimate tx)
+        // handle tx signing
-
-        // sign payout tx
         if (sign) {
+
+            // sign tx
             MoneroMultisigSignResult result = multisigWallet.signMultisigTxHex(payoutTxHex);
             if (result.getSignedMultisigTxHex() == null) throw new RuntimeException("Error signing payout tx");
             payoutTxHex = result.getSignedMultisigTxHex();
+            describedTxSet = multisigWallet.describeMultisigTxSet(payoutTxHex); // update described set
+            payoutTx = describedTxSet.getTxs().get(0);
+
+            // verify fee is within tolerance by recreating payout tx
+            // TODO (monero-project): creating tx will require exchanging updated multisig hex if message needs reprocessed. provide weight with describe_transfer so fee can be estimated?
+            MoneroTxWallet feeEstimateTx = null;
+            try {
+                feeEstimateTx = createPayoutTx();
+            } catch (Exception e) {
+                log.warn("Could not recreate payout tx to verify fee: " + e.getMessage());
+            }
+            if (feeEstimateTx != null) {
+                BigInteger feeEstimate = feeEstimateTx.getFee();
+                double feeDiff = payoutTx.getFee().subtract(feeEstimate).abs().doubleValue() / feeEstimate.doubleValue(); // TODO: use BigDecimal?
+                if (feeDiff > XmrWalletService.MINER_FEE_TOLERANCE) throw new RuntimeException("Miner fee is not within " + (XmrWalletService.MINER_FEE_TOLERANCE * 100) + "% of estimated fee, expected " + feeEstimate + " but was " + payoutTx.getFee());
+                log.info("Payout tx fee {} is within tolerance, diff %={}", payoutTx.getFee(), feeDiff);
+            }
         }
 
         // update trade state
-        setPayoutTx(describedTxSet.getTxs().get(0));
+        setPayoutTx(payoutTx);
         setPayoutTxHex(payoutTxHex);
 
         // submit payout tx

desktop/src/main/java/bisq/desktop/main/overlays/windows/DisputeSummaryWindow.java

@@ -583,7 +583,7 @@ public class DisputeSummaryWindow extends Overlay<DisputeSummaryWindow> {
         closeTicketButton.setOnAction(e -> {
 
             // create payout tx
-            MoneroTxWallet payoutTx = arbitrationManager.createDisputePayoutTx(trade, dispute, disputeResult);
+            MoneroTxWallet payoutTx = arbitrationManager.createDisputePayoutTx(trade, dispute, disputeResult, false);
 
             // show confirmation
             if (dispute.getSupportType() == SupportType.ARBITRATION &&