Skip to content
This repository was archived by the owner on Feb 12, 2025. It is now read-only.

Fix fee calculation for OptimizeSize UTXO picking strategy #924

Merged
viktorb-ledger merged 12 commits into from
Dec 2, 2022
Merged

Fix fee calculation for OptimizeSize UTXO picking strategy #924

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6f96f59
Add "feeIsEnoughFor" tests (CI is expected to fail)
viktorb-ledger Nov 29, 2022
1400edb
Make linter happy
viktorb-ledger Nov 29, 2022
2155ba3
Add missing P2TR script serialization
viktorb-ledger Nov 29, 2022
7005015
Fix tx size estimation in OptimizeSize strategy
viktorb-ledger Nov 29, 2022
1636840
(int64_t) -> static_cast<int64_t>
viktorb-ledger Nov 29, 2022
29e5f27
(int64_t) -> static_cast<int64_t>
viktorb-ledger Nov 29, 2022
40baf7b
+ narrowing_cast<int32_t>
viktorb-ledger Nov 29, 2022
bff942b
Remove unused UTXO picking strategies
viktorb-ledger Nov 30, 2022
5b45413
BitcoinLikeScript::fromAddress: restrict segwit v1 script generation …
viktorb-ledger Nov 30, 2022
a336e1f
Set version to VG-10452
viktorb-ledger Dec 1, 2022
49528a8
Try different levels of feesPerByte in tests
viktorb-ledger Dec 2, 2022
bbda9e5
Set version to 4.4.4
viktorb-ledger Dec 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions core/src/wallet/bitcoin/scripts/BitcoinLikeScript.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ namespace ledger {
BitcoinLikeScript script;
if (a->isP2WPKH() || a->isP2WSH()) {
script << btccore::OP_0 << a->getHash160();
} else if (a->isP2TR()) {
script << btccore::OP_1 << a->getHash160();
} else if (a->isP2PKH()) {
script << btccore::OP_DUP << btccore::OP_HASH160 << a->getHash160() << btccore::OP_EQUALVERIFY
<< btccore::OP_CHECKSIG;
Expand Down
79 changes: 47 additions & 32 deletions core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,23 +192,32 @@ namespace ledger {
*/

// Compute long term fees
uint32_t confirmTarget = 1008;
uint32_t confirmTarget = 1008;
// TODO: we will have a call to estimateSmartFees (bitcoin core/ explorer side) to estimate long term fees
// int64_t longTermFees = estimateSmartFees(confirmTarget);
int64_t longTermFees = DEFAULT_FALLBACK_FEE;
int64_t longTermFees = DEFAULT_FALLBACK_FEE;

// Compute cost of change
auto const fixedSize = BitcoinLikeTransactionApi::estimateSize(0,
0,
currency,
buddy->keychain->getKeychainEngine());
auto const fixedSize = BitcoinLikeTransactionApi::estimateSize(0,
0,
currency,
buddy->keychain->getKeychainEngine());
// Size of only 1 output (without fixed size)
const int64_t oneOutputSize = BitcoinLikeTransactionApi::estimateSize(0,
1,
currency,
buddy->keychain->getKeychainEngine())
.Max -
fixedSize.Max;
const int64_t changeOutputSize = BitcoinLikeTransactionApi::estimateSize(0,
1,
currency,
buddy->keychain->getKeychainEngine())
.Max -
fixedSize.Max;

// Size of target outputs (can be different from change output size)
const int64_t targetOutputsSize = BitcoinLikeTransactionApi::estimateSize(0,
buddy->request.outputs,
currency,
buddy->keychain->getKeychainEngine())
.Max -
fixedSize.Max;

// Size 1 signed UTXO (signed input)
const int64_t signedUTXOSize = BitcoinLikeTransactionApi::estimateSize(1,
0,
Expand All @@ -217,18 +226,16 @@ namespace ledger {
.Max -
fixedSize.Max;

// Size of unsigned change
const int64_t changeSize = oneOutputSize;
// Size of signed change
const int64_t signedChangeSize = signedUTXOSize;

const int64_t effectiveFees = buddy->request.feePerByte->toInt64();
// Here signedChangeSize should be multiplied by discard fees
// but since we don't have access to estimateSmartFees, we assume
// that discard fees are equal to effectiveFees
const int64_t costOfChange = effectiveFees * (signedChangeSize + changeSize);
const int64_t costOfChange = effectiveFees * (signedChangeSize + changeOutputSize);

buddy->logger->debug("Cost of change {}, signedChangeSize {}, changeSize {}", costOfChange, signedChangeSize, changeSize);
buddy->logger->debug("Cost of change {}, signedChangeSize {}, changeOutputSize {}", costOfChange, signedChangeSize, changeOutputSize);

// Calculate effective value of outputs
int64_t currentAvailableValue = 0;
Expand All @@ -254,7 +261,7 @@ namespace ledger {

// Get no inputs fees
// At beginning, there are no outputs in tx, so noInputFees are fixed fees
int64_t notInputFees = effectiveFees * (fixedSize.Max + (int64_t)(oneOutputSize * buddy->request.outputs.size())); // at least fixed size and outputs(version...)
int64_t notInputFees = effectiveFees * (fixedSize.Max + targetOutputsSize); // at least fixed size and outputs(version...)

// Start coin selection algorithm (according to SelectCoinBnb from Bitcoin Core)
int64_t currentValue = 0;
Expand Down Expand Up @@ -413,17 +420,25 @@ namespace ledger {
const BigInt &aggregatedAmount,
const api::Currency &currency) {
// Tx fixed size
auto const fixedSize = BitcoinLikeTransactionApi::estimateSize(0,
0,
currency,
buddy->keychain->getKeychainEngine());
auto const fixedSize = BitcoinLikeTransactionApi::estimateSize(0,
0,
currency,
buddy->keychain->getKeychainEngine());
// Size of one output (without fixed size)
const int64_t oneOutputSize = BitcoinLikeTransactionApi::estimateSize(0,
1,
currency,
buddy->keychain->getKeychainEngine())
.Max -
fixedSize.Max;
const int64_t changeOutputSize = BitcoinLikeTransactionApi::estimateSize(0,
1,
currency,
buddy->keychain->getKeychainEngine())
.Max -
fixedSize.Max;

const int64_t targetOutputsSize = BitcoinLikeTransactionApi::estimateSize(0,
buddy->request.outputs,
currency,
buddy->keychain->getKeychainEngine())
.Max -
fixedSize.Max;

// Size of UTXO as signed input in tx
const int64_t signedUTXOSize = BitcoinLikeTransactionApi::estimateSize(1,
0,
Expand All @@ -435,19 +450,19 @@ namespace ledger {
const int64_t signedUTXOCost = signedUTXOSize * buddy->request.feePerByte->toInt64();

// Amount + fixed size fees + outputs fees
const int64_t amountWithFixedFees = buddy->request.feePerByte->toInt64() * (fixedSize.Max + (buddy->request.outputs.size() * oneOutputSize)) + buddy->outputAmount.toInt64();
const int64_t amountWithFixedFees = buddy->request.feePerByte->toInt64() * (fixedSize.Max + targetOutputsSize) + buddy->outputAmount.toInt64();

// Minimum amount from which we are willing to create a change for it
// We take the dust as a reference plus the cost of spending this change

const int64_t dustAmount_fixedAndOutputPart = BitcoinLikeTransactionApi::computeDustAmount(currency,
(fixedSize.Max + (buddy->request.outputs.size() * oneOutputSize)));
(fixedSize.Max + targetOutputsSize));
Comment thread
viktorb-ledger marked this conversation as resolved.
Outdated

const int64_t dustAmount_OneInputPart = BitcoinLikeTransactionApi::computeDustAmount(currency,
signedUTXOSize);

const int64_t dustAmountWithOneInput = BitcoinLikeTransactionApi::computeDustAmount(currency,
fixedSize.Max + oneOutputSize + signedUTXOSize + signedUTXOSize);
fixedSize.Max + targetOutputsSize + signedUTXOSize + signedUTXOSize);
Comment thread
viktorb-ledger marked this conversation as resolved.
Outdated

const int64_t minimumChangeWithOneInput = dustAmountWithOneInput + signedUTXOCost;

Expand Down Expand Up @@ -555,13 +570,13 @@ namespace ledger {
buddy->logger->debug("Add coinLowestLarger to coin selection");
out.push_back(coinLowestLarger.getValue());

buddy->changeAmount = BigInt(coinLowestLarger.getValue().value.toLong() - signedUTXOCost - (int64_t)(amountWithFixedFees + oneOutputSize * buddy->request.feePerByte->toInt64()));
buddy->changeAmount = BigInt(coinLowestLarger.getValue().value.toLong() - signedUTXOCost - (int64_t)(amountWithFixedFees + changeOutputSize * buddy->request.feePerByte->toInt64()));
Comment thread
viktorb-ledger marked this conversation as resolved.
Outdated
} else { // Pick bestValues
buddy->logger->debug("Push all vUTXOs");
out = tmpOut;
// Set amount of change
// Change amount = amountWithFixedFees + fees for 1 additional output (change)
buddy->changeAmount = BigInt(bestValue - (int64_t)(amountWithFixedFees + oneOutputSize * buddy->request.feePerByte->toInt64()));
buddy->changeAmount = BigInt(bestValue - (int64_t)(amountWithFixedFees + changeOutputSize * buddy->request.feePerByte->toInt64()));
Comment thread
viktorb-ledger marked this conversation as resolved.
Outdated
}

if (buddy->changeAmount.toInt64() < minimumChange) {
Expand Down
76 changes: 65 additions & 11 deletions core/test/bitcoin/bitcoin_utxo_picket_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
#include <gtest/gtest.h>
#include <ledger/core/api/Networks.hpp>
#include <spdlog/sinks/null_sink.h>
#include <wallet/bitcoin/api_impl/BitcoinLikeScriptApi.h>
#include <wallet/bitcoin/api_impl/BitcoinLikeTransactionApi.h>
#include <wallet/bitcoin/scripts/BitcoinLikeScript.h>
#include <wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.h>
#include <wallet/common/Amount.h>
#include <wallet/currencies.hpp>
Expand Down Expand Up @@ -40,12 +43,6 @@ class MockKeychain : public BitcoinLikeKeychain {
MOCK_CONST_METHOD0(getOutputSizeAsSignedTxInput, int32_t());
};

class MockBitcoinLikeScript : public api::BitcoinLikeScript {
public:
MOCK_METHOD0(head, std::shared_ptr<api::BitcoinLikeScriptChunk>());
MOCK_METHOD0(toString, std::string());
};

class MockBitcoinLikeOutput : public api::BitcoinLikeOutput {
public:
MockBitcoinLikeOutput(int64_t amount) : _amount(std::make_shared<Amount>(currencies::BITCOIN, 0, BigInt(amount))){};
Expand Down Expand Up @@ -95,18 +92,20 @@ std::vector<BitcoinLikeUtxo> createUtxos(const std::vector<int64_t> &values) {
return utxos;
}

std::shared_ptr<BitcoinLikeUtxoPicker::Buddy> createBuddy(int64_t feesPerByte, int64_t outputAmount, const api::Currency &currency) {
std::shared_ptr<BitcoinLikeUtxoPicker::Buddy> createBuddy(int64_t feesPerByte, int64_t outputAmount, const api::Currency &currency, const std::string keychainEngine = api::KeychainEngines::BIP32_P2PKH, const std::string address = "bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4") {
BitcoinLikeTransactionBuildRequest r(std::make_shared<BigInt>(0));
r.wipe = false;
r.feePerByte = std::make_shared<BigInt>(feesPerByte);
r.outputs.push_back(std::make_tuple(std::make_shared<BigInt>(outputAmount), std::make_shared<MockBitcoinLikeScript>()));
r.wipe = false;
r.feePerByte = std::make_shared<BigInt>(feesPerByte);
ledger::core::BitcoinLikeScript outputScript = ledger::core::BitcoinLikeScript::fromAddress(address, currency);
r.outputs.push_back(std::make_tuple(std::make_shared<BigInt>(outputAmount), std::make_shared<BitcoinLikeScriptApi>(outputScript)));
r.utxoPicker = BitcoinUtxoPickerParams{api::BitcoinLikePickingStrategy::OPTIMIZE_SIZE, 0, optional<int32_t>()};

BitcoinLikeGetUtxoFunction g;
BitcoinLikeGetTxFunction tx;
std::shared_ptr<BitcoinLikeBlockchainExplorer> e;
auto config = std::make_shared<ledger::core::DynamicObject>();
config->putString(api::Configuration::KEYCHAIN_ENGINE, api::KeychainEngines::BIP32_P2PKH);
config->putString(api::Configuration::KEYCHAIN_ENGINE, keychainEngine);
config->putBoolean(api::Configuration::ALLOW_P2TR, true);
std::shared_ptr<Preferences> preferences;
std::shared_ptr<MockKeychain> k = std::make_shared<MockKeychain>(config, currency, 0, preferences);
static std::shared_ptr<spdlog::logger> l = spdlog::null_logger_mt("null_sink");
Expand Down Expand Up @@ -185,3 +184,58 @@ TEST(OptimizeSize, ApproximationShouldTookEnough) {
if (buddy->changeAmount.toInt64() != 0)
EXPECT_GE(buddy->changeAmount.toInt64(), inputSizeInBytes * feesPerByte);
}

void feeIsEnoughFor(const std::string address, const int64_t targetOutputSizeInBytes) {
const api::Currency currency = currencies::BITCOIN_TESTNET;
const int64_t feesPerByte = 1;
Comment thread
twilgenbus-ledger marked this conversation as resolved.
Outdated
const int64_t inputSizeInBytes = 68; // we are spending P2WPKH input

const int64_t emtyTransactionSizeInBytes = 10;
int64_t outputAmount = 50000000;
std::vector<int64_t> inputAmounts{100000000};

auto buddy = createBuddy(feesPerByte, outputAmount, currency, api::KeychainEngines::BIP173_P2WPKH, address);

const int64_t changeOutputSizeInBytes = 8 + 1 + 1 + 1 + 20;
const int64_t allOutputsSizeInBytes = targetOutputSizeInBytes + changeOutputSizeInBytes;

auto utxos = createUtxos(inputAmounts);
auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithOptimizeSize(buddy, utxos, BigInt(-1), currency);
int64_t totalInputsValue = 0;
for (auto utxo : pickedUtxos) {
totalInputsValue += utxo.value.toLong();
}
int64_t transactionFees = totalInputsValue - buddy->changeAmount.toInt64() - outputAmount;
int64_t minimumRequiredFees = (emtyTransactionSizeInBytes + allOutputsSizeInBytes + inputSizeInBytes * pickedUtxos.size()) * feesPerByte;

EXPECT_GE(transactionFees, minimumRequiredFees);
if (buddy->changeAmount.toInt64() != 0)
EXPECT_GE(buddy->changeAmount.toInt64(), inputSizeInBytes * feesPerByte);
}

TEST(OptimizeSize, FeeIsEnoughForP2WPKH) {
const std::string address = "tb1qw508d6qejxtdg4y5r3zarvary0c5xw7kxpjzsx"; // P2WPKH

const int64_t targetOutputSizeInBytes = 8 + 1 + 1 + 1 + 20;
// amount + script length + witness version + hash size + hash

feeIsEnoughFor(address, targetOutputSizeInBytes);
}

TEST(OptimizeSize, FeeIsEnoughForP2WSH) {
const std::string address = "tb1qrp33g0q5c5txsp9arysrx4k6zdkfs4nce4xj0gdcccefvpysxf3q0sl5k7"; // P2WSH

const int64_t targetOutputSizeInBytes = 8 + 1 + 1 + 1 + 32;
// amount + script length + witness version + hash size + hash

feeIsEnoughFor(address, targetOutputSizeInBytes);
}

TEST(OptimizeSize, FeeIsEnoughForP2TR) {
const std::string address = "tb1pqqqqp399et2xygdj5xreqhjjvcmzhxw4aywxecjdzew6hylgvsesf3hn0c"; // P2TR

const int64_t targetOutputSizeInBytes = 8 + 1 + 1 + 1 + 32;
// amount + script length + witness version + hash size + hash

feeIsEnoughFor(address, targetOutputSizeInBytes);
}