TxBroadcaster improvements

core/src/main/java/bisq/core/btc/exceptions/TxMalleabilityException.java

@@ -1,50 +0,0 @@
-/*
- * This file is part of Bisq.
- *
- * Bisq is free software: you can redistribute it and/or modify it
- * under the terms of the GNU Affero General Public License as published by
- * the Free Software Foundation, either version 3 of the License, or (at
- * your option) any later version.
- *
- * Bisq is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public
- * License for more details.
- *
- * You should have received a copy of the GNU Affero General Public License
- * along with Bisq. If not, see <http://www.gnu.org/licenses/>.
- */
-
-package bisq.core.btc.exceptions;
-
-import org.bitcoinj.core.Transaction;
-
-import lombok.Getter;
-
-import javax.annotation.Nullable;
-
-
-public class TxMalleabilityException extends TxBroadcastException {
-    @Getter
-    @Nullable
-    private final Transaction localTx;
-    @Getter
-    @Nullable
-    private final Transaction networkTx;
-
-    public TxMalleabilityException(Transaction localTx, Transaction networkTx) {
-        super("The transaction we received from the Bitcoin network has a different txId as the one we broadcasted.\n" +
-                "txId of local tx=" + localTx.getHashAsString() +
-                ", txId of received tx=" + localTx.getHashAsString());
-        this.localTx = localTx;
-        this.networkTx = networkTx;
-    }
-
-    @Override
-    public String toString() {
-        return "TxMalleabilityException{" +
-                "\n     localTx=" + localTx +
-                ",\n     networkTx=" + networkTx +
-                "\n} " + super.toString();
-    }
-}

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

@@ -19,7 +19,6 @@ package bisq.core.btc.wallet;
 
 import bisq.core.btc.exceptions.TxBroadcastException;
 import bisq.core.btc.exceptions.TxBroadcastTimeoutException;
-import bisq.core.btc.exceptions.TxMalleabilityException;
 
 import bisq.common.Timer;
 import bisq.common.UserThread;
@@ -48,35 +47,21 @@ public class TxBroadcaster {
         default void onTimeout(TxBroadcastTimeoutException exception) {
             Transaction tx = exception.getLocalTx();
             if (tx != null) {
-                log.warn("TxBroadcaster.onTimeout called: {} \n" +
+                // We optimistically assume that the tx broadcast succeeds later and call onSuccess on the callback handler.
-                                "We optimistically assume that the tx broadcast succeeds later and call onSuccess on the " +
+                // This behaviour carries less potential problems than if we would trigger a failure (e.g. which would cause
-                                "callback handler. This behaviour carries less potential problems than if we would trigger " +
+                // a failed create offer attempt or failed take offer attempt).
-                                "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 many BTC nodes have
-                                "We have no guarantee how long it will take to get the information that sufficiently many BTC " +
+                // reported back to BitcoinJ that the tx is in their mempool.
-                                "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
-                                "In normal situations " +
+                // connection issues). So if we just go on in the application logic and treat it as successful and the
-                                "that's very fast but in some cases it can take minutes (mostly related to Tor connection " +
+                // tx will be broadcast successfully later all is fine.
-                                "issues). So if we just go on in the application logic and treat it as successful and the " +
+                // If it will fail to get broadcast, it will lead to a failure state, the same as if we would trigger a
-                                "tx will be broadcast successfully later all is fine.\n" +
+                // failure due the timeout.
-                                "If it will fail to get broadcast, " +
+                // So we can assume that this behaviour will lead to less problems as otherwise.
-                                "it will lead to a failure state, the same as if we would trigger a failure due the timeout." +
+                // Long term we should implement better monitoring for Tor and the provided Bitcoin nodes to find out
-                                "So we can assume that this behaviour will lead to less problems as otherwise.\n" +
+                // why those delays happen and add some rollback behaviour to the app state in case the tx will never
-                                "Long term we should implement better monitoring for Tor and the provided Bitcoin nodes to " +
+                // get broadcast.
-                                "find out why those delays happen and add some rollback behaviour to the app state in case " +
+                log.warn("TxBroadcaster.onTimeout called: {}", exception.toString());
-                                "the tx will never get broadcast.",
-                        exception.toString());
-
-                // The wallet.maybeCommitTx() call 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.
-                // UPDATE: We got reported an wallet problem that a tx was added twice and wallet could not be loaded anymore even after a SPV resync.
-                // So it seems that this strategy is too risky to cause more problems as it tries to solve.
-                // Need more work from a BitcoinJ expert! For now we comment the call out here but leave it as reference
-                // for future improvements.
-                // exception.getWallet().maybeCommitTx(tx);
-
                 onSuccess(tx);
             } else {
                 log.error("TxBroadcaster.onTimeout: Tx is null. exception={} ", exception.toString());
@@ -84,11 +69,6 @@ public class TxBroadcaster {
             }
         }
 
-        default void onTxMalleability(TxMalleabilityException exception) {
-            log.error("onTxMalleability.onTimeout " + exception.toString());
-            onFailure(exception);
-        }
-
         void onFailure(TxBroadcastException exception);
     }
 
@@ -118,31 +98,25 @@ public class TxBroadcaster {
                     "which has an open timeoutTimer. txId=" + txId, txId)));
         }
 
+        // We decided the least risky scenario is to commit the tx to the wallet and broadcast it later.
+        // If it's a bsq tx WalletManager.publishAndCommitBsqTx() should have commited the tx to both bsq and btc
+        // wallets so the next line causes no effect.
+        // If it's a btc tx, the next line adds the tx to the wallet.
+        wallet.maybeCommitTx(tx);
+
         Futures.addCallback(peerGroup.broadcastTransaction(tx).future(), new FutureCallback<Transaction>() {
             @Override
             public void onSuccess(@Nullable Transaction result) {
-                if (result != null) {
-                    if (txId.equals(result.getHashAsString())) {
                 // We expect that there is still a timeout in our map, otherwise the timeout got triggered
                 if (broadcastTimerMap.containsKey(txId)) {
-                            wallet.maybeCommitTx(tx);
                     stopAndRemoveTimer(txId);
                     // At regtest we get called immediately back but we want to make sure that the handler is not called
                     // before the caller is finished.
                     UserThread.execute(() -> callback.onSuccess(tx));
                 } else {
-                            stopAndRemoveTimer(txId);
+                    stopAndRemoveTimer(txId); //useless - txId was already removed.
                     log.warn("We got an onSuccess callback for a broadcast which already triggered the timeout.", txId);
                 }
-                    } else {
-                        stopAndRemoveTimer(txId);
-                        UserThread.execute(() -> callback.onTxMalleability(new TxMalleabilityException(tx, result)));
-                    }
-                } else {
-                    stopAndRemoveTimer(txId);
-                    UserThread.execute(() -> callback.onFailure(new TxBroadcastException("Transaction returned from the " +
-                            "broadcastTransaction call back is null.", txId)));
-                }
             }
 
             @Override