diff --git a/core/idl/wallet/configuration.djinni b/core/idl/wallet/configuration.djinni index efabdbeaf..00e7512ef 100644 --- a/core/idl/wallet/configuration.djinni +++ b/core/idl/wallet/configuration.djinni @@ -127,6 +127,9 @@ Configuration = interface +c { # Allow the generation of the P2TR (Taproot) outputs const ALLOW_P2TR: string = "ALLOW_P2TR"; + + # Use confirmed UTXOs first in utxo picking strategies + const CONFIRMED_UTXO_FIRST: string = "CONFIRMED_UTXO_FIRST"; } # Configuration of wallet pools. diff --git a/core/src/api/Configuration.cpp b/core/src/api/Configuration.cpp index 5f0c5a3e8..9913b505a 100644 --- a/core/src/api/Configuration.cpp +++ b/core/src/api/Configuration.cpp @@ -43,4 +43,6 @@ std::string const Configuration::MEMPOOL_GRACE_PERIOD_SECS = {"MEMPOOL_GRACE_PER std::string const Configuration::ALLOW_P2TR = {"ALLOW_P2TR"}; +std::string const Configuration::CONFIRMED_UTXO_FIRST = {"CONFIRMED_UTXO_FIRST"}; + } } } // namespace ledger::core::api diff --git a/core/src/api/Configuration.hpp b/core/src/api/Configuration.hpp index bc1e364b5..2b621de37 100644 --- a/core/src/api/Configuration.hpp +++ b/core/src/api/Configuration.hpp @@ -79,6 +79,9 @@ class LIBCORE_EXPORT Configuration { /** Allow the generation of the P2TR (Taproot) outputs */ static std::string const ALLOW_P2TR; + + /** Use confirmed UTXOs first in utxo picking strategies */ + static std::string const CONFIRMED_UTXO_FIRST; }; } } } // namespace ledger::core::api diff --git a/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp b/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp index 12d8670f3..e63eac73f 100644 --- a/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp +++ b/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp @@ -185,7 +185,10 @@ namespace ledger { _synchronizer = synchronizer; _keychain = keychain; _keychain->getAllObservableAddresses(0, 40); - _picker = std::make_shared(getWallet()->getPool()->getThreadPoolExecutionContext(), getWallet()->getCurrency()); + _picker = std::make_shared( + getWallet()->getPool()->getThreadPoolExecutionContext(), + getWallet()->getCurrency(), + getWallet()->getConfig()->getBoolean(api::Configuration::CONFIRMED_UTXO_FIRST).value_or(true)); _currentBlockHeight = 0; } diff --git a/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp b/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp index 178a25840..f0ad88408 100644 --- a/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp +++ b/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp @@ -43,10 +43,24 @@ namespace ledger { namespace core { - BitcoinLikeStrategyUtxoPicker::BitcoinLikeStrategyUtxoPicker(const std::shared_ptr &context, - const api::Currency ¤cy) : BitcoinLikeUtxoPicker(context, currency) { + std::optional compareConfirmation(const BitcoinLikeUtxo &u1, const BitcoinLikeUtxo &u2) { + bool isU1Confirmed = u1.blockHeight.hasValue(); + bool isU2Confirmed = u2.blockHeight.hasValue(); + + if ((isU1Confirmed && isU2Confirmed) || (!isU1Confirmed && !isU2Confirmed)) { + // ignore the case where confirmation status is similar + return {}; + } + + // otherwise confirmed utxo should be first + return isU1Confirmed; } + BitcoinLikeStrategyUtxoPicker::BitcoinLikeStrategyUtxoPicker(const std::shared_ptr &context, + const api::Currency ¤cy, + bool useConfirmedFirst) + : BitcoinLikeUtxoPicker(context, currency), _useConfirmedFirst{useConfirmedFirst} {} + Future> BitcoinLikeStrategyUtxoPicker::filterInputs(const std::shared_ptr &buddy) { return computeAggregatedAmount(buddy).flatMap>(getContext(), [=](BigInt const &amount) { @@ -71,9 +85,9 @@ namespace ledger { case api::BitcoinLikePickingStrategy::DEEP_OUTPUTS_FIRST: return filterWithDeepFirst(buddy, utxos, amount, getCurrency()); case api::BitcoinLikePickingStrategy::OPTIMIZE_SIZE: - return filterWithOptimizeSize(buddy, utxos, amount, getCurrency()); + return filterWithOptimizeSize(buddy, utxos, amount, getCurrency(), _useConfirmedFirst); case api::BitcoinLikePickingStrategy::MERGE_OUTPUTS: - return filterWithMergeOutputs(buddy, utxos, amount, getCurrency()); + return filterWithMergeOutputs(buddy, utxos, amount, getCurrency(), _useConfirmedFirst); } throw make_exception(api::ErrorCode::ILLEGAL_ARGUMENT, "Unknown UTXO picking strategy."); @@ -173,7 +187,8 @@ namespace ledger { BitcoinLikeStrategyUtxoPicker::filterWithOptimizeSize(const std::shared_ptr &buddy, const std::vector &utxos, const BigInt &aggregatedAmount, - const api::Currency ¤cy) { + const api::Currency ¤cy, + bool useConfirmedFirst) { // NOTE: why are we using buddy->outputAmount here instead of aggregatedAmount ? // Don't use this strategy for wipe mode (we have more performent strategies for this use case) if (buddy->request.wipe) { @@ -272,7 +287,13 @@ namespace ledger { throw make_exception(api::ErrorCode::NOT_ENOUGH_FUNDS, "Cannot gather enough funds."); } - auto descendingEffectiveValue = [](const EffectiveUtxo &lhs, const EffectiveUtxo &rhs) -> bool { + auto descendingEffectiveValue = [useConfirmedFirst](const EffectiveUtxo &lhs, const EffectiveUtxo &rhs) -> bool { + if (useConfirmedFirst) { + std::optional comp = compareConfirmation(*lhs.utxo, *rhs.utxo); + if (comp.has_value()) { + return comp.value(); + } + } return lhs.effectiveValue > rhs.effectiveValue; }; @@ -347,7 +368,7 @@ namespace ledger { // If no selection found fallback on filterWithDeepFirst if (bestSelection.empty()) { buddy->logger->debug("No best selection found, fallback on filterWithKnapsackSolver coin selection"); - return filterWithKnapsackSolver(buddy, utxos, aggregatedAmount, currency); + return filterWithKnapsackSolver(buddy, utxos, aggregatedAmount, currency, useConfirmedFirst); } // Prepare result @@ -414,7 +435,8 @@ namespace ledger { const std::shared_ptr &buddy, const std::vector &utxos, const BigInt &aggregatedAmount, - const api::Currency ¤cy) { + const api::Currency ¤cy, + bool useConfirmedFirst) { // Tx fixed size auto const fixedSize = BitcoinLikeTransactionApi::estimateSize(0, 0, @@ -473,12 +495,18 @@ namespace ledger { std::vector out; // Random shuffle utxos - std::vector indexes(utxos.size()); - std::iota(indexes.begin(), indexes.end(), 0); - + const auto &sz = utxos.size(); + std::vector indexes(sz, 0); auto const seed = std::chrono::system_clock::now().time_since_epoch().count(); + std::iota(indexes.begin(), indexes.end(), 0); std::shuffle(indexes.begin(), indexes.end(), std::default_random_engine(seed)); + if (useConfirmedFirst) { + std::stable_sort(indexes.begin(), indexes.end(), [&utxos](auto id1, auto id2) { + return compareConfirmation(utxos[id1], utxos[id2]).value_or(true); + }); + } + // Add fees for a signed input to amount for (auto index : indexes) { auto &utxo = utxos[index]; @@ -516,7 +544,13 @@ namespace ledger { } // Sort vUTXOs descending - std::sort(vUTXOs.begin(), vUTXOs.end(), [](auto const &lhs, auto const &rhs) { + std::sort(vUTXOs.begin(), vUTXOs.end(), [useConfirmedFirst](auto const &lhs, auto const &rhs) { + if (useConfirmedFirst) { + std::optional comp = compareConfirmation(lhs, rhs); + if (comp.has_value()) { + return comp.value(); + } + } return lhs.value.toLong() > rhs.value.toLong(); }); @@ -586,10 +620,18 @@ namespace ledger { const std::shared_ptr &buddy, const std::vector &utxos, const BigInt &aggregatedAmount, - const api::Currency ¤cy) { + const api::Currency ¤cy, + bool useConfirmedFirst) { buddy->logger->debug("Start filterWithMergeOutputs"); - return filterWithSort(buddy, utxos, aggregatedAmount, currency, [](auto &lhs, auto &rhs) { + return filterWithSort(buddy, utxos, aggregatedAmount, currency, [useConfirmedFirst](auto &lhs, auto &rhs) { + if (useConfirmedFirst) { + std::optional comp = compareConfirmation(lhs, rhs); + if (comp.has_value()) { + return comp.value(); + } + } + return lhs.value.toLong() < rhs.value.toLong(); }); } @@ -631,5 +673,6 @@ namespace ledger { return pickedUtxos; } + } // namespace core } // namespace ledger diff --git a/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.h b/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.h index 8f620ad02..410bbce4b 100644 --- a/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.h +++ b/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.h @@ -46,27 +46,33 @@ namespace ledger { class BitcoinLikeStrategyUtxoPicker : public BitcoinLikeUtxoPicker { public: BitcoinLikeStrategyUtxoPicker(const std::shared_ptr &context, - const api::Currency ¤cy); + const api::Currency ¤cy, + bool useConfirmedFirst); public: static std::vector filterWithKnapsackSolver(const std::shared_ptr &buddy, const std::vector &utxos, const BigInt &aggregatedAmount, - const api::Currency &currrency); + const api::Currency &currrency, + bool useConfirmedFirst); static std::vector filterWithOptimizeSize(const std::shared_ptr &buddy, const std::vector &utxos, const BigInt &aggregatedAmount, - const api::Currency &currrency); + const api::Currency &currrency, + bool useConfirmedFirst); static std::vector filterWithMergeOutputs(const std::shared_ptr &buddy, const std::vector &utxos, const BigInt &aggregatedAmount, - const api::Currency &currrency); + const api::Currency &currrency, + bool useConfirmedFirst); + static std::vector filterWithDeepFirst(const std::shared_ptr &buddy, const std::vector &utxo, const BigInt &aggregatedAmount, const api::Currency &currrency); + static bool hasEnough(const std::shared_ptr &buddy, const BigInt &aggregatedAmount, int inputCount, @@ -93,6 +99,8 @@ namespace ledger { BigInt amount, const api::Currency ¤cy, std::function const &functor); + + bool _useConfirmedFirst{true}; }; } // namespace core } // namespace ledger diff --git a/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp b/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp index 7397a2efe..7708bf9ac 100644 --- a/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp +++ b/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp @@ -71,27 +71,32 @@ class MockBitcoinLikeOutput : public api::BitcoinLikeOutput { std::shared_ptr _amount; }; -std::vector createUtxos(const std::vector &values) { +std::vector createUtxos(const std::vector &values, const std::vector> &blockHeights) { std::vector utxos; - - utxos.reserve(values.size()); - - std::transform(values.cbegin(), values.cend(), std::back_inserter(utxos), [](auto const &value) { - auto amount = Amount(currencies::BITCOIN, 0, BigInt(value)); - - return BitcoinLikeUtxo{ - 0, + const auto sz = values.size(); + assert(sz == blockHeights.size()); + utxos.reserve(sz); + + for (unsigned int i = 0; i < sz; ++i) { + auto amount = Amount(currencies::BITCOIN, 0, BigInt(values[i])); + utxos.emplace_back(BitcoinLikeUtxo{ + i, amount.toString(), amount, Option{}, Option{}, "", - Option{}}; - }); + blockHeights[i]}); + } return utxos; } +std::vector createUtxos(const std::vector &values) { + std::vector> blockHeights(3, Option{}); + return createUtxos(values, blockHeights); +} + std::shared_ptr createBuddy(int64_t feesPerByte, int64_t outputAmount, const api::Currency ¤cy, const std::string keychainEngine = api::KeychainEngines::BIP32_P2PKH, const std::string address = "bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4") { BitcoinLikeTransactionBuildRequest r(std::make_shared(0)); r.wipe = false; @@ -125,7 +130,7 @@ TEST(OptimizeSize, BacktrackingCalculateChangeCorrectly) { auto buddy = createBuddy(feesPerByte, outputAmount, currency); auto utxos = createUtxos(inputAmounts); - auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithOptimizeSize(buddy, utxos, BigInt(-1), currency); + auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithOptimizeSize(buddy, utxos, BigInt(-1), currency, false); int64_t totalInputsValue = 0; for (auto utxo : pickedUtxos) { totalInputsValue += utxo.value.toLong(); @@ -149,7 +154,7 @@ TEST(OptimizeSize, ChangeShouldBeBigEnoughToSpend) { auto buddy = createBuddy(feesPerByte, outputAmount, currency); auto utxos = createUtxos(inputAmounts); - auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithOptimizeSize(buddy, utxos, BigInt(-1), currency); + auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithOptimizeSize(buddy, utxos, BigInt(-1), currency, false); int64_t totalInputsValue = 0; for (auto utxo : pickedUtxos) { totalInputsValue += utxo.value.toLong(); @@ -173,7 +178,7 @@ TEST(OptimizeSize, ApproximationShouldTookEnough) { auto buddy = createBuddy(feesPerByte, outputAmount, currency); auto utxos = createUtxos(inputAmounts); - auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithOptimizeSize(buddy, utxos, BigInt(-1), currency); + auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithOptimizeSize(buddy, utxos, BigInt(-1), currency, false); int64_t totalInputsValue = 0; for (auto utxo : pickedUtxos) { totalInputsValue += utxo.value.toLong(); @@ -185,6 +190,68 @@ TEST(OptimizeSize, ApproximationShouldTookEnough) { EXPECT_GE(buddy->changeAmount.toInt64(), inputSizeInBytes * feesPerByte); } +TEST(OptimizeSize, UtxoOrderingShouldUseConfirmedFirst) { + const api::Currency currency = currencies::BITCOIN; + const int64_t feesPerByte = 5; + int64_t outputAmount = 25000; + std::vector inputAmounts{10000, 10000, 10000}; + std::vector> blockHeights = {Option{}, 12000, Option{}}; + + auto buddy = createBuddy(feesPerByte, outputAmount, currency); + + auto utxos = createUtxos(inputAmounts, blockHeights); + { + auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithOptimizeSize(buddy, utxos, BigInt(-1), currency, true); + EXPECT_EQ(pickedUtxos.size(), 3); + EXPECT_EQ(pickedUtxos[0].index, 1); + } + // Cannot perform the "negative" as the algorithm may use randomization and thus might deliver a confirmed-first even if not requested. +} + +TEST(DeepFirst, UtxoOrderingShouldUseConfirmedFirst) { + const api::Currency currency = currencies::BITCOIN; + const int64_t feesPerByte = 20; + int64_t outputAmount = 25000; + std::vector inputAmounts{15000, 15000, 15000, 15000}; + std::vector> blockHeights = {12000, Option{}, 1000, Option{}}; + + auto buddy = createBuddy(feesPerByte, outputAmount, currency); + + auto utxos = createUtxos(inputAmounts, blockHeights); + { + auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithDeepFirst(buddy, utxos, BigInt(-1), currency); + EXPECT_EQ(pickedUtxos.size(), 3); + EXPECT_EQ(pickedUtxos[0].index, 2); + EXPECT_EQ(pickedUtxos[1].index, 0); + } +} + +TEST(MergeOutput, UtxoOrderingShouldUseConfirmedFirst) { + const api::Currency currency = currencies::BITCOIN; + const int64_t feesPerByte = 5; + int64_t outputAmount = 25000; + std::vector inputAmounts{15000, 5000, 5000, 1000, 1001, 1002, 1003, 3000}; + std::vector> blockHeights(inputAmounts.size(), Option{}); + blockHeights[1] = 1000; + blockHeights[6] = 2000; + + auto buddy = createBuddy(feesPerByte, outputAmount, currency); + + auto utxos = createUtxos(inputAmounts, blockHeights); + { + auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithMergeOutputs(buddy, utxos, BigInt(-1), currency, true); + EXPECT_EQ(pickedUtxos.size(), 8); + EXPECT_EQ(pickedUtxos[0].index, 6); + EXPECT_EQ(pickedUtxos[1].index, 1); + } + { + auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithMergeOutputs(buddy, utxos, BigInt(-1), currency, false); + EXPECT_EQ(pickedUtxos.size(), 8); + EXPECT_EQ(pickedUtxos[0].index, 3); + EXPECT_EQ(pickedUtxos[1].index, 4); + } +} + void feeIsEnoughFor(const std::string address, const int64_t targetOutputSizeInBytes, const int64_t feesPerByte) { const api::Currency currency = currencies::BITCOIN_TESTNET; const int64_t inputSizeInBytes = 68; // we are spending P2WPKH input @@ -199,7 +266,7 @@ void feeIsEnoughFor(const std::string address, const int64_t targetOutputSizeInB const int64_t allOutputsSizeInBytes = targetOutputSizeInBytes + changeOutputSizeInBytes; auto utxos = createUtxos(inputAmounts); - auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithOptimizeSize(buddy, utxos, BigInt(-1), currency); + auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithOptimizeSize(buddy, utxos, BigInt(-1), currency, false); int64_t totalInputsValue = 0; for (auto utxo : pickedUtxos) { totalInputsValue += utxo.value.toLong(); diff --git a/core/test/integration/transactions/coin_selection_P2PKH.cpp b/core/test/integration/transactions/coin_selection_P2PKH.cpp index e670d9755..034651579 100644 --- a/core/test/integration/transactions/coin_selection_P2PKH.cpp +++ b/core/test/integration/transactions/coin_selection_P2PKH.cpp @@ -42,6 +42,7 @@ struct CoinSelectionP2PKH : public BitcoinMakeBaseTransaction { testData.configuration->putString(api::Configuration::KEYCHAIN_ENGINE, api::KeychainEngines::BIP49_P2SH); testData.configuration->putString(api::Configuration::KEYCHAIN_DERIVATION_SCHEME, "49'/'/'//
"); testData.configuration->putString(api::Configuration::BLOCKCHAIN_EXPLORER_VERSION, "v3"); + testData.configuration->putBoolean(api::Configuration::CONFIRMED_UTXO_FIRST, false); testData.walletName = randomWalletName(); testData.currencyName = "bitcoin_testnet"; testData.inflate_btc = ledger::testing::coin_selection_xpub::inflate;