diff --git a/core/src/main/java/bisq/core/btc/BitcoinModule.java b/core/src/main/java/bisq/core/btc/BitcoinModule.java index b9e365a3bbd..69c6d3bc128 100644 --- a/core/src/main/java/bisq/core/btc/BitcoinModule.java +++ b/core/src/main/java/bisq/core/btc/BitcoinModule.java @@ -86,6 +86,7 @@ protected void configure() { bind(FeeProvider.class).in(Singleton.class); bind(PriceFeedService.class).in(Singleton.class); bind(FeeService.class).in(Singleton.class); + bind(TxFeeEstimationService.class).in(Singleton.class); } } diff --git a/core/src/main/java/bisq/core/btc/TxFeeEstimationService.java b/core/src/main/java/bisq/core/btc/TxFeeEstimationService.java new file mode 100644 index 00000000000..9a2e1c76ba5 --- /dev/null +++ b/core/src/main/java/bisq/core/btc/TxFeeEstimationService.java @@ -0,0 +1,180 @@ +/* + * 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 . + */ + +package bisq.core.btc; + +import bisq.core.btc.wallet.BtcWalletService; +import bisq.core.provider.fee.FeeService; +import bisq.core.user.Preferences; + +import bisq.common.util.Tuple2; + +import org.bitcoinj.core.Coin; +import org.bitcoinj.core.InsufficientMoneyException; + +import javax.inject.Inject; + +import com.google.common.annotations.VisibleForTesting; + +import java.util.List; + +import lombok.extern.slf4j.Slf4j; + +import static com.google.common.base.Preconditions.checkArgument; + +/** + * Util class for getting the estimated tx fee for maker or taker fee tx. + */ +@Slf4j +public class TxFeeEstimationService { + public static int TYPICAL_TX_WITH_1_INPUT_SIZE = 260; + private static int DEPOSIT_TX_SIZE = 320; + private static int PAYOUT_TX_SIZE = 380; + private static int BSQ_INPUT_INCREASE = 150; + private static int MAX_ITERATIONS = 10; + + private final FeeService feeService; + private final BtcWalletService btcWalletService; + private final Preferences preferences; + + @Inject + public TxFeeEstimationService(FeeService feeService, + BtcWalletService btcWalletService, + Preferences preferences) { + + this.feeService = feeService; + this.btcWalletService = btcWalletService; + this.preferences = preferences; + } + + public Tuple2 getEstimatedFeeAndTxSizeForTaker(Coin fundsNeededForTrade, Coin tradeFee) { + return getEstimatedFeeAndTxSize(true, + fundsNeededForTrade, + tradeFee, + feeService, + btcWalletService, + preferences); + } + + public Tuple2 getEstimatedFeeAndTxSizeForMaker(Coin reservedFundsForOffer, + Coin tradeFee) { + return getEstimatedFeeAndTxSize(false, + reservedFundsForOffer, + tradeFee, + feeService, + btcWalletService, + preferences); + } + + private Tuple2 getEstimatedFeeAndTxSize(boolean isTaker, + Coin amount, + Coin tradeFee, + FeeService feeService, + BtcWalletService btcWalletService, + Preferences preferences) { + Coin txFeePerByte = feeService.getTxFeePerByte(); + // We start with min taker fee size of 260 + int estimatedTxSize = TYPICAL_TX_WITH_1_INPUT_SIZE; + try { + estimatedTxSize = getEstimatedTxSize(List.of(tradeFee, amount), estimatedTxSize, txFeePerByte, btcWalletService); + } catch (InsufficientMoneyException e) { + if (isTaker) { + // if we cannot do the estimation we use the payout tx size + estimatedTxSize = PAYOUT_TX_SIZE; + } + log.info("We cannot do the fee estimation because there are not enough funds in the wallet. This is expected " + + "if the user pays from an external wallet. In that case we use an estimated tx size of {} bytes.", estimatedTxSize); + } + + if (!preferences.isPayFeeInBtc()) { + // If we pay the fee in BSQ we have one input more which adds about 150 bytes + // TODO: Clarify if there is always just one additional input or if there can be more. + estimatedTxSize += BSQ_INPUT_INCREASE; + } + + Coin txFee; + int size; + if (isTaker) { + int averageSize = (estimatedTxSize + DEPOSIT_TX_SIZE) / 2; // deposit tx has about 320 bytes + // We use at least the size of the payout tx to not underpay at payout. + size = Math.max(PAYOUT_TX_SIZE, averageSize); + txFee = txFeePerByte.multiply(size); + log.info("Fee estimation resulted in a tx size of {} bytes.\n" + + "We use an average between the taker fee tx and the deposit tx (320 bytes) which results in {} bytes.\n" + + "The payout tx has 380 bytes, we use that as our min value. Size for fee calculation is {} bytes.\n" + + "The tx fee of {} Sat", estimatedTxSize, averageSize, size, txFee.value); + } else { + size = estimatedTxSize; + txFee = txFeePerByte.multiply(size); + log.info("Fee estimation resulted in a tx size of {} bytes and a tx fee of {} Sat.", size, txFee.value); + } + + return new Tuple2<>(txFee, size); + } + + // We start with the initialEstimatedTxSize for a tx with 1 input (260) bytes and get from BitcoinJ a tx back which + // contains the required inputs to fund that tx (outputs + miner fee). The miner fee in that case is based on + // the assumption that we only need 1 input. Once we receive back the real tx size from the tx BitcoinJ has created + // with the required inputs we compare if the size is not more then 20% different to our assumed tx size. If we are inside + // that tolerance we use that tx size for our fee estimation, if not (if there has been more then 1 inputs) we + // apply the new fee based on the reported tx size and request again from BitcoinJ to fill that tx with the inputs + // to be sufficiently funded. The algorithm how BitcoinJ selects utxos is complex and contains several aspects + // (minimize fee, don't create too many tiny utxos,...). We treat that algorithm as an unknown and it is not + // guaranteed that there are more inputs required if we increase the fee (it could be that there is a better + // selection of inputs chosen if we have increased the fee and therefore less inputs and smaller tx size). As the increased fee might + // change the number of inputs we need to repeat that process until we are inside of a certain tolerance. To avoid + // potential endless loops we add a counter (we use 10, usually it takes just very few iterations). + // Worst case would be that the last size we got reported is > 20% off to + // the real tx size but as fee estimation is anyway a educated guess in the best case we don't worry too much. + // If we have underpaid the tx might take longer to get confirmed. + @VisibleForTesting + static int getEstimatedTxSize(List outputValues, + int initialEstimatedTxSize, + Coin txFeePerByte, + BtcWalletService btcWalletService) + throws InsufficientMoneyException { + boolean isInTolerance; + int estimatedTxSize = initialEstimatedTxSize; + int realTxSize; + int counter = 0; + do { + Coin txFee = txFeePerByte.multiply(estimatedTxSize); + realTxSize = btcWalletService.getEstimatedFeeTxSize(outputValues, txFee); + isInTolerance = isInTolerance(estimatedTxSize, realTxSize, 0.2); + if (!isInTolerance) { + estimatedTxSize = realTxSize; + } + counter++; + } + while (!isInTolerance && counter < MAX_ITERATIONS); + if (!isInTolerance) { + log.warn("We could not find a tx which satisfies our tolerance requirement of 20%. " + + "realTxSize={}, estimatedTxSize={}", + realTxSize, estimatedTxSize); + } + return estimatedTxSize; + } + + @VisibleForTesting + static boolean isInTolerance(int estimatedSize, int txSize, double tolerance) { + checkArgument(estimatedSize > 0, "estimatedSize must be positive"); + checkArgument(txSize > 0, "txSize must be positive"); + checkArgument(tolerance > 0, "tolerance must be positive"); + double deviation = Math.abs(1 - ((double) estimatedSize / (double) txSize)); + return deviation <= tolerance; + } +} 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 d00ac34d923..5760df2abf9 100644 --- a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java +++ b/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java @@ -957,6 +957,24 @@ private boolean feeEstimationNotSatisfied(int counter, Transaction tx) { tx.getFee().value - targetFee > 1000); } + public int getEstimatedFeeTxSize(List outputValues, Coin txFee) + throws InsufficientMoneyException, AddressFormatException { + Transaction transaction = new Transaction(params); + Address dummyAddress = wallet.currentReceiveKey().toAddress(params); + outputValues.forEach(outputValue -> transaction.addOutput(outputValue, dummyAddress)); + + SendRequest sendRequest = SendRequest.forTx(transaction); + sendRequest.shuffleOutputs = false; + sendRequest.aesKey = aesKey; + sendRequest.coinSelector = new BtcCoinSelector(walletsSetup.getAddressesByContext(AddressEntry.Context.AVAILABLE)); + sendRequest.fee = txFee; + sendRequest.feePerKb = Coin.ZERO; + sendRequest.ensureMinRequiredFee = false; + sendRequest.changeAddress = dummyAddress; + wallet.completeTx(sendRequest); + return transaction.bitcoinSerialize().length; + } + /////////////////////////////////////////////////////////////////////////////////////////// // Withdrawal Send 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 cc41d021959..69a5162568a 100644 --- a/core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java +++ b/core/src/main/java/bisq/core/btc/wallet/TradeWalletService.java @@ -230,37 +230,6 @@ public Transaction createBtcTradingFeeTx(Address fundingAddress, } } - public Transaction estimateBtcTradingFeeTxSize(Address fundingAddress, - Address reservedForTradeAddress, - Address changeAddress, - Coin reservedFundsForOffer, - boolean useSavingsWallet, - Coin tradingFee, - Coin txFee, - String feeReceiverAddresses) - throws InsufficientMoneyException, AddressFormatException { - Transaction tradingFeeTx = new Transaction(params); - tradingFeeTx.addOutput(tradingFee, Address.fromBase58(params, feeReceiverAddresses)); - tradingFeeTx.addOutput(reservedFundsForOffer, reservedForTradeAddress); - - SendRequest sendRequest = SendRequest.forTx(tradingFeeTx); - sendRequest.shuffleOutputs = false; - sendRequest.aesKey = aesKey; - if (useSavingsWallet) - sendRequest.coinSelector = new BtcCoinSelector(walletsSetup.getAddressesByContext(AddressEntry.Context.AVAILABLE)); - else - sendRequest.coinSelector = new BtcCoinSelector(fundingAddress); - - sendRequest.fee = txFee; - sendRequest.feePerKb = Coin.ZERO; - sendRequest.ensureMinRequiredFee = false; - sendRequest.changeAddress = changeAddress; - checkNotNull(wallet, "Wallet must not be null"); - log.info("estimateBtcTradingFeeTxSize"); - wallet.completeTx(sendRequest); - return tradingFeeTx; - } - public Transaction completeBsqTradingFeeTx(Transaction preparedBsqTx, Address fundingAddress, Address reservedForTradeAddress, diff --git a/core/src/test/java/bisq/core/btc/TxFeeEstimationServiceTest.java b/core/src/test/java/bisq/core/btc/TxFeeEstimationServiceTest.java new file mode 100644 index 00000000000..2f9d6b36d41 --- /dev/null +++ b/core/src/test/java/bisq/core/btc/TxFeeEstimationServiceTest.java @@ -0,0 +1,146 @@ +/* + * 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 . + */ + +package bisq.core.btc; + +import bisq.core.btc.wallet.BtcWalletService; + +import org.bitcoinj.core.Coin; +import org.bitcoinj.core.InsufficientMoneyException; + +import java.util.List; + +import org.powermock.core.classloader.annotations.PowerMockIgnore; +import org.powermock.core.classloader.annotations.PrepareForTest; + +import org.junit.Ignore; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +@PrepareForTest(BtcWalletService.class) +@PowerMockIgnore({"com.sun.org.apache.xerces.*", "javax.xml.*", "org.xml.*"}) +public class TxFeeEstimationServiceTest { + + @Test + public void testGetEstimatedTxSize_withDefaultTxSize() throws InsufficientMoneyException { + List outputValues = List.of(Coin.valueOf(2000), Coin.valueOf(3000)); + int initialEstimatedTxSize; + Coin txFeePerByte; + BtcWalletService btcWalletService = mock(BtcWalletService.class); + int result; + int realTxSize; + Coin txFee; + + initialEstimatedTxSize = 260; + txFeePerByte = Coin.valueOf(10); + realTxSize = 260; + + txFee = txFeePerByte.multiply(initialEstimatedTxSize); + when(btcWalletService.getEstimatedFeeTxSize(outputValues, txFee)).thenReturn(realTxSize); + result = TxFeeEstimationService.getEstimatedTxSize(outputValues, initialEstimatedTxSize, txFeePerByte, btcWalletService); + assertEquals(260, result); + } + + // FIXME @Bernard could you have a look? + @Test + @Ignore + public void testGetEstimatedTxSize_withLargeTx() throws InsufficientMoneyException { + List outputValues = List.of(Coin.valueOf(2000), Coin.valueOf(3000)); + int initialEstimatedTxSize; + Coin txFeePerByte; + BtcWalletService btcWalletService = mock(BtcWalletService.class); + int result; + int realTxSize; + Coin txFee; + + initialEstimatedTxSize = 260; + txFeePerByte = Coin.valueOf(10); + realTxSize = 2600; + + txFee = txFeePerByte.multiply(initialEstimatedTxSize); + when(btcWalletService.getEstimatedFeeTxSize(outputValues, txFee)).thenReturn(realTxSize); + + // repeated calls to getEstimatedFeeTxSize do not work (returns 0 at second call in loop which cause test to fail) + result = TxFeeEstimationService.getEstimatedTxSize(outputValues, initialEstimatedTxSize, txFeePerByte, btcWalletService); + assertEquals(2600, result); + } + + // FIXME @Bernard could you have a look? + @Test + @Ignore + public void testGetEstimatedTxSize_withSmallTx() throws InsufficientMoneyException { + List outputValues = List.of(Coin.valueOf(2000), Coin.valueOf(3000)); + int initialEstimatedTxSize; + Coin txFeePerByte; + BtcWalletService btcWalletService = mock(BtcWalletService.class); + int result; + int realTxSize; + Coin txFee; + + initialEstimatedTxSize = 2600; + txFeePerByte = Coin.valueOf(10); + realTxSize = 260; + + txFee = txFeePerByte.multiply(initialEstimatedTxSize); + when(btcWalletService.getEstimatedFeeTxSize(outputValues, txFee)).thenReturn(realTxSize); + result = TxFeeEstimationService.getEstimatedTxSize(outputValues, initialEstimatedTxSize, txFeePerByte, btcWalletService); + assertEquals(260, result); + } + + @Test + public void testIsInTolerance() { + int estimatedSize; + int txSize; + double tolerance; + boolean result; + + estimatedSize = 100; + txSize = 100; + tolerance = 0.0001; + result = TxFeeEstimationService.isInTolerance(estimatedSize, txSize, tolerance); + assertTrue(result); + + estimatedSize = 100; + txSize = 200; + tolerance = 0.2; + result = TxFeeEstimationService.isInTolerance(estimatedSize, txSize, tolerance); + assertFalse(result); + + estimatedSize = 120; + txSize = 100; + tolerance = 0.2; + result = TxFeeEstimationService.isInTolerance(estimatedSize, txSize, tolerance); + assertTrue(result); + + estimatedSize = 200; + txSize = 100; + tolerance = 1; + result = TxFeeEstimationService.isInTolerance(estimatedSize, txSize, tolerance); + assertTrue(result); + + estimatedSize = 201; + txSize = 100; + tolerance = 1; + result = TxFeeEstimationService.isInTolerance(estimatedSize, txSize, tolerance); + assertFalse(result); + } +} diff --git a/desktop/src/main/java/bisq/desktop/main/offer/MutableOfferDataModel.java b/desktop/src/main/java/bisq/desktop/main/offer/MutableOfferDataModel.java index 755f4aa3a8e..afb033c5911 100644 --- a/desktop/src/main/java/bisq/desktop/main/offer/MutableOfferDataModel.java +++ b/desktop/src/main/java/bisq/desktop/main/offer/MutableOfferDataModel.java @@ -19,13 +19,13 @@ import bisq.core.app.BisqEnvironment; import bisq.core.arbitration.Arbitrator; +import bisq.core.btc.TxFeeEstimationService; import bisq.core.btc.listeners.BalanceListener; import bisq.core.btc.listeners.BsqBalanceListener; import bisq.core.btc.model.AddressEntry; import bisq.core.btc.wallet.BsqWalletService; import bisq.core.btc.wallet.BtcWalletService; import bisq.core.btc.wallet.Restrictions; -import bisq.core.btc.wallet.TradeWalletService; import bisq.core.filter.FilterManager; import bisq.core.locale.CurrencyUtil; import bisq.core.locale.Res; @@ -58,11 +58,10 @@ import bisq.common.app.Version; import bisq.common.crypto.KeyRing; +import bisq.common.util.Tuple2; import bisq.common.util.Utilities; -import org.bitcoinj.core.Address; import org.bitcoinj.core.Coin; -import org.bitcoinj.core.InsufficientMoneyException; import org.bitcoinj.core.Transaction; import com.google.inject.Inject; @@ -106,8 +105,8 @@ public abstract class MutableOfferDataModel extends OfferDataModel implements Bs final String shortOfferId; private final FilterManager filterManager; private final AccountAgeWitnessService accountAgeWitnessService; - private final TradeWalletService tradeWalletService; private final FeeService feeService; + private final TxFeeEstimationService txFeeEstimationService; private final ReferralIdService referralIdService; private final BSFormatter btcFormatter; private final String offerId; @@ -137,8 +136,7 @@ public abstract class MutableOfferDataModel extends OfferDataModel implements Bs protected double marketPriceMargin = 0; private Coin txFeeFromFeeService = Coin.ZERO; private boolean marketPriceAvailable; - private int feeTxSize = 260; // size of typical tx with 1 input - private int feeTxSizeEstimationRecursionCounter; + private int feeTxSize = TxFeeEstimationService.TYPICAL_TX_WITH_1_INPUT_SIZE; protected boolean allowAmountUpdate = true; @@ -147,11 +145,19 @@ public abstract class MutableOfferDataModel extends OfferDataModel implements Bs /////////////////////////////////////////////////////////////////////////////////////////// @Inject - public MutableOfferDataModel(OpenOfferManager openOfferManager, BtcWalletService btcWalletService, BsqWalletService bsqWalletService, - Preferences preferences, User user, KeyRing keyRing, P2PService p2PService, - PriceFeedService priceFeedService, FilterManager filterManager, - AccountAgeWitnessService accountAgeWitnessService, TradeWalletService tradeWalletService, - FeeService feeService, ReferralIdService referralIdService, + public MutableOfferDataModel(OpenOfferManager openOfferManager, + BtcWalletService btcWalletService, + BsqWalletService bsqWalletService, + Preferences preferences, + User user, + KeyRing keyRing, + P2PService p2PService, + PriceFeedService priceFeedService, + FilterManager filterManager, + AccountAgeWitnessService accountAgeWitnessService, + FeeService feeService, + TxFeeEstimationService txFeeEstimationService, + ReferralIdService referralIdService, BSFormatter btcFormatter) { super(btcWalletService); @@ -164,8 +170,8 @@ public MutableOfferDataModel(OpenOfferManager openOfferManager, BtcWalletService this.priceFeedService = priceFeedService; this.filterManager = filterManager; this.accountAgeWitnessService = accountAgeWitnessService; - this.tradeWalletService = tradeWalletService; this.feeService = feeService; + this.txFeeEstimationService = txFeeEstimationService; this.referralIdService = referralIdService; this.btcFormatter = btcFormatter; @@ -438,57 +444,14 @@ Offer createAndGetOffer() { // This works only if we have already funds in the wallet public void estimateTxSize() { - txFeeFromFeeService = feeService.getTxFee(feeTxSize); - Address fundingAddress = btcWalletService.getFreshAddressEntry().getAddress(); - Address reservedForTradeAddress = btcWalletService.getOrCreateAddressEntry(offerId, AddressEntry.Context.RESERVED_FOR_TRADE).getAddress(); - Address changeAddress = btcWalletService.getFreshAddressEntry().getAddress(); - Coin reservedFundsForOffer = getSecurityDeposit(); if (!isBuyOffer()) reservedFundsForOffer = reservedFundsForOffer.add(amount.get()); - checkNotNull(user.getAcceptedArbitrators(), "user.getAcceptedArbitrators() must not be null"); - checkArgument(!user.getAcceptedArbitrators().isEmpty(), "user.getAcceptedArbitrators() must not be empty"); - String dummyArbitratorAddress = user.getAcceptedArbitrators().get(0).getBtcAddress(); - try { - log.info("We create a dummy tx to see if our estimated size is in the accepted range. feeTxSize={}," + - " txFee based on feeTxSize: {}, recommended txFee is {} sat/byte", - feeTxSize, txFeeFromFeeService.toFriendlyString(), feeService.getTxFeePerByte()); - Transaction tradeFeeTx = tradeWalletService.estimateBtcTradingFeeTxSize( - fundingAddress, - reservedForTradeAddress, - changeAddress, - reservedFundsForOffer, - true, - getMakerFee(), - txFeeFromFeeService, - dummyArbitratorAddress); - - final int txSize = tradeFeeTx.bitcoinSerialize().length; - // use feeTxSizeEstimationRecursionCounter to avoid risk for endless loop - if (txSize > feeTxSize * 1.2 && feeTxSizeEstimationRecursionCounter < 10) { - feeTxSizeEstimationRecursionCounter++; - log.info("txSize is {} bytes but feeTxSize used for txFee calculation was {} bytes. We try again with an " + - "adjusted txFee to reach the target tx fee.", txSize, feeTxSize); - feeTxSize = txSize; - txFeeFromFeeService = feeService.getTxFee(feeTxSize); - // lets try again with the adjusted txSize and fee. - estimateTxSize(); - } else { - log.info("feeTxSize {} bytes", feeTxSize); - log.info("txFee based on estimated size: {}, recommended txFee is {} sat/byte", - txFeeFromFeeService.toFriendlyString(), feeService.getTxFeePerByte()); - } - } catch (InsufficientMoneyException e) { - // If we need to fund from an external wallet we can assume we only have 1 input (260 bytes). - log.warn("We cannot do the fee estimation because there are not enough funds in the wallet. This is expected " + - "if the user pays from an external wallet. In that case we use an estimated tx size of 260 bytes."); - feeTxSize = 260; - txFeeFromFeeService = feeService.getTxFee(feeTxSize); - log.info("feeTxSize {} bytes", feeTxSize); - log.info("txFee based on estimated size: {}, recommended txFee is {} sat/byte", - txFeeFromFeeService.toFriendlyString(), feeService.getTxFeePerByte()); - } + Tuple2 estimatedFeeAndTxSize = txFeeEstimationService.getEstimatedFeeAndTxSizeForMaker(reservedFundsForOffer, + getMakerFee()); + txFeeFromFeeService = estimatedFeeAndTxSize.first; + feeTxSize = estimatedFeeAndTxSize.second; } void onPlaceOffer(Offer offer, TransactionResultHandler resultHandler) { diff --git a/desktop/src/main/java/bisq/desktop/main/offer/createoffer/CreateOfferDataModel.java b/desktop/src/main/java/bisq/desktop/main/offer/createoffer/CreateOfferDataModel.java index b3f3dd1e2ae..21286166366 100644 --- a/desktop/src/main/java/bisq/desktop/main/offer/createoffer/CreateOfferDataModel.java +++ b/desktop/src/main/java/bisq/desktop/main/offer/createoffer/CreateOfferDataModel.java @@ -23,9 +23,9 @@ import bisq.desktop.main.offer.MutableOfferDataModel; +import bisq.core.btc.TxFeeEstimationService; import bisq.core.btc.wallet.BsqWalletService; import bisq.core.btc.wallet.BtcWalletService; -import bisq.core.btc.wallet.TradeWalletService; import bisq.core.filter.FilterManager; import bisq.core.offer.OpenOfferManager; import bisq.core.payment.AccountAgeWitnessService; @@ -60,8 +60,8 @@ public CreateOfferDataModel(OpenOfferManager openOfferManager, PriceFeedService priceFeedService, FilterManager filterManager, AccountAgeWitnessService accountAgeWitnessService, - TradeWalletService tradeWalletService, FeeService feeService, + TxFeeEstimationService txFeeEstimationService, ReferralIdService referralIdService, BSFormatter btcFormatter) { super(openOfferManager, @@ -74,8 +74,8 @@ public CreateOfferDataModel(OpenOfferManager openOfferManager, priceFeedService, filterManager, accountAgeWitnessService, - tradeWalletService, feeService, + txFeeEstimationService, referralIdService, btcFormatter); } diff --git a/desktop/src/main/java/bisq/desktop/main/offer/takeoffer/TakeOfferDataModel.java b/desktop/src/main/java/bisq/desktop/main/offer/takeoffer/TakeOfferDataModel.java index 64a35a7995e..f6dd5544374 100644 --- a/desktop/src/main/java/bisq/desktop/main/offer/takeoffer/TakeOfferDataModel.java +++ b/desktop/src/main/java/bisq/desktop/main/offer/takeoffer/TakeOfferDataModel.java @@ -21,12 +21,12 @@ import bisq.desktop.main.overlays.popups.Popup; import bisq.core.arbitration.Arbitrator; +import bisq.core.btc.TxFeeEstimationService; import bisq.core.btc.listeners.BalanceListener; import bisq.core.btc.model.AddressEntry; import bisq.core.btc.wallet.BsqWalletService; import bisq.core.btc.wallet.BtcWalletService; import bisq.core.btc.wallet.Restrictions; -import bisq.core.btc.wallet.TradeWalletService; import bisq.core.filter.FilterManager; import bisq.core.locale.CurrencyUtil; import bisq.core.locale.Res; @@ -48,9 +48,9 @@ import bisq.core.user.User; import bisq.core.util.CoinUtil; -import org.bitcoinj.core.Address; +import bisq.common.util.Tuple2; + import org.bitcoinj.core.Coin; -import org.bitcoinj.core.InsufficientMoneyException; import org.bitcoinj.core.Transaction; import org.bitcoinj.wallet.Wallet; @@ -65,6 +65,8 @@ import java.util.List; import java.util.Set; +import org.jetbrains.annotations.NotNull; + import javax.annotation.Nullable; import static com.google.common.base.Preconditions.checkArgument; @@ -82,8 +84,8 @@ class TakeOfferDataModel extends OfferDataModel { private final FeeService feeService; private final FilterManager filterManager; private final Preferences preferences; + private final TxFeeEstimationService txFeeEstimationService; private final PriceFeedService priceFeedService; - private final TradeWalletService tradeWalletService; private final AccountAgeWitnessService accountAgeWitnessService; private Coin txFeeFromFeeService; @@ -103,7 +105,6 @@ class TakeOfferDataModel extends OfferDataModel { Price tradePrice; // 260 kb is size of typical trade fee tx with 1 input but trade tx (deposit and payout) are larger so we adjust to 320 private int feeTxSize = 320; - private int feeTxSizeEstimationRecursionCounter; private boolean freezeFee; private Coin txFeePerByteFromFeeService; @@ -115,9 +116,13 @@ class TakeOfferDataModel extends OfferDataModel { @Inject TakeOfferDataModel(TradeManager tradeManager, - BtcWalletService btcWalletService, BsqWalletService bsqWalletService, - User user, FeeService feeService, FilterManager filterManager, - Preferences preferences, PriceFeedService priceFeedService, TradeWalletService tradeWalletService, + BtcWalletService btcWalletService, + BsqWalletService bsqWalletService, + User user, FeeService feeService, + FilterManager filterManager, + Preferences preferences, + TxFeeEstimationService txFeeEstimationService, + PriceFeedService priceFeedService, AccountAgeWitnessService accountAgeWitnessService) { super(btcWalletService); @@ -127,8 +132,8 @@ class TakeOfferDataModel extends OfferDataModel { this.feeService = feeService; this.filterManager = filterManager; this.preferences = preferences; + this.txFeeEstimationService = txFeeEstimationService; this.priceFeedService = priceFeedService; - this.tradeWalletService = tradeWalletService; this.accountAgeWitnessService = accountAgeWitnessService; // isMainNet.set(preferences.getBaseCryptoNetwork() == BitcoinNetwork.BTC_MAINNET); @@ -287,7 +292,7 @@ void onTakeOffer(TradeResultHandler tradeResultHandler) { checkNotNull(txFeeFromFeeService, "txFeeFromFeeService must not be null"); checkNotNull(getTakerFee(), "takerFee must not be null"); - Coin fundsNeededForTrade = getSecurityDeposit().add(txFeeFromFeeService).add(txFeeFromFeeService); + Coin fundsNeededForTrade = getFundsNeededForTrade(); if (isBuyOffer()) fundsNeededForTrade = fundsNeededForTrade.add(amount.get()); @@ -328,81 +333,37 @@ void onTakeOffer(TradeResultHandler tradeResultHandler) { // leading to a smaller tx and too high fees. Simply updating the fee estimation would lead to changed required funds // and if funds get higher (if tx get larger) the user would get confused (adding small inputs would increase total required funds). // So that would require more thoughts how to deal with all those cases. - private void estimateTxSize() { - Address fundingAddress = btcWalletService.getFreshAddressEntry().getAddress(); + public void estimateTxSize() { int txSize = 0; if (btcWalletService.getBalance(Wallet.BalanceType.AVAILABLE).isPositive()) { - txFeeFromFeeService = getTxFeeBySize(feeTxSize); - - Address reservedForTradeAddress = btcWalletService.getOrCreateAddressEntry(offer.getId(), AddressEntry.Context.RESERVED_FOR_TRADE).getAddress(); - Address changeAddress = btcWalletService.getFreshAddressEntry().getAddress(); - - Coin reservedFundsForOffer = getSecurityDeposit().add(txFeeFromFeeService).add(txFeeFromFeeService); + Coin fundsNeededForTrade = getFundsNeededForTrade(); if (isBuyOffer()) - reservedFundsForOffer = reservedFundsForOffer.add(amount.get()); - - checkNotNull(user.getAcceptedArbitrators(), "user.getAcceptedArbitrators() must not be null"); - checkArgument(!user.getAcceptedArbitrators().isEmpty(), "user.getAcceptedArbitrators() must not be empty"); - String dummyArbitratorAddress = user.getAcceptedArbitrators().get(0).getBtcAddress(); - try { - log.debug("We create a dummy tx to see if our estimated size is in the accepted range. feeTxSize={}," + - " txFee based on feeTxSize: {}, recommended txFee is {} sat/byte", - feeTxSize, txFeeFromFeeService.toFriendlyString(), feeService.getTxFeePerByte()); - Transaction tradeFeeTx = tradeWalletService.estimateBtcTradingFeeTxSize( - fundingAddress, - reservedForTradeAddress, - changeAddress, - reservedFundsForOffer, - true, - getTakerFee(), - txFeeFromFeeService, - dummyArbitratorAddress); - - txSize = tradeFeeTx.bitcoinSerialize().length; - // use feeTxSizeEstimationRecursionCounter to avoid risk for endless loop - // We use the tx size for the trade fee tx as target for the fees. - // The deposit and payout txs are determined +/- 1 output but the trade fee tx can have either 1 or many inputs - // so we need to make sure the trade fee tx gets the correct fee to not get stuck. - // We use a 20% tolerance frm out default 320 byte size (typical for deposit and payout) and only if we get a - // larger size we increase the fee. Worst case is that we overpay for the other follow up txs, but better than - // use a too low fee and get stuck. - if (txSize > feeTxSize * 1.2 && feeTxSizeEstimationRecursionCounter < 10) { - feeTxSizeEstimationRecursionCounter++; - log.info("txSize is {} bytes but feeTxSize used for txFee calculation was {} bytes. We try again with an " + - "adjusted txFee to reach the target tx fee.", txSize, feeTxSize); - - feeTxSize = txSize; - txFeeFromFeeService = getTxFeeBySize(txSize); - - // lets try again with the adjusted txSize and fee. - estimateTxSize(); - } else { - // We are done with estimation iterations - if (feeTxSizeEstimationRecursionCounter < 10) - log.info("Fee estimation completed:\n" + - "txFee based on estimated size of {} bytes. Average tx size = {} bytes. Actual tx size = {} bytes. TxFee is {} ({} sat/byte)", - feeTxSize, getAverageSize(feeTxSize), txSize, txFeeFromFeeService.toFriendlyString(), feeService.getTxFeePerByte()); - else - log.warn("We could not estimate the fee as the feeTxSizeEstimationRecursionCounter exceeded our limit of 10 recursions.\n" + - "txFee based on estimated size of {} bytes. Average tx size = {} bytes. Actual tx size = {} bytes. " + - "TxFee is {} ({} sat/byte)", - feeTxSize, getAverageSize(feeTxSize), txSize, txFeeFromFeeService.toFriendlyString(), feeService.getTxFeePerByte()); - } - } catch (InsufficientMoneyException e) { - log.info("We cannot complete the fee estimation because there are not enough funds in the wallet.\n" + - "This is expected if the user has not sufficient funds yet.\n" + - "In that case we use the latest estimated tx size or the default if none has been calculated yet.\n" + - "txFee based on estimated size of {} bytes. Average tx size = {} bytes. Actual tx size = {} bytes. TxFee is {} ({} sat/byte)", - feeTxSize, getAverageSize(feeTxSize), txSize, txFeeFromFeeService.toFriendlyString(), feeService.getTxFeePerByte()); - } + fundsNeededForTrade = fundsNeededForTrade.add(amount.get()); + + // As taker we pay 3 times the fee and currently the fee is the same for all 3 txs (trade fee tx, deposit + // tx and payout tx). + // We should try to change that in future to have the deposit and payout tx with a fixed fee as the size is + // there more deterministic. + // The trade fee tx can be in the worst case very large if there are many inputs so if we take that tx alone + // for the fee estimation we would overpay a lot. + // On the other side if we have the best case of a 1 input tx fee tx then it is only 260 bytes but the + // other 2 txs are larger (320 and 380 bytes) and would get a lower fee/byte as intended. + // We apply following model to not overpay too much but be on the safe side as well. + // We sum the taker fee tx and the deposit tx together as it can be assumed that both be in the same block and + // as they are dependent txs the miner will pick both if the fee in total is good enough. + // We make sure that the fee is sufficient to meet our intended fee/byte for the larger payout tx with 380 bytes. + Tuple2 estimatedFeeAndTxSize = txFeeEstimationService.getEstimatedFeeAndTxSizeForTaker(fundsNeededForTrade, + getTakerFee()); + txFeeFromFeeService = estimatedFeeAndTxSize.first; + feeTxSize = estimatedFeeAndTxSize.second; } else { - feeTxSize = 320; - txFeeFromFeeService = getTxFeeBySize(feeTxSize); + feeTxSize = 380; + txFeeFromFeeService = txFeePerByteFromFeeService.multiply(feeTxSize); log.info("We cannot do the fee estimation because there are no funds in the wallet.\nThis is expected " + "if the user has not funded his wallet yet.\n" + - "In that case we use an estimated tx size of 320 bytes.\n" + - "txFee based on estimated size of {} bytes. Average tx size = {} bytes. Actual tx size = {} bytes. TxFee is {} ({} sat/byte)", - feeTxSize, getAverageSize(feeTxSize), txSize, txFeeFromFeeService.toFriendlyString(), feeService.getTxFeePerByte()); + "In that case we use an estimated tx size of 380 bytes.\n" + + "txFee based on estimated size of {} bytes. feeTxSize = {} bytes. Actual tx size = {} bytes. TxFee is {} ({} sat/byte)", + feeTxSize, feeTxSize, txSize, txFeeFromFeeService.toFriendlyString(), feeService.getTxFeePerByte()); } } @@ -620,10 +581,32 @@ public String getCurrencyNameAndCode() { } public Coin getTotalTxFee() { + Coin totalTxFees = txFeeFromFeeService.add(getTxFeeForDepositTx()).add(getTxFeeForPayoutTx()); if (isCurrencyForTakerFeeBtc()) - return txFeeFromFeeService.multiply(3); + return totalTxFees; else - return txFeeFromFeeService.multiply(3).subtract(getTakerFee() != null ? getTakerFee() : Coin.ZERO); + return totalTxFees.subtract(getTakerFee() != null ? getTakerFee() : Coin.ZERO); + } + + @NotNull + private Coin getFundsNeededForTrade() { + return getSecurityDeposit().add(getTxFeeForDepositTx()).add(getTxFeeForPayoutTx()); + } + + private Coin getTxFeeForDepositTx() { + //TODO fix with new trade protocol! + // Unfortunately we cannot change that to the correct fees as it would break backward compatibility + // We still might find a way with offer version or app version checks so lets keep that commented out + // code as that shows how it should be. + return txFeeFromFeeService; //feeService.getTxFee(320); + } + + private Coin getTxFeeForPayoutTx() { + //TODO fix with new trade protocol! + // Unfortunately we cannot change that to the correct fees as it would break backward compatibility + // We still might find a way with offer version or app version checks so lets keep that commented out + // code as that shows how it should be. + return txFeeFromFeeService; //feeService.getTxFee(380); } public AddressEntry getAddressEntry() { diff --git a/desktop/src/main/java/bisq/desktop/main/portfolio/editoffer/EditOfferDataModel.java b/desktop/src/main/java/bisq/desktop/main/portfolio/editoffer/EditOfferDataModel.java index 2054869aa06..c6cbc56f3fa 100644 --- a/desktop/src/main/java/bisq/desktop/main/portfolio/editoffer/EditOfferDataModel.java +++ b/desktop/src/main/java/bisq/desktop/main/portfolio/editoffer/EditOfferDataModel.java @@ -20,9 +20,9 @@ import bisq.desktop.main.offer.MutableOfferDataModel; +import bisq.core.btc.TxFeeEstimationService; import bisq.core.btc.wallet.BsqWalletService; import bisq.core.btc.wallet.BtcWalletService; -import bisq.core.btc.wallet.TradeWalletService; import bisq.core.filter.FilterManager; import bisq.core.locale.CurrencyUtil; import bisq.core.locale.TradeCurrency; @@ -67,8 +67,8 @@ class EditOfferDataModel extends MutableOfferDataModel { PriceFeedService priceFeedService, FilterManager filterManager, AccountAgeWitnessService accountAgeWitnessService, - TradeWalletService tradeWalletService, FeeService feeService, + TxFeeEstimationService txFeeEstimationService, ReferralIdService referralIdService, BSFormatter btcFormatter, CorePersistenceProtoResolver corePersistenceProtoResolver) { @@ -82,8 +82,8 @@ class EditOfferDataModel extends MutableOfferDataModel { priceFeedService, filterManager, accountAgeWitnessService, - tradeWalletService, feeService, + txFeeEstimationService, referralIdService, btcFormatter); this.corePersistenceProtoResolver = corePersistenceProtoResolver; diff --git a/desktop/src/test/java/bisq/desktop/main/offer/createoffer/CreateOfferViewModelTest.java b/desktop/src/test/java/bisq/desktop/main/offer/createoffer/CreateOfferViewModelTest.java index d38f32ed5cc..d63836a444c 100644 --- a/desktop/src/test/java/bisq/desktop/main/offer/createoffer/CreateOfferViewModelTest.java +++ b/desktop/src/test/java/bisq/desktop/main/offer/createoffer/CreateOfferViewModelTest.java @@ -22,6 +22,7 @@ import bisq.desktop.util.validation.FiatPriceValidator; import bisq.desktop.util.validation.SecurityDepositValidator; +import bisq.core.btc.TxFeeEstimationService; import bisq.core.btc.model.AddressEntry; import bisq.core.btc.wallet.BsqWalletService; import bisq.core.btc.wallet.BtcWalletService; @@ -70,7 +71,7 @@ @PrepareForTest({BtcWalletService.class, AddressEntry.class, PriceFeedService.class, User.class, FeeService.class, CreateOfferDataModel.class, PaymentAccount.class, BsqWalletService.class, SecurityDepositValidator.class, AccountAgeWitnessService.class, BsqFormatter.class, Preferences.class, - BsqWalletService.class}) + BsqWalletService.class, TxFeeEstimationService.class}) @PowerMockIgnore({"com.sun.org.apache.xerces.*", "javax.xml.*", "org.xml.*"}) public class CreateOfferViewModelTest { @@ -98,6 +99,7 @@ public void setUp() { BsqWalletService bsqWalletService = mock(BsqWalletService.class); SecurityDepositValidator securityDepositValidator = mock(SecurityDepositValidator.class); AccountAgeWitnessService accountAgeWitnessService = mock(AccountAgeWitnessService.class); + TxFeeEstimationService txFeeEstimationService = mock(TxFeeEstimationService.class); when(btcWalletService.getOrCreateAddressEntry(anyString(), any())).thenReturn(addressEntry); when(btcWalletService.getBalanceForAddress(any())).thenReturn(Coin.valueOf(1000L)); @@ -112,7 +114,7 @@ public void setUp() { when(bsqFormatter.formatCoin(any())).thenReturn("0"); when(bsqWalletService.getAvailableBalance()).thenReturn(Coin.ZERO); - CreateOfferDataModel dataModel = new CreateOfferDataModel(null, btcWalletService, bsqWalletService, empty, user, null, null, priceFeedService, null, accountAgeWitnessService, null, feeService, null, bsFormatter); + CreateOfferDataModel dataModel = new CreateOfferDataModel(null, btcWalletService, bsqWalletService, empty, user, null, null, priceFeedService, null, accountAgeWitnessService, feeService, txFeeEstimationService, null, bsFormatter); dataModel.initWithData(OfferPayload.Direction.BUY, new CryptoCurrency("BTC", "bitcoin")); dataModel.activate();