Skip to content

Commit

Permalink
Make Transactions view tolerate missing tx witness data
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@stejbac
stejbac committed Sep 28, 2024
1 parent d5ce7bc commit 97f761b
Show file tree
Hide file tree
Showing 7 changed files with 28 additions and 23 deletions.
4 changes: 2 additions & 2 deletions core/src/main/java/bisq/core/trade/model/bisq_v1/Trade.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/bisq/core/trade/protocol/bisq_v1/model/ProcessModel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
6 changes: 3 additions & 3 deletions core/src/main/java/bisq/core/trade/protocol/bisq_v1/model/TradingPeer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public final class TradingPeer implements TradePeer {
private byte[] finalizedRedirectTx;

@Nullable
private byte[] signedClaimTx;
private byte[] claimTx;


// Transient/Mutable
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/bisq/core/trade/protocol/bisq_v5/tasks/SetupStagedTxListeners.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
9 changes: 7 additions & 2 deletions desktop/src/main/java/bisq/desktop/main/funds/transactions/TransactionAwareTradable.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,19 @@ 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) {
// The maximum ScriptSig length of a (canonically signed) P2PKH or P2SH-P2WH input is 107 bytes, whereas
// 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);
}
}
26 changes: 13 additions & 13 deletions desktop/src/main/java/bisq/desktop/main/funds/transactions/TransactionsListItem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion proto/src/main/proto/pb.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 97f761b

Please sign in to comment.