From 6bba6a526f0404701cce7c33a1a75961e64df2f1 Mon Sep 17 00:00:00 2001 From: Oscar Guindzberg Date: Mon, 26 Oct 2020 18:36:45 -0300 Subject: [PATCH 1/8] Use bitcoinj 0.15.8 (commit 60b4f2f) --- build.gradle | 2 +- core/src/main/java/bisq/core/app/WalletAppSetup.java | 2 +- gradle/witness/gradle-witness.gradle | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/build.gradle b/build.gradle index fd827538fb..2b7c2853ef 100644 --- a/build.gradle +++ b/build.gradle @@ -28,7 +28,7 @@ configure(subprojects) { ext { // in alphabetical order bcVersion = '1.63' - bitcoinjVersion = 'fcec3da' + bitcoinjVersion = '60b4f2f' btcdCli4jVersion = '27b94333' codecVersion = '1.13' easybindVersion = '1.0.3' diff --git a/core/src/main/java/bisq/core/app/WalletAppSetup.java b/core/src/main/java/bisq/core/app/WalletAppSetup.java index bccd033ee0..9851996a7d 100644 --- a/core/src/main/java/bisq/core/app/WalletAppSetup.java +++ b/core/src/main/java/bisq/core/app/WalletAppSetup.java @@ -107,7 +107,7 @@ public class WalletAppSetup { Runnable downloadCompleteHandler, Runnable walletInitializedHandler) { log.info("Initialize WalletAppSetup with BitcoinJ version {} and hash of BitcoinJ commit {}", - VersionMessage.BITCOINJ_VERSION, "fcec3da"); + VersionMessage.BITCOINJ_VERSION, "60b4f2f"); ObjectProperty walletServiceException = new SimpleObjectProperty<>(); btcInfoBinding = EasyBind.combine(walletsSetup.downloadPercentageProperty(), diff --git a/gradle/witness/gradle-witness.gradle b/gradle/witness/gradle-witness.gradle index cecaf6c8b7..339f6bb106 100644 --- a/gradle/witness/gradle-witness.gradle +++ b/gradle/witness/gradle-witness.gradle @@ -23,7 +23,7 @@ dependencyVerification { 'com.github.bisq-network.netlayer:tor.external:a3606a592d6b6caa6a2fb7db224eaf43c6874c6730da4815bd37ad686b283dcb', 'com.github.bisq-network.netlayer:tor.native:b15aba7fe987185037791c7ec7c529cb001b90d723d047d54aab87aceb3b3d45', 'com.github.bisq-network.netlayer:tor:a974190aa3a031067ccd1dda28a3ae58cad14060792299d86ea38a05fb21afc5', - 'com.github.bisq-network.bitcoinj:bitcoinj-core:8af7faa2155feff5afd1fa0fcea6fe7f7fa0d7ee977bdc648d1e73f3dcf2c754', + 'com.github.bisq-network:bitcoinj:804f587a44b1ce9cd9b8dd1848fc911329634163b7905bafb86f502a3d08264c', 'com.github.cd2357.tor-binary:tor-binary-geoip:ae27b6aca1a3a50a046eb11e38202b6d21c2fcd2b8643bbeb5ea85e065fbc1be', 'com.github.cd2357.tor-binary:tor-binary-linux32:7b5d6770aa442ef6d235e8a9bfbaa7c62560690f9fe69ff03c7a752eae84f7dc', 'com.github.cd2357.tor-binary:tor-binary-linux64:24111fa35027599a750b0176392dc1e9417d919414396d1b221ac2e707eaba76', From b2023e236692e6ad77afd6f9379de4e275393928 Mon Sep 17 00:00:00 2001 From: Oscar Guindzberg Date: Fri, 16 Oct 2020 17:03:34 -0300 Subject: [PATCH 2/8] Use tx.getVsize() Replace tx.bitcoinSerialize().length --- .../core/btc/wallet/BtcWalletService.java | 20 +++++++++---------- .../blindvote/MyBlindVoteListService.java | 2 +- .../bond/lockup/LockupTxService.java | 2 +- .../bond/unlock/UnlockTxService.java | 2 +- .../dao/burnbsq/assetfee/AssetFeeView.java | 2 +- .../burnbsq/proofofburn/ProofOfBurnView.java | 2 +- .../dao/governance/make/MakeProposalView.java | 2 +- .../main/dao/wallet/send/BsqSendView.java | 4 ++-- .../main/funds/withdrawal/WithdrawalView.java | 2 +- .../steps/buyer/BuyerStep4View.java | 2 +- 10 files changed, 20 insertions(+), 20 deletions(-) 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 2ab2e64e71..fdbc5cbda7 100644 --- a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java +++ b/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java @@ -263,7 +263,7 @@ public class BtcWalletService extends WalletService { resultTx.addOutput(new TransactionOutput(params, resultTx, Coin.ZERO, ScriptBuilder.createOpReturnScript(opReturnData).getProgram())); numInputs = resultTx.getInputs().size(); - txSizeWithUnsignedInputs = resultTx.bitcoinSerialize().length; + txSizeWithUnsignedInputs = resultTx.getVsize(); long estimatedFeeAsLong = txFeePerByte.multiply(txSizeWithUnsignedInputs + sigSizePerInput * numInputs).value; // calculated fee must be inside of a tolerance range with tx fee isFeeOutsideTolerance = Math.abs(resultTx.getFee().value - estimatedFeeAsLong) > 1000; @@ -373,7 +373,7 @@ public class BtcWalletService extends WalletService { resultTx.addOutput(new TransactionOutput(params, resultTx, Coin.ZERO, ScriptBuilder.createOpReturnScript(opReturnData).getProgram())); numInputs = resultTx.getInputs().size(); - txSizeWithUnsignedInputs = resultTx.bitcoinSerialize().length; + txSizeWithUnsignedInputs = resultTx.getVsize(); final long estimatedFeeAsLong = txFeePerByte.multiply(txSizeWithUnsignedInputs + sigSizePerInput * numInputs).value; // calculated fee must be inside of a tolerance range with tx fee isFeeOutsideTolerance = Math.abs(resultTx.getFee().value - estimatedFeeAsLong) > 1000; @@ -529,14 +529,14 @@ public class BtcWalletService extends WalletService { resultTx.addOutput(new TransactionOutput(params, resultTx, Coin.ZERO, ScriptBuilder.createOpReturnScript(opReturnData).getProgram())); numInputs = resultTx.getInputs().size(); - txSizeWithUnsignedInputs = resultTx.bitcoinSerialize().length; + txSizeWithUnsignedInputs = resultTx.getVsize(); final long estimatedFeeAsLong = txFeePerByte.multiply(txSizeWithUnsignedInputs + sigSizePerInput * numInputs).value; // calculated fee must be inside of a tolerance range with tx fee isFeeOutsideTolerance = Math.abs(resultTx.getFee().value - estimatedFeeAsLong) > 1000; } while (opReturnIsOnlyOutput || isFeeOutsideTolerance || - resultTx.getFee().value < txFeePerByte.multiply(resultTx.bitcoinSerialize().length).value); + resultTx.getFee().value < txFeePerByte.multiply(resultTx.getVsize()).value); // Sign all BTC inputs signAllBtcInputs(preparedBsqTxInputs.size(), resultTx); @@ -809,7 +809,7 @@ public class BtcWalletService extends WalletService { ); log.info("newTransaction no. of inputs " + newTransaction.getInputs().size()); - log.info("newTransaction size in kB " + newTransaction.bitcoinSerialize().length / 1024); + log.info("newTransaction size in kB " + newTransaction.getVsize() / 1024); if (!newTransaction.getInputs().isEmpty()) { Coin amount = Coin.valueOf(newTransaction.getInputs().stream() @@ -839,7 +839,7 @@ public class BtcWalletService extends WalletService { sendRequest.changeAddress = toAddress; wallet.completeTx(sendRequest); tx = sendRequest.tx; - txSize = tx.bitcoinSerialize().length; + txSize = tx.getVsize(); printTx("FeeEstimationTransaction", tx); sendRequest.tx.getOutputs().forEach(o -> log.debug("Output value " + o.getValue().toFriendlyString())); } @@ -947,7 +947,7 @@ public class BtcWalletService extends WalletService { SendRequest sendRequest = getSendRequest(fromAddress, toAddress, amount, fee, aesKey, context); wallet.completeTx(sendRequest); tx = sendRequest.tx; - txSize = tx.bitcoinSerialize().length; + txSize = tx.getVsize(); printTx("FeeEstimationTransaction", tx); } while (feeEstimationNotSatisfied(counter, tx)); @@ -996,7 +996,7 @@ public class BtcWalletService extends WalletService { SendRequest sendRequest = getSendRequestForMultipleAddresses(fromAddresses, dummyReceiver, amount, fee, null, aesKey); wallet.completeTx(sendRequest); tx = sendRequest.tx; - txSize = tx.bitcoinSerialize().length; + txSize = tx.getVsize(); printTx("FeeEstimationTransactionForMultipleAddresses", tx); } while (feeEstimationNotSatisfied(counter, tx)); @@ -1012,7 +1012,7 @@ public class BtcWalletService extends WalletService { } private boolean feeEstimationNotSatisfied(int counter, Transaction tx) { - long targetFee = getTxFeeForWithdrawalPerByte().multiply(tx.bitcoinSerialize().length).value; + long targetFee = getTxFeeForWithdrawalPerByte().multiply(tx.getVsize()).value; return counter < 10 && (tx.getFee().value < targetFee || tx.getFee().value - targetFee > 1000); @@ -1034,7 +1034,7 @@ public class BtcWalletService extends WalletService { sendRequest.ensureMinRequiredFee = false; sendRequest.changeAddress = dummyAddress; wallet.completeTx(sendRequest); - return transaction.bitcoinSerialize().length; + return transaction.getVsize(); } diff --git a/core/src/main/java/bisq/core/dao/governance/blindvote/MyBlindVoteListService.java b/core/src/main/java/bisq/core/dao/governance/blindvote/MyBlindVoteListService.java index cee5d1b917..996ff207bc 100644 --- a/core/src/main/java/bisq/core/dao/governance/blindvote/MyBlindVoteListService.java +++ b/core/src/main/java/bisq/core/dao/governance/blindvote/MyBlindVoteListService.java @@ -195,7 +195,7 @@ public class MyBlindVoteListService implements PersistedDataHost, DaoStateListen Coin blindVoteFee = BlindVoteConsensus.getFee(daoStateService, daoStateService.getChainHeight()); Transaction dummyTx = getBlindVoteTx(stake, blindVoteFee, new byte[22]); Coin miningFee = dummyTx.getFee(); - int txSize = dummyTx.bitcoinSerialize().length; + int txSize = dummyTx.getVsize(); return new Tuple2<>(miningFee, txSize); } diff --git a/core/src/main/java/bisq/core/dao/governance/bond/lockup/LockupTxService.java b/core/src/main/java/bisq/core/dao/governance/bond/lockup/LockupTxService.java index 4ffa4772e9..9a61c09be8 100644 --- a/core/src/main/java/bisq/core/dao/governance/bond/lockup/LockupTxService.java +++ b/core/src/main/java/bisq/core/dao/governance/bond/lockup/LockupTxService.java @@ -95,7 +95,7 @@ public class LockupTxService { throws InsufficientMoneyException, WalletException, TransactionVerificationException, IOException { Transaction tx = getLockupTx(lockupAmount, lockTime, lockupReason, hash); Coin miningFee = tx.getFee(); - int txSize = tx.bitcoinSerialize().length; + int txSize = tx.getVsize(); return new Tuple2<>(miningFee, txSize); } diff --git a/core/src/main/java/bisq/core/dao/governance/bond/unlock/UnlockTxService.java b/core/src/main/java/bisq/core/dao/governance/bond/unlock/UnlockTxService.java index 0682186d2e..fa4272d7b5 100644 --- a/core/src/main/java/bisq/core/dao/governance/bond/unlock/UnlockTxService.java +++ b/core/src/main/java/bisq/core/dao/governance/bond/unlock/UnlockTxService.java @@ -93,7 +93,7 @@ public class UnlockTxService { throws InsufficientMoneyException, WalletException, TransactionVerificationException { Transaction tx = getUnlockTx(lockupTxId); Coin miningFee = tx.getFee(); - int txSize = tx.bitcoinSerialize().length; + int txSize = tx.getVsize(); return new Tuple2<>(miningFee, txSize); } 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 d873ed60d7..f2f2e9b14e 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 @@ -189,7 +189,7 @@ public class AssetFeeView extends ActivatableView implements Bsq try { Transaction transaction = assetService.payFee(selectedAsset, listingFee.value); Coin miningFee = transaction.getFee(); - int txSize = transaction.bitcoinSerialize().length; + int txSize = transaction.getVsize(); if (!DevEnv.isDevMode()) { GUIUtil.showBsqFeeInfoPopup(listingFee, miningFee, txSize, bsqFormatter, btcFormatter, 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 d9b5d7c281..b8ea8553b3 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 @@ -172,7 +172,7 @@ public class ProofOfBurnView extends ActivatableView implements String preImageAsString = preImageTextField.getText(); Transaction transaction = proofOfBurnService.burn(preImageAsString, amount.value); Coin miningFee = transaction.getFee(); - int txSize = transaction.bitcoinSerialize().length; + int txSize = transaction.getVsize(); if (!DevEnv.isDevMode()) { GUIUtil.showBsqFeeInfoPopup(amount, miningFee, txSize, bsqFormatter, btcFormatter, diff --git a/desktop/src/main/java/bisq/desktop/main/dao/governance/make/MakeProposalView.java b/desktop/src/main/java/bisq/desktop/main/dao/governance/make/MakeProposalView.java index 89372c9393..8ec6ed6d28 100644 --- a/desktop/src/main/java/bisq/desktop/main/dao/governance/make/MakeProposalView.java +++ b/desktop/src/main/java/bisq/desktop/main/dao/governance/make/MakeProposalView.java @@ -283,7 +283,7 @@ public class MakeProposalView extends ActivatableView implements Proposal proposal = proposalWithTransaction.getProposal(); Transaction transaction = proposalWithTransaction.getTransaction(); Coin miningFee = transaction.getFee(); - int txSize = transaction.bitcoinSerialize().length; + int txSize = transaction.getVsize(); Coin fee = daoFacade.getProposalFee(daoFacade.getChainHeight()); if (type.equals(ProposalType.BONDED_ROLE)) { diff --git a/desktop/src/main/java/bisq/desktop/main/dao/wallet/send/BsqSendView.java b/desktop/src/main/java/bisq/desktop/main/dao/wallet/send/BsqSendView.java index 0c8a6cd841..06861f3002 100644 --- a/desktop/src/main/java/bisq/desktop/main/dao/wallet/send/BsqSendView.java +++ b/desktop/src/main/java/bisq/desktop/main/dao/wallet/send/BsqSendView.java @@ -243,7 +243,7 @@ public class BsqSendView extends ActivatableView implements BsqB Transaction txWithBtcFee = btcWalletService.completePreparedSendBsqTx(preparedSendTx, true); Transaction signedTx = bsqWalletService.signTx(txWithBtcFee); Coin miningFee = signedTx.getFee(); - int txSize = signedTx.bitcoinSerialize().length; + int txSize = signedTx.getVsize(); showPublishTxPopup(receiverAmount, txWithBtcFee, TxType.TRANSFER_BSQ, @@ -305,7 +305,7 @@ public class BsqSendView extends ActivatableView implements BsqB if (miningFee.getValue() >= receiverAmount.getValue()) GUIUtil.showWantToBurnBTCPopup(miningFee, receiverAmount, btcFormatter); else { - int txSize = signedTx.bitcoinSerialize().length; + int txSize = signedTx.getVsize(); showPublishTxPopup(receiverAmount, txWithBtcFee, TxType.INVALID, diff --git a/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java b/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java index 8d3620c67e..47a5ba3151 100644 --- a/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java +++ b/desktop/src/main/java/bisq/desktop/main/funds/withdrawal/WithdrawalView.java @@ -356,7 +356,7 @@ public class WithdrawalView extends ActivatableView { } if (areInputsValid()) { - int txSize = feeEstimationTransaction.bitcoinSerialize().length; + int txSize = feeEstimationTransaction.getVsize(); log.info("Fee for tx with size {}: {} " + Res.getBaseCurrencyCode() + "", txSize, fee.toPlainString()); if (receiverAmount.isPositive()) { diff --git a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep4View.java b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep4View.java index 55378fd567..e01b1e0b91 100644 --- a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep4View.java +++ b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/steps/buyer/BuyerStep4View.java @@ -205,7 +205,7 @@ public class BuyerStep4View extends TradeStepView { validateWithdrawAddress(); } else if (Restrictions.isAboveDust(receiverAmount)) { CoinFormatter formatter = model.btcFormatter; - int txSize = feeEstimationTransaction.bitcoinSerialize().length; + int txSize = feeEstimationTransaction.getVsize(); double feePerByte = CoinUtil.getFeePerByte(fee, txSize); double kb = txSize / 1000d; String recAmount = formatter.formatCoinWithCode(receiverAmount); From 7a58bfbafa19fd155ae95ba5daf12fb4fe58ce12 Mon Sep 17 00:00:00 2001 From: Oscar Guindzberg Date: Tue, 6 Oct 2020 15:22:53 -0300 Subject: [PATCH 3/8] Use SegwitAddress for fee estimation --- .../java/bisq/core/btc/wallet/BtcWalletService.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) 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 fdbc5cbda7..8573e1c687 100644 --- a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java +++ b/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java @@ -34,7 +34,7 @@ import org.bitcoinj.core.AddressFormatException; import org.bitcoinj.core.Coin; import org.bitcoinj.core.ECKey; import org.bitcoinj.core.InsufficientMoneyException; -import org.bitcoinj.core.LegacyAddress; +import org.bitcoinj.core.SegwitAddress; import org.bitcoinj.core.Transaction; import org.bitcoinj.core.TransactionConfidence; import org.bitcoinj.core.TransactionInput; @@ -992,7 +992,9 @@ public class BtcWalletService extends WalletService { counter++; fee = txFeeForWithdrawalPerByte.multiply(txSize); // We use a dummy address for the output - final String dummyReceiver = LegacyAddress.fromKey(params, new ECKey()).toBase58(); + // We don't know here whether the output is segwit or not but we don't care too much because the size of + // a segwit ouput is just 3 byte smaller than the size of a legacy ouput. + final String dummyReceiver = SegwitAddress.fromKey(params, new ECKey()).toString(); SendRequest sendRequest = getSendRequestForMultipleAddresses(fromAddresses, dummyReceiver, amount, fee, null, aesKey); wallet.completeTx(sendRequest); tx = sendRequest.tx; @@ -1021,7 +1023,9 @@ public class BtcWalletService extends WalletService { public int getEstimatedFeeTxSize(List outputValues, Coin txFee) throws InsufficientMoneyException, AddressFormatException { Transaction transaction = new Transaction(params); - Address dummyAddress = LegacyAddress.fromKey(params, new ECKey()); + // In reality txs have a mix of segwit/legacy ouputs, but we don't care too much because the size of + // a segwit ouput is just 3 byte smaller than the size of a legacy ouput. + Address dummyAddress = SegwitAddress.fromKey(params, new ECKey()); outputValues.forEach(outputValue -> transaction.addOutput(outputValue, dummyAddress)); SendRequest sendRequest = SendRequest.forTx(transaction); From 29f23fe50c6727f8784f3af7c8de3d988ba517ac Mon Sep 17 00:00:00 2001 From: Oscar Guindzberg Date: Fri, 16 Oct 2020 17:00:56 -0300 Subject: [PATCH 4/8] Use segwit tx sizes --- .../bisq/core/btc/TxFeeEstimationService.java | 41 +++++++++++++------ .../core/btc/TxFeeEstimationServiceTest.java | 18 ++++---- .../offer/takeoffer/TakeOfferDataModel.java | 32 +++++++-------- 3 files changed, 53 insertions(+), 38 deletions(-) diff --git a/core/src/main/java/bisq/core/btc/TxFeeEstimationService.java b/core/src/main/java/bisq/core/btc/TxFeeEstimationService.java index c0fbfb5b68..71648011af 100644 --- a/core/src/main/java/bisq/core/btc/TxFeeEstimationService.java +++ b/core/src/main/java/bisq/core/btc/TxFeeEstimationService.java @@ -41,9 +41,24 @@ import static com.google.common.base.Preconditions.checkArgument; */ @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; + +// Size/vsize of typical trade txs +// Real txs size/vsize may vary in 1 or 2 bytes from the estimated values. +// Values calculated with https://gist.github.com/oscarguindzberg/3d1349cb65d9fd9af9de0feaa3fd27ac +// legacy fee tx with 1 input, maker/taker fee paid in btc size/vsize = 258 +// legacy deposit tx without change size/vsize = 381 +// legacy deposit tx with change size/vsize = 414 +// legacy payout tx size/vsize = 337 +// legacy delayed payout tx size/vsize = 302 +// segwit fee tx with 1 input, maker/taker fee paid in btc vsize = 173 +// segwit deposit tx without change vsize = 232 +// segwit deposit tx with change vsize = 263 +// segwit payout tx vsize = 169 +// segwit delayed payout tx vsize = 139 + public static int TYPICAL_TX_WITH_1_INPUT_SIZE = 175; + private static int DEPOSIT_TX_SIZE = 233; + private static int PAYOUT_TX_SIZE = 169; + private static int BSQ_INPUT_INCREASE = 150; private static int MAX_ITERATIONS = 10; @@ -87,14 +102,14 @@ public class TxFeeEstimationService { BtcWalletService btcWalletService, Preferences preferences) { Coin txFeePerByte = feeService.getTxFeePerByte(); - // We start with min taker fee size of 260 + // We start with min taker fee size of 175 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; + // if we cannot do the estimation we use the deposit tx size + estimatedTxSize = DEPOSIT_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); @@ -109,13 +124,13 @@ public class TxFeeEstimationService { 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); + int averageSize = (estimatedTxSize + DEPOSIT_TX_SIZE) / 2; // deposit tx has about 233 bytes + // We use at least the size of the deposit tx to not underpay it. + size = Math.max(DEPOSIT_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" + + "We use an average between the taker fee tx and the deposit tx (233 bytes) which results in {} bytes.\n" + + "The deposit tx has 233 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; @@ -130,7 +145,7 @@ public class TxFeeEstimationService { FeeService feeService, BtcWalletService btcWalletService) { Coin txFeePerByte = feeService.getTxFeePerByte(); - // We start with min taker fee size of 260 + // We start with min taker fee size of 175 int estimatedTxSize = TYPICAL_TX_WITH_1_INPUT_SIZE; try { estimatedTxSize = getEstimatedTxSize(List.of(amount), estimatedTxSize, txFeePerByte, btcWalletService); @@ -145,7 +160,7 @@ public class TxFeeEstimationService { return new Tuple2<>(txFee, estimatedTxSize); } - // We start with the initialEstimatedTxSize for a tx with 1 input (260) bytes and get from BitcoinJ a tx back which + // We start with the initialEstimatedTxSize for a tx with 1 input (175) 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 diff --git a/core/src/test/java/bisq/core/btc/TxFeeEstimationServiceTest.java b/core/src/test/java/bisq/core/btc/TxFeeEstimationServiceTest.java index a71fb8237b..ca7076bf29 100644 --- a/core/src/test/java/bisq/core/btc/TxFeeEstimationServiceTest.java +++ b/core/src/test/java/bisq/core/btc/TxFeeEstimationServiceTest.java @@ -45,14 +45,14 @@ public class TxFeeEstimationServiceTest { int realTxSize; Coin txFee; - initialEstimatedTxSize = 260; + initialEstimatedTxSize = 175; txFeePerByte = Coin.valueOf(10); - realTxSize = 260; + realTxSize = 175; txFee = txFeePerByte.multiply(initialEstimatedTxSize); when(btcWalletService.getEstimatedFeeTxSize(outputValues, txFee)).thenReturn(realTxSize); result = TxFeeEstimationService.getEstimatedTxSize(outputValues, initialEstimatedTxSize, txFeePerByte, btcWalletService); - assertEquals(260, result); + assertEquals(175, result); } // FIXME @Bernard could you have a look? @@ -67,16 +67,16 @@ public class TxFeeEstimationServiceTest { int realTxSize; Coin txFee; - initialEstimatedTxSize = 260; + initialEstimatedTxSize = 175; txFeePerByte = Coin.valueOf(10); - realTxSize = 2600; + realTxSize = 1750; 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); + assertEquals(1750, result); } // FIXME @Bernard could you have a look? @@ -91,14 +91,14 @@ public class TxFeeEstimationServiceTest { int realTxSize; Coin txFee; - initialEstimatedTxSize = 2600; + initialEstimatedTxSize = 1750; txFeePerByte = Coin.valueOf(10); - realTxSize = 260; + realTxSize = 175; txFee = txFeePerByte.multiply(initialEstimatedTxSize); when(btcWalletService.getEstimatedFeeTxSize(outputValues, txFee)).thenReturn(realTxSize); result = TxFeeEstimationService.getEstimatedTxSize(outputValues, initialEstimatedTxSize, txFeePerByte, btcWalletService); - assertEquals(260, result); + assertEquals(175, result); } @Test 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 399f45bbef..bbc060ea0e 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 @@ -109,8 +109,8 @@ class TakeOfferDataModel extends OfferDataModel { private PaymentAccount paymentAccount; private boolean isTabSelected; 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; + // Use an average of a typical trade fee tx with 1 input, deposit tx and payout tx. + private int feeTxSize = 192; // (175+233+169)/3 private boolean freezeFee; private Coin txFeePerByteFromFeeService; @@ -213,13 +213,13 @@ class TakeOfferDataModel extends OfferDataModel { // multiple batch-signed payout tx with different fees might be an option but RBF is not supported yet in BitcoinJ // and batched txs would add more complexity to the trade protocol. - // A typical trade fee tx has about 260 bytes (if one input). The trade txs has about 336-414 bytes. - // We use 320 as a average value. + // A typical trade fee tx has about 175 bytes (if one input). The trade txs has about 169-263 bytes. + // We use 192 as average value. - // trade fee tx: 260 bytes (1 input) - // deposit tx: 336 bytes (1 MS output+ OP_RETURN) - 414 bytes (1 MS output + OP_RETURN + change in case of smaller trade amount) - // payout tx: 371 bytes - // disputed payout tx: 408 bytes + // trade fee tx: 175 bytes (1 input) + // deposit tx: 233 bytes (1 MS output+ OP_RETURN) - 263 bytes (1 MS output + OP_RETURN + change in case of smaller trade amount) + // payout tx: 169 bytes + // disputed payout tx: 139 bytes // Set the default values (in rare cases if the fee request was not done yet we get the hard coded default values) // But the "take offer" happens usually after that so we should have already the value from the estimation service. @@ -351,22 +351,22 @@ class TakeOfferDataModel extends OfferDataModel { // 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. + // On the other side if we have the best case of a 1 input tx fee tx then it is only 175 bytes but the + // other 2 txs are different (233 and 169 bytes) and may 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. + // We make sure that the fee is sufficient to meet our intended fee/byte for the larger deposit tx with 233 bytes. Tuple2 estimatedFeeAndTxSize = txFeeEstimationService.getEstimatedFeeAndTxSizeForTaker(fundsNeededForTrade, getTakerFee()); txFeeFromFeeService = estimatedFeeAndTxSize.first; feeTxSize = estimatedFeeAndTxSize.second; } else { - feeTxSize = 380; + feeTxSize = 233; 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 their wallet yet.\n" + - "In that case we use an estimated tx size of 380 bytes.\n" + + "In that case we use an estimated tx size of 233 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()); } @@ -531,7 +531,7 @@ class TakeOfferDataModel extends OfferDataModel { // With that we avoid that we overpay in case that the trade fee has many inputs and we would apply that fee for the // other 2 txs as well. We still might overpay a bit for the payout tx. private int getAverageSize(int txSize) { - return (txSize + 320) / 2; + return (txSize + 233) / 2; } private Coin getTxFeeBySize(int sizeInBytes) { @@ -606,7 +606,7 @@ class TakeOfferDataModel extends OfferDataModel { // 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); + return txFeeFromFeeService; //feeService.getTxFee(233); } private Coin getTxFeeForPayoutTx() { @@ -614,7 +614,7 @@ class TakeOfferDataModel extends OfferDataModel { // 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); + return txFeeFromFeeService; //feeService.getTxFee(169); } public AddressEntry getAddressEntry() { From 327a3584463d0cb4dbe4b529441f7f893c543687 Mon Sep 17 00:00:00 2001 From: Oscar Guindzberg Date: Wed, 21 Oct 2020 18:49:21 -0300 Subject: [PATCH 5/8] Split segwit from legacy inputs Goal: Have a more accurate fee calculation --- .../core/btc/wallet/BtcWalletService.java | 70 +++++++++++++++---- 1 file changed, 58 insertions(+), 12 deletions(-) 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 8573e1c687..c324d5808f 100644 --- a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java +++ b/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java @@ -28,6 +28,7 @@ import bisq.core.provider.fee.FeeService; import bisq.core.user.Preferences; import bisq.common.handlers.ErrorMessageHandler; +import bisq.common.util.Tuple2; import org.bitcoinj.core.Address; import org.bitcoinj.core.AddressFormatException; @@ -44,6 +45,7 @@ import org.bitcoinj.crypto.DeterministicKey; import org.bitcoinj.crypto.KeyCrypterScrypt; import org.bitcoinj.script.Script; import org.bitcoinj.script.ScriptBuilder; +import org.bitcoinj.script.ScriptPattern; import org.bitcoinj.wallet.SendRequest; import org.bitcoinj.wallet.Wallet; @@ -228,7 +230,9 @@ public class BtcWalletService extends WalletService { preferences.getIgnoreDustThreshold()); List preparedBsqTxInputs = preparedTx.getInputs(); List preparedBsqTxOutputs = preparedTx.getOutputs(); - int numInputs = preparedBsqTxInputs.size(); + Tuple2 numInputs = getNumInputs(preparedTx); + int numLegacyInputs = numInputs.first; + int numSegwitInputs = numInputs.second; Transaction resultTx = null; boolean isFeeOutsideTolerance; do { @@ -249,7 +253,10 @@ public class BtcWalletService extends WalletService { // signInputs needs to be false as it would try to sign all inputs (BSQ inputs are not in this wallet) sendRequest.signInputs = false; - sendRequest.fee = txFeePerByte.multiply(txSizeWithUnsignedInputs + sigSizePerInput * numInputs); + sendRequest.fee = txFeePerByte.multiply(txSizeWithUnsignedInputs + + sigSizePerInput * numLegacyInputs + + sigSizePerInput * numSegwitInputs / 4); + sendRequest.feePerKb = Coin.ZERO; sendRequest.ensureMinRequiredFee = false; @@ -262,9 +269,14 @@ public class BtcWalletService extends WalletService { // add OP_RETURN output resultTx.addOutput(new TransactionOutput(params, resultTx, Coin.ZERO, ScriptBuilder.createOpReturnScript(opReturnData).getProgram())); - numInputs = resultTx.getInputs().size(); + numInputs = getNumInputs(resultTx); + numLegacyInputs = numInputs.first; + numSegwitInputs = numInputs.second; txSizeWithUnsignedInputs = resultTx.getVsize(); - long estimatedFeeAsLong = txFeePerByte.multiply(txSizeWithUnsignedInputs + sigSizePerInput * numInputs).value; + long estimatedFeeAsLong = txFeePerByte.multiply(txSizeWithUnsignedInputs + + sigSizePerInput * numLegacyInputs + + sigSizePerInput * numSegwitInputs / 4).value; + // calculated fee must be inside of a tolerance range with tx fee isFeeOutsideTolerance = Math.abs(resultTx.getFee().value - estimatedFeeAsLong) > 1000; } @@ -338,7 +350,9 @@ public class BtcWalletService extends WalletService { preferences.getIgnoreDustThreshold()); List preparedBsqTxInputs = preparedTx.getInputs(); List preparedBsqTxOutputs = preparedTx.getOutputs(); - int numInputs = preparedBsqTxInputs.size(); + Tuple2 numInputs = getNumInputs(preparedTx); + int numLegacyInputs = numInputs.first; + int numSegwitInputs = numInputs.second; Transaction resultTx = null; boolean isFeeOutsideTolerance; do { @@ -359,7 +373,9 @@ public class BtcWalletService extends WalletService { // signInputs needs to be false as it would try to sign all inputs (BSQ inputs are not in this wallet) sendRequest.signInputs = false; - sendRequest.fee = txFeePerByte.multiply(txSizeWithUnsignedInputs + sigSizePerInput * numInputs); + sendRequest.fee = txFeePerByte.multiply(txSizeWithUnsignedInputs + + sigSizePerInput * numLegacyInputs + + sigSizePerInput * numSegwitInputs / 4); sendRequest.feePerKb = Coin.ZERO; sendRequest.ensureMinRequiredFee = false; @@ -372,9 +388,13 @@ public class BtcWalletService extends WalletService { // add OP_RETURN output resultTx.addOutput(new TransactionOutput(params, resultTx, Coin.ZERO, ScriptBuilder.createOpReturnScript(opReturnData).getProgram())); - numInputs = resultTx.getInputs().size(); + numInputs = getNumInputs(resultTx); + numLegacyInputs = numInputs.first; + numSegwitInputs = numInputs.second; txSizeWithUnsignedInputs = resultTx.getVsize(); - final long estimatedFeeAsLong = txFeePerByte.multiply(txSizeWithUnsignedInputs + sigSizePerInput * numInputs).value; + final long estimatedFeeAsLong = txFeePerByte.multiply(txSizeWithUnsignedInputs + + sigSizePerInput * numLegacyInputs + + sigSizePerInput * numSegwitInputs / 4).value; // calculated fee must be inside of a tolerance range with tx fee isFeeOutsideTolerance = Math.abs(resultTx.getFee().value - estimatedFeeAsLong) > 1000; } @@ -479,7 +499,10 @@ public class BtcWalletService extends WalletService { preferences.getIgnoreDustThreshold()); List preparedBsqTxInputs = preparedBsqTx.getInputs(); List preparedBsqTxOutputs = preparedBsqTx.getOutputs(); - int numInputs = preparedBsqTxInputs.size() + 1; // We add 1 for the BTC fee input + // We don't know at this point what type the btc input would be (segwit/legacy). + // We use legacy to be on the safe side. + int numLegacyInputs = preparedBsqTxInputs.size() + 1; // We add 1 for the BTC fee input + int numSegwitInputs = 0; Transaction resultTx = null; boolean isFeeOutsideTolerance; boolean opReturnIsOnlyOutput; @@ -508,7 +531,9 @@ public class BtcWalletService extends WalletService { // signInputs needs to be false as it would try to sign all inputs (BSQ inputs are not in this wallet) sendRequest.signInputs = false; - sendRequest.fee = txFeePerByte.multiply(txSizeWithUnsignedInputs + sigSizePerInput * numInputs); + sendRequest.fee = txFeePerByte.multiply(txSizeWithUnsignedInputs + + sigSizePerInput * numLegacyInputs + + sigSizePerInput * numSegwitInputs / 4); sendRequest.feePerKb = Coin.ZERO; sendRequest.ensureMinRequiredFee = false; @@ -528,9 +553,13 @@ public class BtcWalletService extends WalletService { if (opReturnData != null) resultTx.addOutput(new TransactionOutput(params, resultTx, Coin.ZERO, ScriptBuilder.createOpReturnScript(opReturnData).getProgram())); - numInputs = resultTx.getInputs().size(); + Tuple2 numInputs = getNumInputs(resultTx); + numLegacyInputs = numInputs.first; + numSegwitInputs = numInputs.second; txSizeWithUnsignedInputs = resultTx.getVsize(); - final long estimatedFeeAsLong = txFeePerByte.multiply(txSizeWithUnsignedInputs + sigSizePerInput * numInputs).value; + final long estimatedFeeAsLong = txFeePerByte.multiply(txSizeWithUnsignedInputs + + sigSizePerInput * numLegacyInputs + + sigSizePerInput * numSegwitInputs / 4).value; // calculated fee must be inside of a tolerance range with tx fee isFeeOutsideTolerance = Math.abs(resultTx.getFee().value - estimatedFeeAsLong) > 1000; } @@ -548,6 +577,23 @@ public class BtcWalletService extends WalletService { return resultTx; } + private Tuple2 getNumInputs(Transaction tx) { + int numLegacyInputs = 0; + int numSegwitInputs = 0; + for (TransactionInput input : tx.getInputs()) { + TransactionOutput connectedOutput = input.getConnectedOutput(); + if (connectedOutput == null || ScriptPattern.isP2PKH(connectedOutput.getScriptPubKey()) || + ScriptPattern.isP2PK(connectedOutput.getScriptPubKey())) { + numLegacyInputs++; + } else if (ScriptPattern.isP2WPKH(connectedOutput.getScriptPubKey())) { + numSegwitInputs++; + } else { + throw new IllegalArgumentException("Inputs should spend a P2PKH, P2PK or P2WPKH ouput"); + } + } + return new Tuple2(numLegacyInputs, numSegwitInputs); + } + /////////////////////////////////////////////////////////////////////////////////////////// // Commit tx From 5f0009e1a4ef1100ed35f0311e530b4c21416d40 Mon Sep 17 00:00:00 2001 From: Oscar Guindzberg Date: Wed, 28 Oct 2020 17:50:23 -0300 Subject: [PATCH 6/8] Explain why legacy is used by default --- core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java | 2 ++ 1 file changed, 2 insertions(+) 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 c324d5808f..5a3b522d33 100644 --- a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java +++ b/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java @@ -584,6 +584,8 @@ public class BtcWalletService extends WalletService { TransactionOutput connectedOutput = input.getConnectedOutput(); if (connectedOutput == null || ScriptPattern.isP2PKH(connectedOutput.getScriptPubKey()) || ScriptPattern.isP2PK(connectedOutput.getScriptPubKey())) { + // If connectedOutput is null, we don't know here the input type. To avoid underpaying fees, + // we treat it as a legacy input which will result in a higher fee estimation. numLegacyInputs++; } else if (ScriptPattern.isP2WPKH(connectedOutput.getScriptPubKey())) { numSegwitInputs++; From 0d59d5e8725cc3241e8370b3f755e7df05c4f1ec Mon Sep 17 00:00:00 2001 From: Oscar Guindzberg Date: Wed, 28 Oct 2020 17:51:22 -0300 Subject: [PATCH 7/8] Remove unused PAYOUT_TX_SIZE --- core/src/main/java/bisq/core/btc/TxFeeEstimationService.java | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/main/java/bisq/core/btc/TxFeeEstimationService.java b/core/src/main/java/bisq/core/btc/TxFeeEstimationService.java index 71648011af..9401fb0a9b 100644 --- a/core/src/main/java/bisq/core/btc/TxFeeEstimationService.java +++ b/core/src/main/java/bisq/core/btc/TxFeeEstimationService.java @@ -57,7 +57,6 @@ public class TxFeeEstimationService { // segwit delayed payout tx vsize = 139 public static int TYPICAL_TX_WITH_1_INPUT_SIZE = 175; private static int DEPOSIT_TX_SIZE = 233; - private static int PAYOUT_TX_SIZE = 169; private static int BSQ_INPUT_INCREASE = 150; private static int MAX_ITERATIONS = 10; From 7740e3e56d397e94434460ad36eec6f482c1d3e8 Mon Sep 17 00:00:00 2001 From: Oscar Guindzberg Date: Wed, 28 Oct 2020 17:53:20 -0300 Subject: [PATCH 8/8] Fix comment --- core/src/main/java/bisq/core/btc/TxFeeEstimationService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/bisq/core/btc/TxFeeEstimationService.java b/core/src/main/java/bisq/core/btc/TxFeeEstimationService.java index 9401fb0a9b..eeb5a545e2 100644 --- a/core/src/main/java/bisq/core/btc/TxFeeEstimationService.java +++ b/core/src/main/java/bisq/core/btc/TxFeeEstimationService.java @@ -107,7 +107,7 @@ public class TxFeeEstimationService { estimatedTxSize = getEstimatedTxSize(List.of(tradeFee, amount), estimatedTxSize, txFeePerByte, btcWalletService); } catch (InsufficientMoneyException e) { if (isTaker) { - // if we cannot do the estimation we use the deposit tx size + // If we cannot do the estimation, we use the size o the largest of our txs which is the deposit tx. estimatedTxSize = DEPOSIT_TX_SIZE; } log.info("We cannot do the fee estimation because there are not enough funds in the wallet. This is expected " +