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

…3413)

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

* Enforce exactly 1 BSQ output for vote reveal tx

* Fix missing balance and button state update

* Refactor isBtcOutputOfBurnFeeTx method and add comments and TODOs

No functional change.

* Handle asset listing fee in custom method

We need to enforce a BSQ change output

As this is just tx creation code it has no consequences for the hard
fork.

* Use getPreparedBurnFeeTxForAssetListing

* Update comments to not use dust output values

* Fix missing balance and button state update

* Use same method for asset listing fee and proof of burn

Use same method for asset listing fee and proof of burn as tx structure
is same.
Update comments to be more general.

* Use getPreparedProofOfBurnTx

* Require mandatory BSQ change output for proposal fee tx.

We had in the doc stated that we require a mandatory BSQ change output
but it was not enforced in the implementation, causing similar issues
as in Asset listing and proof of  burn txs.

* Add fix for not correctly handled issuance tx

* Use new method for issuance tx

// 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.

* Handle all possible blind vote fee transactions

* Move check for invalid opReturn output up

* Add dust check at final sign method

* Fix incorrect comments

* Refactor

- Remove requireChangeOutput param which is always false
- Remove method which is used only by one caller
- Cleanup

* Add comment

* Fix comments, rename methods

* Move code of isBlindVoteBurnedFeeOutput to isBtcOutputOfBurnFeeTx

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);