From 97f761bdf8714802c19742bebef0a01f204f7175 Mon Sep 17 00:00:00 2001
From: Steven Barclay <stejbac@gmail.com>
Date: Sat, 28 Sep 2024 23:43:43 +0800
Subject: [PATCH] Make Transactions view tolerate missing tx witness data

Make the filtering methods 'isPossible(RedirectOrClaimTx|EscrowSpend)',
used to speed up the Transactions view, lenient towards segwit txs that
have missing witness data. This appears to be the case for a lot of txs
fetched from the network by bitcoinj, including all the segwit txs in
the wallet after an SPV resync.

Since the witness data of a peer's claim tx (picked up by bitcoinj) may
in fact be missing, rename the field 'TradingPeer.signedClaimTx' to
'claimTx', along with corresponding proto field.

Finally, make sure the correct details message is shown for refund agent
payout txs, for v5 protocol trades in the Transactions view, by changing
the order of some of the nested if-else branches, in order to avoid an
unwanted '!trade.hasV5Protocol()' clause.
---
 .../bisq/core/trade/model/bisq_v1/Trade.java  |  4 +--
 .../protocol/bisq_v1/model/ProcessModel.java  |  2 +-
 .../protocol/bisq_v1/model/TradingPeer.java   |  6 ++---
 .../bisq_v5/tasks/SetupStagedTxListeners.java |  2 +-
 .../TransactionAwareTradable.java             |  9 +++++--
 .../transactions/TransactionsListItem.java    | 26 +++++++++----------
 proto/src/main/proto/pb.proto                 |  2 +-
 7 files changed, 28 insertions(+), 23 deletions(-)

diff --git a/core/src/main/java/bisq/core/trade/model/bisq_v1/Trade.java b/core/src/main/java/bisq/core/trade/model/bisq_v1/Trade.java
index a25e4ee809..3ec00753b5 100644
--- a/core/src/main/java/bisq/core/trade/model/bisq_v1/Trade.java
+++ b/core/src/main/java/bisq/core/trade/model/bisq_v1/Trade.java
@@ -720,8 +720,8 @@ public Transaction getClaimTx(BtcWalletService btcWalletService) {
 
     @Nullable
     public Transaction getPeersClaimTx(BtcWalletService btcWalletService) {
-        byte[] signedClaimTx = processModel.getTradePeer().getSignedClaimTx();
-        return signedClaimTx != null ? btcWalletService.getTxFromSerializedTx(signedClaimTx) : null;
+        byte[] claimTx = processModel.getTradePeer().getClaimTx();
+        return claimTx != null ? btcWalletService.getTxFromSerializedTx(claimTx) : null;
     }
 
     public List<Script> getWatchedScripts(BtcWalletService btcWalletService) {
diff --git a/core/src/main/java/bisq/core/trade/protocol/bisq_v1/model/ProcessModel.java b/core/src/main/java/bisq/core/trade/protocol/bisq_v1/model/ProcessModel.java
index 42c7253adc..7145b245f5 100644
--- a/core/src/main/java/bisq/core/trade/protocol/bisq_v1/model/ProcessModel.java
+++ b/core/src/main/java/bisq/core/trade/protocol/bisq_v1/model/ProcessModel.java
@@ -402,7 +402,7 @@ public boolean maybeClearSensitiveData(boolean keepStagedTxs) {
             if (keepStagedTxs) {
                 newTradingPeer.setFinalizedWarningTx(tradingPeer.getFinalizedWarningTx());
                 newTradingPeer.setFinalizedRedirectTx(tradingPeer.getFinalizedRedirectTx());
-                newTradingPeer.setSignedClaimTx(tradingPeer.getSignedClaimTx());
+                newTradingPeer.setClaimTx(tradingPeer.getClaimTx());
             }
             this.tradingPeer = newTradingPeer;
             changed = true;
diff --git a/core/src/main/java/bisq/core/trade/protocol/bisq_v1/model/TradingPeer.java b/core/src/main/java/bisq/core/trade/protocol/bisq_v1/model/TradingPeer.java
index 6d8e54be1b..ac600bb8f1 100644
--- a/core/src/main/java/bisq/core/trade/protocol/bisq_v1/model/TradingPeer.java
+++ b/core/src/main/java/bisq/core/trade/protocol/bisq_v1/model/TradingPeer.java
@@ -68,7 +68,7 @@ public final class TradingPeer implements TradePeer {
     private byte[] finalizedRedirectTx;
 
     @Nullable
-    private byte[] signedClaimTx;
+    private byte[] claimTx;
 
 
     // Transient/Mutable
@@ -142,7 +142,7 @@ public Message toProtoMessage() {
         Optional.ofNullable(hashOfPaymentAccountPayload).ifPresent(e -> builder.setHashOfPaymentAccountPayload(ByteString.copyFrom(e)));
         Optional.ofNullable(finalizedWarningTx).ifPresent(e -> builder.setFinalizedWarningTx(ByteString.copyFrom(e)));
         Optional.ofNullable(finalizedRedirectTx).ifPresent(e -> builder.setFinalizedRedirectTx(ByteString.copyFrom(e)));
-        Optional.ofNullable(signedClaimTx).ifPresent(e -> builder.setFinalizedRedirectTx(ByteString.copyFrom(e)));
+        Optional.ofNullable(claimTx).ifPresent(e -> builder.setClaimTx(ByteString.copyFrom(e)));
         builder.setCurrentDate(currentDate);
         return builder.build();
     }
@@ -174,7 +174,7 @@ public static TradingPeer fromProto(protobuf.TradingPeer proto, CoreProtoResolve
             tradingPeer.setHashOfPaymentAccountPayload(ProtoUtil.byteArrayOrNullFromProto(proto.getHashOfPaymentAccountPayload()));
             tradingPeer.setFinalizedWarningTx(ProtoUtil.byteArrayOrNullFromProto(proto.getFinalizedWarningTx()));
             tradingPeer.setFinalizedRedirectTx(ProtoUtil.byteArrayOrNullFromProto(proto.getFinalizedRedirectTx()));
-            tradingPeer.setSignedClaimTx(ProtoUtil.byteArrayOrNullFromProto(proto.getSignedClaimTx()));
+            tradingPeer.setClaimTx(ProtoUtil.byteArrayOrNullFromProto(proto.getClaimTx()));
             tradingPeer.setCurrentDate(proto.getCurrentDate());
             return tradingPeer;
         }
diff --git a/core/src/main/java/bisq/core/trade/protocol/bisq_v5/tasks/SetupStagedTxListeners.java b/core/src/main/java/bisq/core/trade/protocol/bisq_v5/tasks/SetupStagedTxListeners.java
index c656d34520..51bde1c671 100644
--- a/core/src/main/java/bisq/core/trade/protocol/bisq_v5/tasks/SetupStagedTxListeners.java
+++ b/core/src/main/java/bisq/core/trade/protocol/bisq_v5/tasks/SetupStagedTxListeners.java
@@ -136,7 +136,7 @@ private void applyRedirectOrClaimConfidence(TransactionConfidence confidence) {
                 newDisputeState = isMine ? Trade.DisputeState.ESCROW_CLAIMED : Trade.DisputeState.ESCROW_CLAIMED_BY_PEER;
                 if (!isMine) {
                     // Set the peer's claim tx, so that it shows up in the details window for past trades.
-                    processModel.getTradePeer().setSignedClaimTx(redirectOrClaimTx.bitcoinSerialize());
+                    processModel.getTradePeer().setClaimTx(redirectOrClaimTx.bitcoinSerialize());
                 }
             } else {
                 Transaction myRedirectTx = walletService.getTxFromSerializedTx(processModel.getFinalizedRedirectTx());
diff --git a/desktop/src/main/java/bisq/desktop/main/funds/transactions/TransactionAwareTradable.java b/desktop/src/main/java/bisq/desktop/main/funds/transactions/TransactionAwareTradable.java
index 73d54cbc78..c5497fb67b 100644
--- a/desktop/src/main/java/bisq/desktop/main/funds/transactions/TransactionAwareTradable.java
+++ b/desktop/src/main/java/bisq/desktop/main/funds/transactions/TransactionAwareTradable.java
@@ -54,7 +54,11 @@ static int bucketIndex(@Nullable String txId) {
     }
 
     static boolean isPossibleRedirectOrClaimTx(Transaction tx) {
-        return tx.getInput(0).getWitness().getPushCount() == 5 || tx.hasRelativeLockTime();
+        // The txs bitcoinj retrieves from the network frequently have missing witness data, so we must be lenient in
+        // that case (hence the last possibility checked for here), and similarly in the method below.
+        TransactionInput input = tx.getInput(0);
+        return input.getWitness().getPushCount() == 5 || tx.hasRelativeLockTime() || (tx.getVersion() == 1 &&
+                !input.hasWitness() && input.getScriptBytes().length == 0);
     }
 
     static boolean isPossibleEscrowSpend(TransactionInput input) {
@@ -62,6 +66,7 @@ static boolean isPossibleEscrowSpend(TransactionInput input) {
         // multisig P2SH will always be longer than that. P2PKH, P2SH-P2WPKH and P2WPKH have a witness push count less
         // than 3, but all Segwit trade escrow spends have a witness push count of at least 3. So we catch all escrow
         // spends this way, without too many false positives.
-        return input.getScriptBytes().length > 107 || input.getWitness().getPushCount() > 2;
+        return input.getScriptBytes().length > 107 || input.getWitness().getPushCount() > 2 || (!input.hasWitness() &&
+                input.getScriptBytes().length == 0);
     }
 }
diff --git a/desktop/src/main/java/bisq/desktop/main/funds/transactions/TransactionsListItem.java b/desktop/src/main/java/bisq/desktop/main/funds/transactions/TransactionsListItem.java
index b1dce72bb3..95c9a55956 100644
--- a/desktop/src/main/java/bisq/desktop/main/funds/transactions/TransactionsListItem.java
+++ b/desktop/src/main/java/bisq/desktop/main/funds/transactions/TransactionsListItem.java
@@ -214,6 +214,16 @@ else if (txTypeOptional.get().equals(TxType.REIMBURSEMENT_REQUEST))
                         if (amountAsCoin.isZero()) {
                             initialTxConfidenceVisibility = false;
                         }
+                    } else if (transactionAwareTrade.isWarningTx(txId)) {
+                        details = valueSentToMe.isPositive()
+                                ? Res.get("funds.tx.warningTx")
+                                : Res.get("funds.tx.peersWarningTx");
+                    } else if (transactionAwareTrade.isRedirectTx(txId)) {
+                        details = Res.get("funds.tx.collateralForRefund");
+                    } else if (transactionAwareTrade.isClaimTx(txId)) {
+                        details = valueSentToMe.isPositive()
+                                ? Res.get("funds.tx.claimTx")
+                                : Res.get("funds.tx.peersClaimTx");
                     } else {
                         Trade.DisputeState disputeState = trade.getDisputeState();
                         if (disputeState == Trade.DisputeState.DISPUTE_CLOSED) {
@@ -223,9 +233,9 @@ else if (txTypeOptional.get().equals(TxType.REIMBURSEMENT_REQUEST))
                                 details = Res.get("funds.tx.disputeLost");
                                 initialTxConfidenceVisibility = false;
                             }
-                        } else if ((disputeState == Trade.DisputeState.REFUND_REQUEST_CLOSED ||
+                        } else if (disputeState == Trade.DisputeState.REFUND_REQUEST_CLOSED ||
                                 disputeState == Trade.DisputeState.REFUND_REQUESTED ||
-                                disputeState == Trade.DisputeState.REFUND_REQUEST_STARTED_BY_PEER) && !trade.hasV5Protocol()) {
+                                disputeState == Trade.DisputeState.REFUND_REQUEST_STARTED_BY_PEER) {
                             if (valueSentToMe.isPositive()) { // FIXME: This will give a false positive if user is a BM.
                                 details = Res.get("funds.tx.refund");
                             } else {
@@ -239,17 +249,7 @@ else if (txTypeOptional.get().equals(TxType.REIMBURSEMENT_REQUEST))
                                 initialTxConfidenceVisibility = false;
                             }
                         } else {
-                            if (transactionAwareTrade.isWarningTx(txId)) {
-                                details = valueSentToMe.isPositive()
-                                        ? Res.get("funds.tx.warningTx")
-                                        : Res.get("funds.tx.peersWarningTx");
-                            } else if (transactionAwareTrade.isRedirectTx(txId)) {
-                                details = Res.get("funds.tx.collateralForRefund");
-                            } else if (transactionAwareTrade.isClaimTx(txId)) {
-                                details = valueSentToMe.isPositive()
-                                        ? Res.get("funds.tx.claimTx")
-                                        : Res.get("funds.tx.peersClaimTx");
-                            } else if (transactionAwareTrade.isDelayedPayoutTx(txId)) {
+                            if (transactionAwareTrade.isDelayedPayoutTx(txId)) {
                                 details = Res.get("funds.tx.timeLockedPayoutTx");
                                 initialTxConfidenceVisibility = false;
                             } else {
diff --git a/proto/src/main/proto/pb.proto b/proto/src/main/proto/pb.proto
index 3408945d55..26ebbf75c4 100644
--- a/proto/src/main/proto/pb.proto
+++ b/proto/src/main/proto/pb.proto
@@ -1928,7 +1928,7 @@ message TradingPeer {
     bytes hash_of_payment_account_payload = 16;
     bytes finalized_warning_tx = 17;  // Added for v5 protocol
     bytes finalized_redirect_tx = 18;  // Added for v5 protocol
-    bytes signed_claim_tx = 19;  // Added for v5 protocol
+    bytes claim_tx = 19;  // Added for v5 protocol
 }
 
 message BsqSwapProtocolModel {