diff --git a/core/src/main/java/bisq/core/btc/wallet/BsqWalletService.java b/core/src/main/java/bisq/core/btc/wallet/BsqWalletService.java index 300771460f4..915e00ca235 100644 --- a/core/src/main/java/bisq/core/btc/wallet/BsqWalletService.java +++ b/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; } diff --git a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java b/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java index 4841551cb9a..7e1ed9c9c74 100644 --- a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java +++ b/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) diff --git a/core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java b/core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java index 798ae449c5f..e0c57058548 100644 --- a/core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java +++ b/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; diff --git a/core/src/main/java/bisq/core/dao/governance/asset/AssetService.java b/core/src/main/java/bisq/core/dao/governance/asset/AssetService.java index 7b9390da6cd..c765697a704 100644 --- a/core/src/main/java/bisq/core/dao/governance/asset/AssetService.java +++ b/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); diff --git a/core/src/main/java/bisq/core/dao/governance/proofofburn/ProofOfBurnService.java b/core/src/main/java/bisq/core/dao/governance/proofofburn/ProofOfBurnService.java index e347656a597..cca3185a507 100644 --- a/core/src/main/java/bisq/core/dao/governance/proofofburn/ProofOfBurnService.java +++ b/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); diff --git a/core/src/main/java/bisq/core/dao/governance/proposal/BaseProposalFactory.java b/core/src/main/java/bisq/core/dao/governance/proposal/BaseProposalFactory.java index 4c3cb5f78f7..abb58e32c2d 100644 --- a/core/src/main/java/bisq/core/dao/governance/proposal/BaseProposalFactory.java +++ b/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); diff --git a/core/src/main/java/bisq/core/dao/node/parser/TxOutputParser.java b/core/src/main/java/bisq/core/dao/node/parser/TxOutputParser.java index e520e191015..0a9d09bf34d 100644 --- a/core/src/main/java/bisq/core/dao/node/parser/TxOutputParser.java +++ b/core/src/main/java/bisq/core/dao/node/parser/TxOutputParser.java @@ -17,7 +17,9 @@ package bisq.core.dao.node.parser; +import bisq.core.app.BisqEnvironment; import bisq.core.dao.governance.bond.BondConsensus; +import bisq.core.dao.governance.param.Param; import bisq.core.dao.state.DaoStateService; import bisq.core.dao.state.model.blockchain.OpReturnType; import bisq.core.dao.state.model.blockchain.TxOutput; @@ -38,11 +40,57 @@ /** * Checks if an output is a BSQ output and apply state change. + * + * With block 602500 (about 4 weeks after v1.2.0 release) we enforce a new rule which represents a + * hard fork. Not updated nodes would see an out of sync dao state hash if a relevant transaction would + * happen again. + * Further (highly unlikely) consequences could be: + * If the BSQ output would be sent to a BSQ address the old client would accept that even it is + * invalid according to the new rules. But sending such a output would require a manually crafted tx + * (not possible in the UI). Worst case a not updated user would buy invalid BSQ but that is not possible as we + * enforce update to 1.2.0 for trading a few days after release as that release introduced the new trade protocol + * and protection tool. Only of both both traders would have deactivated filter messages they could trade. + * + * Problem description: + * We did not apply the check to not allow BSQ outputs after we had detected a BTC output. + * The supported BSQ transactions did not support such cases anyway but we missed an edge case: + * A trade fee tx in case when the BTC input matches exactly the BTC output + * (or BTC change was <= the miner fee) and the BSQ fee was > the miner fee. Then we + * create a change output after the BTC output (using an address from the BTC wallet) and as + * available BSQ was >= as spent BSQ it was considered a valid BSQ output. + * There have been observed 5 such transactions where 4 got spent later to a BTC address and by that burned + * the pending BSQ (spending amount was higher than sending amount). One was still unspent. + * The BSQ was sitting in the BTC wallet so not even visible as BSQ to the user. + * If the user would have crafted a custom BSQ tx he could have avoided that the full trade fee was burned. + * + * Not an universal rule: + * We cannot enforce the rule that no BSQ output is permitted to all possible transactions because there can be cases + * where we need to permit this case. + * For instance in case we confiscate a lockupTx we have usually 2 BSQ outputs: The first one is the bond which + * should be confiscated and the second one is the BSQ change output. + * At confiscating we set the first to TxOutputType.BTC_OUTPUT but we do not want to confiscate + * the second BSQ change output as well. So we do not apply the rule that no BSQ is allowed once a BTC output is + * found. Theoretically other transactions could be confiscated as well and all BSQ tx which allow > 1 BSQ outputs + * would have the same issue as well if the first output gets confiscated. + * We also don't enforce the rule for irregular or invalid txs which are usually set and detected at the end of + * the tx parsing which is done in the TxParser. Blind vote and LockupTx with invalid OpReturn would be such cases + * where we don't want to invalidate the change output (See comments in TxParser). + * + * Most transactions created in Bisq (Proposal, blind vote and lockup,...) have only 1 or 2 BSQ + * outputs but we do not enforce a limit of max. 2 transactions in the parser. + * We leave for now that flexibility but it should not be considered as a rule. We might strengthen + * it any time if we find a reason for that (e.g. attack risk) and add checks that no more + * BSQ outputs are permitted for those txs. + * Some transactions like issuance, vote reveal and unlock have exactly 1 BSQ output and that rule + * is enforced. */ @Slf4j -public class TxOutputParser { - private final DaoStateService daoStateService; +class TxOutputParser { + private static int ACTIVATE_HARD_FORK_1_HEIGHT_MAINNET = 605000; + private static int ACTIVATE_HARD_FORK_1_HEIGHT_TESTNET = 1583054; + private static int ACTIVATE_HARD_FORK_1_HEIGHT_REGTEST = 1; + private final DaoStateService daoStateService; // Setters @Getter @Setter @@ -66,10 +114,12 @@ public class TxOutputParser { private Optional optionalVoteRevealUnlockStakeOutput = Optional.empty(); @Getter private Optional optionalLockupOutput = Optional.empty(); + private Optional optionalOpReturnIndex = Optional.empty(); // Private private int lockTime; private final List utxoCandidates = new ArrayList<>(); + private boolean prohibitMoreBsqOutputs = false; /////////////////////////////////////////////////////////////////////////////////////////// @@ -93,6 +143,8 @@ void processOpReturnOutput(TempTxOutput tempTxOutput) { optionalOpReturnType = getMappedOpReturnType(txOutputType); + optionalOpReturnType.ifPresent(e -> optionalOpReturnIndex = Optional.of(tempTxOutput.getIndex())); + // If we have a LOCKUP opReturn output we save the lockTime to apply it later to the LOCKUP output. // We keep that data in that other output as it makes parsing of the UNLOCK tx easier. optionalOpReturnType.filter(opReturnType -> opReturnType == OpReturnType.LOCKUP) @@ -100,14 +152,14 @@ void processOpReturnOutput(TempTxOutput tempTxOutput) { } void processTxOutput(TempTxOutput tempTxOutput) { - if (!daoStateService.isConfiscatedOutput(tempTxOutput.getKey())) { - // We don not expect here an opReturn output as we do not get called on the last output. Any opReturn at - // another output index is invalid. - if (tempTxOutput.isOpReturnOutput()) { - tempTxOutput.setTxOutputType(TxOutputType.INVALID_OUTPUT); - return; - } + // We don not expect here an opReturn output as we do not get called on the last output. Any opReturn at + // another output index is invalid. + if (tempTxOutput.isOpReturnOutput()) { + tempTxOutput.setTxOutputType(TxOutputType.INVALID_OUTPUT); + return; + } + if (!daoStateService.isConfiscatedOutput(tempTxOutput.getKey())) { long txOutputValue = tempTxOutput.getValue(); int index = tempTxOutput.getIndex(); if (isUnlockBondTx(tempTxOutput.getValue(), index)) { @@ -118,8 +170,18 @@ void processTxOutput(TempTxOutput tempTxOutput) { } else if (isBtcOutputOfBurnFeeTx(tempTxOutput)) { // In case we have the opReturn for a burn fee tx all outputs after 1st output are considered BTC handleBtcOutput(tempTxOutput, index); + } else if (isHardForkActivated(tempTxOutput) && isIssuanceCandidateTxOutput(tempTxOutput)) { + // After the hard fork activation we fix a bug with a transaction which would have interpreted the + // issuance output as BSQ if the availableInputValue was >= issuance amount. + // Such a tx was never created but as we don't know if it will happen before activation date we cannot + // enforce the bug fix which represents a rule change before the activation date. + handleIssuanceCandidateOutput(tempTxOutput); } else if (availableInputValue > 0 && availableInputValue >= txOutputValue) { - handleBsqOutput(tempTxOutput, index, txOutputValue); + if (isHardForkActivated(tempTxOutput) && prohibitMoreBsqOutputs) { + handleBtcOutput(tempTxOutput, index); + } else { + handleBsqOutput(tempTxOutput, index, txOutputValue); + } } else { handleBtcOutput(tempTxOutput, index); } @@ -127,6 +189,9 @@ void processTxOutput(TempTxOutput tempTxOutput) { log.warn("TxOutput {} is confiscated ", tempTxOutput.getKey()); // We only burn that output availableInputValue -= tempTxOutput.getValue(); + + // We must not set prohibitMoreBsqOutputs at confiscation transactions as optional + // BSQ change output (output 2) must not be confiscated. tempTxOutput.setTxOutputType(TxOutputType.BTC_OUTPUT); } } @@ -139,6 +204,7 @@ void commitUTXOCandidates() { * This sets all outputs to BTC_OUTPUT and doesn't add any txOutputs to the unspentTxOutput map in daoStateService */ void invalidateUTXOCandidates() { + // We do not need to apply prohibitMoreBsqOutputs as all spendable outputs are set to BTC_OUTPUT anyway. utxoCandidates.forEach(output -> output.setTxOutputType(TxOutputType.BTC_OUTPUT)); } @@ -171,17 +237,108 @@ private void handleUnlockBondTx(TempTxOutput txOutput) { utxoCandidates.add(txOutput); bsqOutputFound = true; + + // We do not permit more BSQ outputs after the unlock txo as we don't expect additional BSQ outputs. + prohibitMoreBsqOutputs = true; } private boolean isBtcOutputOfBurnFeeTx(TempTxOutput tempTxOutput) { - // If we get a asset listing or proof of burn tx we have only 1 BSQ output and if the - // burned amount is larger than the miner fee we might have a BTC output for receiving the burned funds. - // If the burned funds are less than the miner fee a BTC input is used for miner fee and a BTC change output for - // the remaining funds. In any case only the first output is BSQ all the others are BTC. - return optionalOpReturnType.isPresent() && - (optionalOpReturnType.get() == OpReturnType.ASSET_LISTING_FEE || - optionalOpReturnType.get() == OpReturnType.PROOF_OF_BURN) && - tempTxOutput.getIndex() >= 1; + if (optionalOpReturnType.isPresent()) { + int index = tempTxOutput.getIndex(); + switch (optionalOpReturnType.get()) { + case UNDEFINED: + break; + case PROPOSAL: + if (isHardForkActivated(tempTxOutput)) { + // We enforce a mandatory BSQ change output. + // We need that as similar to ASSET_LISTING_FEE and PROOF_OF_BURN + // we could not distinguish between 2 structurally same transactions otherwise (only way here + // would be to check the proposal fee as that is known from the params). + return index >= 1; + } + break; + case COMPENSATION_REQUEST: + break; + case REIMBURSEMENT_REQUEST: + break; + case BLIND_VOTE: + if (isHardForkActivated(tempTxOutput)) { + // After the hard fork activation we fix a bug with a transaction which would have interpreted the + // burned vote fee output as BSQ if the vote fee was >= miner fee. + // Such a tx was never created but as we don't know if it will happen before activation date we cannot + // enforce the bug fix which represents a rule change before the activation date. + + // If it is the vote stake output we return false. + if (index == 0) { + return false; + } + + // There must be a vote fee left + if (availableInputValue <= 0) { + return false; + } + + // Burned BSQ output is last output before opReturn. + // We could have also a BSQ change output as last output before opReturn but that will + // be detected at blindVoteFee check. + // We always have the BSQ change before the burned BSQ output if both are present. + checkArgument(optionalOpReturnIndex.isPresent()); + if (index != optionalOpReturnIndex.get() - 1) { + return false; + } + + // Without checking the fee we would not be able to distinguish between 2 structurally same transactions, one + // where the output is burned BSQ and one where it is a BSQ change output. + long blindVoteFee = daoStateService.getParamValueAsCoin(Param.BLIND_VOTE_FEE, tempTxOutput.getBlockHeight()).value; + return availableInputValue == blindVoteFee; + } + case VOTE_REVEAL: + break; + case LOCKUP: + break; + case ASSET_LISTING_FEE: + case PROOF_OF_BURN: + // Asset listing fee and proof of burn tx are structurally the same. + + // 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. + + // 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) + return index >= 1; + } + } + return false; + } + + private boolean isIssuanceCandidateTxOutput(TempTxOutput tempTxOutput) { + // If we have BSQ left as fee and we are at the second output we interpret it as a compensation request output. + return availableInputValue > 0 && + tempTxOutput.getIndex() == 1 && + optionalOpReturnType.isPresent() && + (optionalOpReturnType.get() == OpReturnType.COMPENSATION_REQUEST || + optionalOpReturnType.get() == OpReturnType.REIMBURSEMENT_REQUEST); + } + + private void handleIssuanceCandidateOutput(TempTxOutput tempTxOutput) { + // We do not permit more BSQ outputs after the issuance candidate. + prohibitMoreBsqOutputs = true; + + // We store the candidate but we don't apply the TxOutputType yet as we need to verify the fee after all + // outputs are parsed and check the phase. The TxParser will do that.... + optionalIssuanceCandidate = Optional.of(tempTxOutput); } private void handleBsqOutput(TempTxOutput txOutput, int index, long txOutputValue) { @@ -201,6 +358,9 @@ private void handleBsqOutput(TempTxOutput txOutput, int index, long txOutputValu } else if (isFirstOutput && opReturnTypeCandidate == OpReturnType.VOTE_REVEAL) { txOutputType = TxOutputType.VOTE_REVEAL_UNLOCK_STAKE_OUTPUT; optionalVoteRevealUnlockStakeOutput = Optional.of(txOutput); + + // We do not permit more BSQ outputs after the VOTE_REVEAL_UNLOCK_STAKE_OUTPUT. + prohibitMoreBsqOutputs = true; } else if (isFirstOutput && opReturnTypeCandidate == OpReturnType.LOCKUP) { txOutputType = TxOutputType.LOCKUP_OUTPUT; @@ -219,20 +379,43 @@ private void handleBsqOutput(TempTxOutput txOutput, int index, long txOutputValu } private void handleBtcOutput(TempTxOutput txOutput, int index) { - // If we have BSQ left as fee and we are at the second output it might be a compensation request output. - // We store the candidate but we don't apply the TxOutputType yet as we need to verify the fee after all - // outputs are parsed and check the phase. The TxParser will do that.... - if (availableInputValue > 0 && - index == 1 && - optionalOpReturnType.isPresent() && - (optionalOpReturnType.get() == OpReturnType.COMPENSATION_REQUEST || - optionalOpReturnType.get() == OpReturnType.REIMBURSEMENT_REQUEST)) { - optionalIssuanceCandidate = Optional.of(txOutput); - } else { + if (isHardForkActivated(txOutput)) { txOutput.setTxOutputType(TxOutputType.BTC_OUTPUT); + + // For regular transactions we don't permit BSQ outputs after a BTC output was detected. + prohibitMoreBsqOutputs = true; + } else { + // If we have BSQ left as fee and we are at the second output it might be a compensation request output. + // We store the candidate but we don't apply the TxOutputType yet as we need to verify the fee after all + // outputs are parsed and check the phase. The TxParser will do that.... + if (availableInputValue > 0 && + index == 1 && + optionalOpReturnType.isPresent() && + (optionalOpReturnType.get() == OpReturnType.COMPENSATION_REQUEST || + optionalOpReturnType.get() == OpReturnType.REIMBURSEMENT_REQUEST)) { + optionalIssuanceCandidate = Optional.of(txOutput); + + // We do not permit more BSQ outputs after the issuance candidate. + prohibitMoreBsqOutputs = true; + } else { + txOutput.setTxOutputType(TxOutputType.BTC_OUTPUT); + + // For regular transactions we don't permit BSQ outputs after a BTC output was detected. + prohibitMoreBsqOutputs = true; + } } } + private boolean isHardForkActivated(TempTxOutput tempTxOutput) { + return tempTxOutput.getBlockHeight() >= getActivateHardFork1Height(); + } + + private int getActivateHardFork1Height() { + return BisqEnvironment.getBaseCurrencyNetwork().isMainnet() ? ACTIVATE_HARD_FORK_1_HEIGHT_MAINNET : + BisqEnvironment.getBaseCurrencyNetwork().isTestnet() ? ACTIVATE_HARD_FORK_1_HEIGHT_TESTNET : + ACTIVATE_HARD_FORK_1_HEIGHT_REGTEST; + } + /////////////////////////////////////////////////////////////////////////////////////////// // Static diff --git a/core/src/main/java/bisq/core/dao/node/parser/TxParser.java b/core/src/main/java/bisq/core/dao/node/parser/TxParser.java index 42be945a632..1b518de77d1 100644 --- a/core/src/main/java/bisq/core/dao/node/parser/TxParser.java +++ b/core/src/main/java/bisq/core/dao/node/parser/TxParser.java @@ -217,19 +217,33 @@ private void applyTxTypeAndTxOutputType(int blockHeight, TempTx tempTx, long bsq // We need to check if any tempTxOutput is available and if so and the OpReturn data is invalid we // set the output to a BTC output. We must not use `if else` cases here! if (opReturnType != OpReturnType.COMPENSATION_REQUEST && opReturnType != OpReturnType.REIMBURSEMENT_REQUEST) { + // We applied already the check to not permit further BSQ outputs after the issuanceCandidate in the + // txOutputParser so we don't need to do any additional check here when we change to BTC_OUTPUT. txOutputParser.getOptionalIssuanceCandidate().ifPresent(tempTxOutput -> tempTxOutput.setTxOutputType(TxOutputType.BTC_OUTPUT)); } if (opReturnType != OpReturnType.BLIND_VOTE) { - txOutputParser.getOptionalBlindVoteLockStakeOutput().ifPresent(tempTxOutput -> tempTxOutput.setTxOutputType(TxOutputType.BTC_OUTPUT)); + txOutputParser.getOptionalBlindVoteLockStakeOutput().ifPresent(tempTxOutput -> { + // We cannot apply the rule to not allow BSQ outputs after a BTC output as the 2nd output is an + // optional BSQ change output and we don't want to burn that in case the opReturn is invalid. + tempTxOutput.setTxOutputType(TxOutputType.BTC_OUTPUT); + }); } if (opReturnType != OpReturnType.VOTE_REVEAL) { - txOutputParser.getOptionalVoteRevealUnlockStakeOutput().ifPresent(tempTxOutput -> tempTxOutput.setTxOutputType(TxOutputType.BTC_OUTPUT)); + txOutputParser.getOptionalVoteRevealUnlockStakeOutput().ifPresent(tempTxOutput -> { + // We do not apply the rule to not allow BSQ outputs after a BTC output here because we expect only + // one BSQ output anyway. + tempTxOutput.setTxOutputType(TxOutputType.BTC_OUTPUT); + }); } if (opReturnType != OpReturnType.LOCKUP) { - txOutputParser.getOptionalLockupOutput().ifPresent(tempTxOutput -> tempTxOutput.setTxOutputType(TxOutputType.BTC_OUTPUT)); + txOutputParser.getOptionalLockupOutput().ifPresent(tempTxOutput -> { + // We cannot apply the rule to not allow BSQ outputs after a BTC output as the 2nd output is an + // optional BSQ change output and we don't want to burn that in case the opReturn is invalid. + tempTxOutput.setTxOutputType(TxOutputType.BTC_OUTPUT); + }); } } @@ -259,12 +273,14 @@ private void processIssuance(int blockHeight, TempTx tempTx, long bsqFee) { } } else { // This could be a valid compensation request that failed to be included in a block during the - // correct phase due to no fault of the user. Better not burn the change as long as the BSQ inputs + // correct phase due to no fault of the user. We must not burn the change as long as the BSQ inputs // cover the value of the outputs. // We tolerate such an incorrect tx and do not burn the BSQ tempTx.setTxType(TxType.IRREGULAR); // Make sure the optionalIssuanceCandidate is set to BTC + // We applied already the check to not permit further BSQ outputs after the issuanceCandidate in the + // txOutputParser so we don't need to do any additional check here when we change to BTC_OUTPUT. optionalIssuanceCandidate.ifPresent(tempTxOutput -> tempTxOutput.setTxOutputType(TxOutputType.BTC_OUTPUT)); // Empty Optional case is a possible valid case where a random tx matches our opReturn rules but it is not a // valid BSQ tx. diff --git a/core/src/main/java/bisq/core/offer/placeoffer/tasks/CreateMakerFeeTx.java b/core/src/main/java/bisq/core/offer/placeoffer/tasks/CreateMakerFeeTx.java index 70bf6d5cf25..da2b60a0ace 100644 --- a/core/src/main/java/bisq/core/offer/placeoffer/tasks/CreateMakerFeeTx.java +++ b/core/src/main/java/bisq/core/offer/placeoffer/tasks/CreateMakerFeeTx.java @@ -109,7 +109,7 @@ public void onFailure(TxBroadcastException exception) { }); } else { final BsqWalletService bsqWalletService = model.getBsqWalletService(); - Transaction preparedBurnFeeTx = model.getBsqWalletService().getPreparedBurnFeeTx(offer.getMakerFee()); + Transaction preparedBurnFeeTx = model.getBsqWalletService().getPreparedTradeFeeTx(offer.getMakerFee()); Transaction txWithBsqFee = tradeWalletService.completeBsqTradingFeeTx(preparedBurnFeeTx, fundingAddress, reservedForTradeAddress, diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/taker/CreateTakerFeeTx.java b/core/src/main/java/bisq/core/trade/protocol/tasks/taker/CreateTakerFeeTx.java index 15ede07092e..1e9f1a561c8 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/taker/CreateTakerFeeTx.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/taker/CreateTakerFeeTx.java @@ -79,7 +79,7 @@ protected void run() { false, null); } else { - Transaction preparedBurnFeeTx = processModel.getBsqWalletService().getPreparedBurnFeeTx(trade.getTakerFee()); + Transaction preparedBurnFeeTx = processModel.getBsqWalletService().getPreparedTradeFeeTx(trade.getTakerFee()); Transaction txWithBsqFee = tradeWalletService.completeBsqTradingFeeTx(preparedBurnFeeTx, fundingAddress, reservedForTradeAddress, diff --git a/desktop/src/main/java/bisq/desktop/main/dao/burnbsq/assetfee/AssetFeeView.java b/desktop/src/main/java/bisq/desktop/main/dao/burnbsq/assetfee/AssetFeeView.java index 5988bf6f28e..1a40b35fa49 100644 --- a/desktop/src/main/java/bisq/desktop/main/dao/burnbsq/assetfee/AssetFeeView.java +++ b/desktop/src/main/java/bisq/desktop/main/dao/burnbsq/assetfee/AssetFeeView.java @@ -162,6 +162,7 @@ protected void activate() { assetService.getUpdateFlag().addListener(updateListener); bsqWalletService.addBsqBalanceListener(this); + onUpdateAvailableConfirmedBalance(bsqWalletService.getAvailableConfirmedBalance()); payFeeButton.setOnAction((event) -> { Coin listingFee = getListingFee(); @@ -225,7 +226,8 @@ public void onUpdateBalances(Coin availableConfirmedBalance, Coin lockedForVotingBalance, Coin lockupBondsBalance, Coin unlockingBondsBalance) { - bsqValidator.setAvailableBalance(availableConfirmedBalance); + + onUpdateAvailableConfirmedBalance(availableConfirmedBalance); } @@ -248,6 +250,11 @@ private void createListeners() { updateListener = observable -> updateList(); } + private void onUpdateAvailableConfirmedBalance(Coin availableConfirmedBalance) { + bsqValidator.setAvailableBalance(availableConfirmedBalance); + updateButtonState(); + } + private long getDays() { return getListingFee().value / assetService.getFeePerDay().value; } diff --git a/desktop/src/main/java/bisq/desktop/main/dao/burnbsq/proofofburn/ProofOfBurnView.java b/desktop/src/main/java/bisq/desktop/main/dao/burnbsq/proofofburn/ProofOfBurnView.java index 7ac3097ce24..c3944e85ad6 100644 --- a/desktop/src/main/java/bisq/desktop/main/dao/burnbsq/proofofburn/ProofOfBurnView.java +++ b/desktop/src/main/java/bisq/desktop/main/dao/burnbsq/proofofburn/ProofOfBurnView.java @@ -163,6 +163,7 @@ protected void activate() { proofOfBurnService.getUpdateFlag().addListener(updateListener); bsqWalletService.addBsqBalanceListener(this); + onUpdateAvailableConfirmedBalance(bsqWalletService.getAvailableConfirmedBalance()); burnButton.setOnAction((event) -> { Coin amount = getAmountFee(); @@ -221,7 +222,7 @@ public void onUpdateBalances(Coin availableConfirmedBalance, Coin lockedForVotingBalance, Coin lockupBondsBalance, Coin unlockingBondsBalance) { - bsqValidator.setAvailableBalance(availableConfirmedBalance); + onUpdateAvailableConfirmedBalance(availableConfirmedBalance); } @@ -253,6 +254,11 @@ private void createListeners() { updateListener = observable -> updateList(); } + private void onUpdateAvailableConfirmedBalance(Coin availableConfirmedBalance) { + bsqValidator.setAvailableBalance(availableConfirmedBalance); + updateButtonState(); + } + private void updateList() { myItemsObservableList.setAll(myProofOfBurnListService.getMyProofOfBurnList().stream() .map(myProofOfBurn -> new MyProofOfBurnListItem(myProofOfBurn, proofOfBurnService, bsqFormatter))