diff --git a/src/main/java/bisq/core/btc/wallet/TxBroadcastTimeoutException.java b/src/main/java/bisq/core/btc/wallet/TxBroadcastTimeoutException.java index 4c710a6b..0f6c1635 100644 --- a/src/main/java/bisq/core/btc/wallet/TxBroadcastTimeoutException.java +++ b/src/main/java/bisq/core/btc/wallet/TxBroadcastTimeoutException.java @@ -18,6 +18,7 @@ package bisq.core.btc.wallet; import org.bitcoinj.core.Transaction; +import org.bitcoinj.wallet.Wallet; import lombok.Getter; @@ -29,13 +30,22 @@ public class TxBroadcastTimeoutException extends TxBroadcastException { @Nullable private final Transaction localTx; @Getter - private int delay; - - public TxBroadcastTimeoutException(Transaction localTx, int delay) { + private final int delay; + @Getter + private final Wallet wallet; + + /** + * + * @param localTx The tx we sent out + * @param delay The timeout delay + * @param wallet Wallet is needed if a client is calling wallet.commitTx(tx) + */ + public TxBroadcastTimeoutException(Transaction localTx, int delay, Wallet wallet) { super("The transaction was not broadcasted in " + delay + "seconds. txId=" + localTx.getHashAsString()); this.localTx = localTx; this.delay = delay; + this.wallet = wallet; } @Override diff --git a/src/main/java/bisq/core/btc/wallet/TxBroadcaster.java b/src/main/java/bisq/core/btc/wallet/TxBroadcaster.java index e9ba9e33..9bb426c2 100644 --- a/src/main/java/bisq/core/btc/wallet/TxBroadcaster.java +++ b/src/main/java/bisq/core/btc/wallet/TxBroadcaster.java @@ -42,8 +42,41 @@ public interface Callback { void onSuccess(Transaction transaction); default void onTimeout(TxBroadcastTimeoutException exception) { - log.error("TxBroadcaster.onTimeout " + exception.toString()); - onFailure(exception); + Transaction tx = exception.getLocalTx(); + if (tx != null) { + String txId = tx.getHashAsString(); + log.warn("TxBroadcaster.onTimeout called: {}\n" + + "We optimistically assume that the tx broadcast succeeds later and call onSuccess on the " + + "callback handler. This behaviour carries less potential problems than if we would trigger " + + "a failure (e.g. which would cause a failed create offer attempt of failed take offer attempt).\n" + + "We have no guarantee how long it will take to get the information that sufficiently BTC " + + "nodes have reported back to BitcoinJ that the tx is in their mempool.\n" + + "In normal situations " + + "that's very fast but in some cases it can take minutes (mostly related to Tor connection " + + "issues). So if we just go on in the application logic and treat it as successful and the " + + "tx will be broadcasted successfully later all is fine.\n" + + "If it will fail to get broadcasted, " + + "it will lead to a failure state, the same as if we would trigger a failure due the timeout." + + "So we can assume that this behaviour will lead to less problems as otherwise.\n" + + "Long term we should implement better monitoring for Tor and the provided Bitcoin nodes to " + + "find out why those delays happen and add some rollback behaviour to the app state in case " + + "the tx will never get broadcasted.", + exception.toString(), + txId, + exception.getDelay()); + + // The commit is required in case the tx is spent by a follow up tx as otherwise there would be an + // InsufficientMoneyException thrown. But in some test scenarios we also got issues with wallet + // inconsistency if the tx was committed twice. It should be prevented by the maybeCommitTx methods but + // not 100% if that is always the case. Just added that comment to make clear that this might be a risky + // strategy and might need improvement if we get problems. + exception.getWallet().maybeCommitTx(tx); + + onSuccess(tx); + } else { + log.error("TxBroadcaster.onTimeout: Tx is null. exception={} ", exception.toString()); + onFailure(exception); + } } default void onTxMalleability(TxMalleabilityException exception) { @@ -54,7 +87,7 @@ default void onTxMalleability(TxMalleabilityException exception) { void onFailure(TxBroadcastException exception); } - private static final int DEFAULT_BROADCAST_TIMEOUT = 8; + private static final int DEFAULT_BROADCAST_TIMEOUT = 20; private static Map broadcastTimerMap = new HashMap<>(); public static void broadcastTx(Wallet wallet, PeerGroup peerGroup, Transaction localTx, Callback callback) { @@ -66,15 +99,14 @@ public static void broadcastTx(Wallet wallet, PeerGroup peerGroup, Transaction t final String txId = tx.getHashAsString(); if (!broadcastTimerMap.containsKey(txId)) { timeoutTimer = UserThread.runAfter(() -> { - log.warn("Broadcast of tx {} not completed after {} sec. We optimistically assume that the tx " + - "broadcast succeeded and call onSuccess on the callback handler.", txId, delayInSec); - broadcastTimerMap.remove(txId); + log.warn("Broadcast of tx {} not completed after {} sec.", txId, delayInSec); stopAndRemoveTimer(txId); - UserThread.execute(() -> callback.onTimeout(new TxBroadcastTimeoutException(tx, delayInSec))); + UserThread.execute(() -> callback.onTimeout(new TxBroadcastTimeoutException(tx, delayInSec, wallet))); }, delayInSec); broadcastTimerMap.put(txId, timeoutTimer); } else { + // Would be due a wrong way how to use the API (calling 2 times a broadcast with same tx). stopAndRemoveTimer(txId); UserThread.execute(() -> callback.onFailure(new TxBroadcastException("We got broadcastTx called with a tx " + "which has an open timeoutTimer. txId=" + txId, txId))); @@ -94,8 +126,8 @@ public void onSuccess(@Nullable Transaction result) { UserThread.execute(() -> callback.onSuccess(tx)); } else { stopAndRemoveTimer(txId); - UserThread.execute(() -> callback.onFailure(new TxBroadcastException("We got an onSuccess callback for " + - "a broadcast which got already triggered the timeout.", txId))); + log.warn("We got an onSuccess callback for a broadcast which got already triggered " + + "the timeout.", txId); } } else { stopAndRemoveTimer(txId); diff --git a/src/main/java/bisq/core/dao/bonding/lockup/LockupService.java b/src/main/java/bisq/core/dao/bonding/lockup/LockupService.java index 35b49683..8187b5b9 100644 --- a/src/main/java/bisq/core/dao/bonding/lockup/LockupService.java +++ b/src/main/java/bisq/core/dao/bonding/lockup/LockupService.java @@ -22,7 +22,6 @@ import bisq.core.btc.wallet.BsqWalletService; import bisq.core.btc.wallet.BtcWalletService; import bisq.core.btc.wallet.TxBroadcastException; -import bisq.core.btc.wallet.TxBroadcastTimeoutException; import bisq.core.btc.wallet.TxBroadcaster; import bisq.core.btc.wallet.TxMalleabilityException; import bisq.core.btc.wallet.WalletsManager; @@ -83,11 +82,6 @@ public void onSuccess(Transaction transaction) { resultHandler.handleResult(); } - @Override - public void onTimeout(TxBroadcastTimeoutException exception) { - exceptionHandler.handleException(exception); - } - @Override public void onTxMalleability(TxMalleabilityException exception) { exceptionHandler.handleException(exception); diff --git a/src/main/java/bisq/core/dao/bonding/unlock/UnlockService.java b/src/main/java/bisq/core/dao/bonding/unlock/UnlockService.java index fe875149..1a8ae7d7 100644 --- a/src/main/java/bisq/core/dao/bonding/unlock/UnlockService.java +++ b/src/main/java/bisq/core/dao/bonding/unlock/UnlockService.java @@ -22,7 +22,6 @@ import bisq.core.btc.wallet.BsqWalletService; import bisq.core.btc.wallet.BtcWalletService; import bisq.core.btc.wallet.TxBroadcastException; -import bisq.core.btc.wallet.TxBroadcastTimeoutException; import bisq.core.btc.wallet.TxBroadcaster; import bisq.core.btc.wallet.TxMalleabilityException; import bisq.core.btc.wallet.WalletsManager; @@ -75,11 +74,6 @@ public void onSuccess(Transaction transaction) { resultHandler.handleResult(); } - @Override - public void onTimeout(TxBroadcastTimeoutException exception) { - exceptionHandler.handleException(exception); - } - @Override public void onTxMalleability(TxMalleabilityException exception) { exceptionHandler.handleException(exception); diff --git a/src/main/java/bisq/core/dao/governance/blindvote/MyBlindVoteListService.java b/src/main/java/bisq/core/dao/governance/blindvote/MyBlindVoteListService.java index 5d3337e0..cae19ed0 100644 --- a/src/main/java/bisq/core/dao/governance/blindvote/MyBlindVoteListService.java +++ b/src/main/java/bisq/core/dao/governance/blindvote/MyBlindVoteListService.java @@ -23,7 +23,6 @@ import bisq.core.btc.wallet.BsqWalletService; import bisq.core.btc.wallet.BtcWalletService; import bisq.core.btc.wallet.TxBroadcastException; -import bisq.core.btc.wallet.TxBroadcastTimeoutException; import bisq.core.btc.wallet.TxBroadcaster; import bisq.core.btc.wallet.TxMalleabilityException; import bisq.core.btc.wallet.WalletsManager; @@ -324,15 +323,6 @@ public void onSuccess(Transaction transaction) { resultHandler.handleResult(); } - @Override - public void onTimeout(TxBroadcastTimeoutException exception) { - // TODO handle - // We need to handle cases where a timeout happens and - // the tx might get broadcasted at a later restart! - // We need to be sure that in case of a failed tx the locked stake gets unlocked! - exceptionHandler.handleException(exception); - } - @Override public void onTxMalleability(TxMalleabilityException exception) { // TODO handle diff --git a/src/main/java/bisq/core/dao/governance/proposal/MyProposalListService.java b/src/main/java/bisq/core/dao/governance/proposal/MyProposalListService.java index cdc2b419..236c020b 100644 --- a/src/main/java/bisq/core/dao/governance/proposal/MyProposalListService.java +++ b/src/main/java/bisq/core/dao/governance/proposal/MyProposalListService.java @@ -19,7 +19,6 @@ import bisq.core.app.BisqEnvironment; import bisq.core.btc.wallet.TxBroadcastException; -import bisq.core.btc.wallet.TxBroadcastTimeoutException; import bisq.core.btc.wallet.TxBroadcaster; import bisq.core.btc.wallet.TxMalleabilityException; import bisq.core.btc.wallet.WalletsManager; @@ -153,12 +152,6 @@ public void onSuccess(Transaction transaction) { resultHandler.handleResult(); } - @Override - public void onTimeout(TxBroadcastTimeoutException exception) { - // TODO handle - errorMessageHandler.handleErrorMessage(exception.getMessage()); - } - @Override public void onTxMalleability(TxMalleabilityException exception) { // TODO handle diff --git a/src/main/java/bisq/core/dao/governance/votereveal/VoteRevealService.java b/src/main/java/bisq/core/dao/governance/votereveal/VoteRevealService.java index 69e8bbd9..bf42ff1a 100644 --- a/src/main/java/bisq/core/dao/governance/votereveal/VoteRevealService.java +++ b/src/main/java/bisq/core/dao/governance/votereveal/VoteRevealService.java @@ -22,7 +22,6 @@ import bisq.core.btc.wallet.BsqWalletService; import bisq.core.btc.wallet.BtcWalletService; import bisq.core.btc.wallet.TxBroadcastException; -import bisq.core.btc.wallet.TxBroadcastTimeoutException; import bisq.core.btc.wallet.TxBroadcaster; import bisq.core.btc.wallet.TxMalleabilityException; import bisq.core.btc.wallet.WalletsManager; @@ -240,14 +239,6 @@ public void onSuccess(Transaction transaction) { myVoteListService.applyRevealTxId(myVote, voteRevealTx.getHashAsString()); } - @Override - public void onTimeout(TxBroadcastTimeoutException exception) { - log.error(exception.toString()); - // TODO handle - voteRevealExceptions.add(new VoteRevealException("Publishing of voteRevealTx failed.", - exception, voteRevealTx)); - } - @Override public void onTxMalleability(TxMalleabilityException exception) { log.error(exception.toString()); diff --git a/src/main/java/bisq/core/trade/protocol/tasks/buyer_as_taker/BuyerAsTakerSignAndPublishDepositTx.java b/src/main/java/bisq/core/trade/protocol/tasks/buyer_as_taker/BuyerAsTakerSignAndPublishDepositTx.java index 409e69b2..490d5e5b 100644 --- a/src/main/java/bisq/core/trade/protocol/tasks/buyer_as_taker/BuyerAsTakerSignAndPublishDepositTx.java +++ b/src/main/java/bisq/core/trade/protocol/tasks/buyer_as_taker/BuyerAsTakerSignAndPublishDepositTx.java @@ -92,6 +92,7 @@ protected void run() { @Override public void onSuccess(Transaction transaction) { if (!completed) { + trade.setDepositTx(transaction); log.trace("takerSignsAndPublishesDepositTx succeeded " + transaction); trade.setState(Trade.State.TAKER_PUBLISHED_DEPOSIT_TX); walletService.swapTradeEntryToAvailableEntry(id, AddressEntry.Context.RESERVED_FOR_TRADE); @@ -111,8 +112,11 @@ public void onFailure(TxBroadcastException exception) { } } }); - // We set the deposit tx in case we get the onFailure called. - trade.setDepositTx(depositTx); + if (trade.getDepositTx() == null) { + // We set the deposit tx in case we get the onFailure called. We cannot set it in the onFailure + // callback as the tx is returned by the method call where the callback is used as an argument. + trade.setDepositTx(depositTx); + } } catch (Throwable t) { failed(t); } diff --git a/src/main/java/bisq/core/trade/protocol/tasks/seller_as_taker/SellerAsTakerSignAndPublishDepositTx.java b/src/main/java/bisq/core/trade/protocol/tasks/seller_as_taker/SellerAsTakerSignAndPublishDepositTx.java index 4dee799d..e0eeba15 100644 --- a/src/main/java/bisq/core/trade/protocol/tasks/seller_as_taker/SellerAsTakerSignAndPublishDepositTx.java +++ b/src/main/java/bisq/core/trade/protocol/tasks/seller_as_taker/SellerAsTakerSignAndPublishDepositTx.java @@ -93,6 +93,7 @@ protected void run() { @Override public void onSuccess(Transaction transaction) { if (!completed) { + trade.setDepositTx(transaction); log.trace("takerSignsAndPublishesDepositTx succeeded " + transaction); trade.setState(Trade.State.TAKER_PUBLISHED_DEPOSIT_TX); walletService.swapTradeEntryToAvailableEntry(id, AddressEntry.Context.RESERVED_FOR_TRADE); @@ -112,8 +113,11 @@ public void onFailure(TxBroadcastException exception) { } } }); - // We set the deposit tx in case we get the onFailure called. - trade.setDepositTx(depositTx); + if (trade.getDepositTx() == null) { + // We set the deposit tx in case we get the onFailure called. We cannot set it in the onFailure + // callback as the tx is returned by the method call where the callback is used as an argument. + trade.setDepositTx(depositTx); + } } catch (Throwable t) { final Contract contract = trade.getContract(); if (contract != null)