Apply rule to not allow BSQ outputs after BTC output for regular txs

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

@@ -493,6 +493,15 @@ public Transaction signTx(Transaction tx) throws WalletException, TransactionVer
             }
         }
 
+        for (TransactionOutput txo : tx.getOutputs()) {
+            Coin value = txo.getValue();
+            // OpReturn outputs have value 0
+            if (value.isPositive()) {
+                checkArgument(Restrictions.isAboveDust(txo.getValue()),
+                        "An output value is below dust limit. Transaction=" + tx);
+            }
+        }
+
         checkWalletConsistency(wallet);
         verifyTransaction(tx);
         printTx("BSQ wallet: Signed Tx", tx);
@@ -556,11 +565,11 @@ private Transaction getPreparedSendTx(String receiverAddress, Coin receiverAmoun
             // Tx has as first output BSQ and an optional second BSQ change output.
             // At that stage we do not have added the BTC inputs so there is no BTC change output here.
             if (tx.getOutputs().size() == 2) {
-                TransactionOutput bsqChangeOutput = tx.getOutputs().get(1);
-                if (!Restrictions.isAboveDust(bsqChangeOutput.getValue())) {
-                    String msg = "BSQ change output is below dust limit. outputValue=" + bsqChangeOutput.getValue().toFriendlyString();
+                Coin bsqChangeOutputValue = tx.getOutputs().get(1).getValue();
+                if (!Restrictions.isAboveDust(bsqChangeOutputValue)) {
+                    String msg = "BSQ change output is below dust limit. outputValue=" + bsqChangeOutputValue.value / 100 + " BSQ";
                     log.warn(msg);
-                    throw new BsqChangeBelowDustException(msg, bsqChangeOutput.getValue());
+                    throw new BsqChangeBelowDustException(msg, bsqChangeOutputValue);
                 }
             }
 
@@ -573,60 +582,128 @@ private Transaction getPreparedSendTx(String receiverAddress, Coin receiverAmoun
 
 
     ///////////////////////////////////////////////////////////////////////////////////////////
-    // Burn fee tx
+    // Burn fee txs
     ///////////////////////////////////////////////////////////////////////////////////////////
 
+    public Transaction getPreparedTradeFeeTx(Coin fee) throws InsufficientBsqException {
+        daoKillSwitch.assertDaoIsNotDisabled();
+
+        Transaction tx = new Transaction(params);
+        addInputsAndChangeOutputForTx(tx, fee, bsqCoinSelector);
+        return tx;
+    }
+
     // We create a tx with Bsq inputs for the fee and optional BSQ change output.
     // As the fee amount will be missing in the output those BSQ fees are burned.
-    public Transaction getPreparedProposalTx(Coin fee, boolean requireChangeOutput) throws InsufficientBsqException {
-        return getPreparedBurnFeeTx(fee, requireChangeOutput);
+    public Transaction getPreparedProposalTx(Coin fee) throws InsufficientBsqException {
+        return getPreparedTxWithMandatoryBsqChangeOutput(fee);
     }
 
-    public Transaction getPreparedBurnFeeTx(Coin fee) throws InsufficientBsqException {
-        return getPreparedBurnFeeTx(fee, false);
+    public Transaction getPreparedIssuanceTx(Coin fee) throws InsufficientBsqException {
+        return getPreparedTxWithMandatoryBsqChangeOutput(fee);
     }
 
-    private Transaction getPreparedBurnFeeTx(Coin fee, boolean requireChangeOutput) throws InsufficientBsqException {
+    public Transaction getPreparedProofOfBurnTx(Coin fee) throws InsufficientBsqException {
+        return getPreparedTxWithMandatoryBsqChangeOutput(fee);
+    }
+
+    public Transaction getPreparedBurnFeeTxForAssetListing(Coin fee) throws InsufficientBsqException {
+        return getPreparedTxWithMandatoryBsqChangeOutput(fee);
+    }
+
+    // We need to require one BSQ change output as we could otherwise not be able to distinguish between 2
+    // structurally same transactions where only the BSQ fee is different. In case of asset listing fee and proof of
+    // burn it is a user input, so it is not know to the parser, instead we derive the burned fee from the parser.
+
+    // In case of proposal fee we could derive it from the params.
+
+    // For issuance txs we also require a BSQ change output before the issuance output gets added. There was a
+    // minor bug with the old version that multiple inputs would have caused an exception in case there was no
+    // change output (e.g. inputs of 21 and 6 BSQ for BSQ fee of 21 BSQ would have caused that only 1 input was used
+    // and then caused an error as we enforced a change output. This new version handles such cases correctly.
+
+    // Examples for the structurally indistinguishable transactions:
+    // Case 1: 10 BSQ fee to burn
+    // In: 17 BSQ
+    // Out: BSQ change 7 BSQ -> valid BSQ
+    // Out: OpReturn
+    // Miner fee: 1000 sat  (10 BSQ burned)
+
+    // Case 2: 17 BSQ fee to burn
+    // In: 17 BSQ
+    // Out: burned BSQ change 7 BSQ -> BTC (7 BSQ burned)
+    // Out: OpReturn
+    // Miner fee: 1000 sat  (10 BSQ burned)
+
+    private Transaction getPreparedTxWithMandatoryBsqChangeOutput(Coin fee) throws InsufficientBsqException {
         daoKillSwitch.assertDaoIsNotDisabled();
-        final Transaction tx = new Transaction(params);
-        addInputsAndChangeOutputForTx(tx, fee, bsqCoinSelector, requireChangeOutput);
-        // printTx("getPreparedFeeTx", tx);
-        return tx;
+
+        Transaction tx = new Transaction(params);
+        // We look for inputs covering out BSQ fee we want to pay.
+        CoinSelection coinSelection = bsqCoinSelector.select(fee, wallet.calculateAllSpendCandidates());
+        try {
+            Coin change = bsqCoinSelector.getChange(fee, coinSelection);
+            if (change.isZero() || Restrictions.isDust(change)) {
+                // If change is zero or below dust we increase required input amount to enforce a BSQ change output.
+                // All outputs after that are considered BTC and therefore would be burned BSQ if BSQ is left from what
+                // we use for miner fee.
+
+                Coin minDustThreshold = Coin.valueOf(preferences.getIgnoreDustThreshold());
+                Coin increasedRequiredInput = fee.add(minDustThreshold);
+                coinSelection = bsqCoinSelector.select(increasedRequiredInput, wallet.calculateAllSpendCandidates());
+                change = bsqCoinSelector.getChange(fee, coinSelection);
+
+                log.warn("We increased required input as change output was zero or dust: New change value={}", change);
+                String info = "Available BSQ balance=" + coinSelection.valueGathered.value / 100 + " BSQ. " +
+                        "Intended fee to burn=" + fee.value / 100 + " BSQ. " +
+                        "Please increase your balance to at least " + (coinSelection.valueGathered.value + minDustThreshold.value) / 100 + " BSQ.";
+                checkArgument(coinSelection.valueGathered.compareTo(fee) > 0,
+                        "This transaction require a change output of at least " + minDustThreshold.value / 100 + " BSQ (dust limit). " +
+                                info);
+
+                checkArgument(!Restrictions.isDust(change),
+                        "This transaction would create a dust output of " + change.value / 100 + " BSQ. " +
+                                "It requires a change output of at least " + minDustThreshold.value / 100 + " BSQ (dust limit). " +
+                                info);
+            }
+
+            coinSelection.gathered.forEach(tx::addInput);
+            tx.addOutput(change, getChangeAddress());
+
+            return tx;
+
+        } catch (InsufficientMoneyException e) {
+            log.error("coinSelection.gathered={}", coinSelection.gathered);
+            throw new InsufficientBsqException(e.missing);
+        }
     }
 
     private void addInputsAndChangeOutputForTx(Transaction tx,
                                                Coin fee,
-                                               BsqCoinSelector bsqCoinSelector,
-                                               boolean requireChangeOutput)
+                                               BsqCoinSelector bsqCoinSelector)
             throws InsufficientBsqException {
         Coin requiredInput;
         // If our fee is less then dust limit we increase it so we are sure to not get any dust output.
-        if (Restrictions.isDust(fee))
-            requiredInput = Restrictions.getMinNonDustOutput().add(fee);
-        else
+        if (Restrictions.isDust(fee)) {
+            requiredInput = fee.add(Restrictions.getMinNonDustOutput());
+        } else {
             requiredInput = fee;
+        }
 
         CoinSelection coinSelection = bsqCoinSelector.select(requiredInput, wallet.calculateAllSpendCandidates());
         coinSelection.gathered.forEach(tx::addInput);
         try {
-            // TODO why is fee passed to getChange ???
             Coin change = bsqCoinSelector.getChange(fee, coinSelection);
-            if (requireChangeOutput) {
-                checkArgument(change.isPositive(),
-                        "This transaction requires a mandatory BSQ change output. " +
-                                "At least " + Restrictions.getMinNonDustOutput().add(fee).value / 100d + " " +
-                                "BSQ is needed for this transaction");
-            }
-
             if (change.isPositive()) {
                 checkArgument(Restrictions.isAboveDust(change),
                         "The change output of " + change.value / 100d + " BSQ is below the min. dust value of "
                                 + Restrictions.getMinNonDustOutput().value / 100d +
-                                ". At least " + Restrictions.getMinNonDustOutput().add(fee).value / 100d + " " +
-                                "BSQ is needed for this transaction");
+                                ". At least " + Restrictions.getMinNonDustOutput().add(fee).value / 100d +
+                                " BSQ is needed for this transaction");
                 tx.addOutput(change, getChangeAddress());
             }
         } catch (InsufficientMoneyException e) {
+            log.error(tx.toString());
             throw new InsufficientBsqException(e.missing);
         }
     }
@@ -642,7 +719,7 @@ public Transaction getPreparedBlindVoteTx(Coin fee, Coin stake) throws Insuffici
         daoKillSwitch.assertDaoIsNotDisabled();
         Transaction tx = new Transaction(params);
         tx.addOutput(new TransactionOutput(params, tx, stake, getUnusedAddress()));
-        addInputsAndChangeOutputForTx(tx, fee.add(stake), bsqCoinSelector, false);
+        addInputsAndChangeOutputForTx(tx, fee.add(stake), bsqCoinSelector);
         //printTx("getPreparedBlindVoteTx", tx);
         return tx;
     }
@@ -676,7 +753,7 @@ public Transaction getPreparedLockupTx(Coin lockupAmount) throws AddressFormatEx
         Transaction tx = new Transaction(params);
         checkArgument(Restrictions.isAboveDust(lockupAmount), "The amount is too low (dust limit).");
         tx.addOutput(new TransactionOutput(params, tx, lockupAmount, getUnusedAddress()));
-        addInputsAndChangeOutputForTx(tx, lockupAmount, bsqCoinSelector, false);
+        addInputsAndChangeOutputForTx(tx, lockupAmount, bsqCoinSelector);
         printTx("prepareLockupTx", tx);
         return tx;
     }

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

@@ -408,13 +408,13 @@ public Transaction completePreparedSendBsqTx(Transaction preparedBsqTx, boolean
             TransactionVerificationException, WalletException, InsufficientMoneyException {
         // preparedBsqTx has following structure:
         // inputs [1-n] BSQ inputs
-        // outputs [0-1] BSQ receivers output
+        // outputs [1] BSQ receivers output
         // outputs [0-1] BSQ change output
 
         // We add BTC mining fee. Result tx looks like:
         // inputs [1-n] BSQ inputs
         // inputs [1-n] BTC inputs
-        // outputs [0-1] BSQ receivers output
+        // outputs [1] BSQ receivers output
         // outputs [0-1] BSQ change output
         // outputs [0-1] BTC change output
         // mining fee: BTC mining fee
@@ -426,7 +426,7 @@ public Transaction completePreparedBsqTx(Transaction preparedBsqTx, boolean useC
 
         // preparedBsqTx has following structure:
         // inputs [1-n] BSQ inputs
-        // outputs [0-1] BSQ receivers output
+        // outputs [1] BSQ receivers output
         // outputs [0-1] BSQ change output
         // mining fee: optional burned BSQ fee (only if opReturnData != null)
 

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

@@ -220,6 +220,10 @@ public Transaction completeBsqTradingFeeTx(Transaction preparedBsqTx,
         // wait for 1 confirmation)
         // In case of double spend we will detect later in the trade process and use a ban score to penalize bad behaviour (not impl. yet)
 
+        // If BSQ trade fee > reservedFundsForOffer we would create a BSQ output instead of a BTC output.
+        // As the min. reservedFundsForOffer is 0.001 BTC which is 1000 BSQ this is an unrealistic scenario and not
+        // handled atm (if BTC price is 1M USD and BSQ price is 0.1 USD, then fee would be 10% which still is unrealistic).
+
         // WalletService.printTx("preparedBsqTx", preparedBsqTx);
         SendRequest sendRequest = SendRequest.forTx(preparedBsqTx);
         sendRequest.shuffleOutputs = false;

core/src/main/java/bisq/core/dao/governance/asset/AssetService.java

@@ -322,11 +322,11 @@ public Transaction payFee(StatefulAsset statefulAsset, long listingFee) throws I
         checkArgument(listingFee % 100 == 0, "Fee must be a multiple of 1 BSQ (100 satoshi).");
         try {
             // We create a prepared Bsq Tx for the listing fee.
-            final Transaction preparedBurnFeeTx = bsqWalletService.getPreparedBurnFeeTx(Coin.valueOf(listingFee));
+            Transaction preparedBurnFeeTx = bsqWalletService.getPreparedBurnFeeTxForAssetListing(Coin.valueOf(listingFee));
             byte[] hash = AssetConsensus.getHash(statefulAsset);
             byte[] opReturnData = AssetConsensus.getOpReturnData(hash);
             // We add the BTC inputs for the miner fee.
-            final Transaction txWithBtcFee = btcWalletService.completePreparedBurnBsqTx(preparedBurnFeeTx, opReturnData);
+            Transaction txWithBtcFee = btcWalletService.completePreparedBurnBsqTx(preparedBurnFeeTx, opReturnData);
             // We sign the BSQ inputs of the final tx.
             Transaction transaction = bsqWalletService.signTx(txWithBtcFee);
             log.info("Asset listing fee tx: " + transaction);

core/src/main/java/bisq/core/dao/governance/proofofburn/ProofOfBurnService.java

@@ -134,11 +134,11 @@ public void onParseBlockCompleteAfterBatchProcessing(Block block) {
     public Transaction burn(String preImageAsString, long amount) throws InsufficientMoneyException, TxException {
         try {
             // We create a prepared Bsq Tx for the burn amount
-            final Transaction preparedBurnFeeTx = bsqWalletService.getPreparedBurnFeeTx(Coin.valueOf(amount));
+            Transaction preparedBurnFeeTx = bsqWalletService.getPreparedProofOfBurnTx(Coin.valueOf(amount));
             byte[] hash = getHashFromPreImage(preImageAsString);
             byte[] opReturnData = ProofOfBurnConsensus.getOpReturnData(hash);
             // We add the BTC inputs for the miner fee.
-            final Transaction txWithBtcFee = btcWalletService.completePreparedBurnBsqTx(preparedBurnFeeTx, opReturnData);
+            Transaction txWithBtcFee = btcWalletService.completePreparedBurnBsqTx(preparedBurnFeeTx, opReturnData);
             // We sign the BSQ inputs of the final tx.
             Transaction transaction = bsqWalletService.signTx(txWithBtcFee);
             log.info("Proof of burn tx: " + transaction);

core/src/main/java/bisq/core/dao/governance/proposal/BaseProposalFactory.java

@@ -87,8 +87,9 @@ private Transaction createTransaction(R proposal) throws InsufficientMoneyExcept
         try {
             Coin fee = ProposalConsensus.getFee(daoStateService, daoStateService.getChainHeight());
             // We create a prepared Bsq Tx for the proposal fee.
-            boolean requireChangeOutput = proposal instanceof IssuanceProposal;
-            Transaction preparedBurnFeeTx = bsqWalletService.getPreparedProposalTx(fee, requireChangeOutput);
+            Transaction preparedBurnFeeTx = proposal instanceof IssuanceProposal ?
+                    bsqWalletService.getPreparedIssuanceTx(fee) :
+                    bsqWalletService.getPreparedProposalTx(fee);
 
             // payload does not have txId at that moment
             byte[] hashOfPayload = ProposalConsensus.getHashOfPayload(proposal);