From 5748ef06bbae45ac6ac33fdff5280110a3c53b39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Coatelen?= Date: Tue, 21 Feb 2023 10:10:16 +0100 Subject: [PATCH 01/13] add QUERY_CONFIRMED_UTXO_FIRST configuration to select the order for querying utxos --- core/idl/wallet/configuration.djinni | 3 +++ core/src/api/Configuration.cpp | 2 ++ core/src/api/Configuration.hpp | 3 +++ 3 files changed, 8 insertions(+) diff --git a/core/idl/wallet/configuration.djinni b/core/idl/wallet/configuration.djinni index efabdbeaf..a905737e0 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"; + + # Order when querying utxos + const QUERY_CONFIRMED_UTXO_FIRST: string = "QUERY_CONFIRMED_UTXO_FIRST"; } # Configuration of wallet pools. diff --git a/core/src/api/Configuration.cpp b/core/src/api/Configuration.cpp index 5f0c5a3e8..9860323fa 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::QUERY_CONFIRMED_UTXO_FIRST = {"QUERY_CONFIRMED_UTXO_FIRST"}; + } } } // namespace ledger::core::api diff --git a/core/src/api/Configuration.hpp b/core/src/api/Configuration.hpp index bc1e364b5..6c4169e47 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; + + /** Order when querying utxos */ + static std::string const QUERY_CONFIRMED_UTXO_FIRST; }; } } } // namespace ledger::core::api From 0c48d5ccc5abf0112548083e38d95dc16b76ccee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Coatelen?= Date: Tue, 21 Feb 2023 10:11:32 +0100 Subject: [PATCH 02/13] use utxo ordering at queryutxo --- .../src/wallet/bitcoin/BitcoinLikeAccount.cpp | 30 ++++++++++++-- .../BitcoinLikeUTXODatabaseHelper.cpp | 39 ++++++++++++------- .../database/BitcoinLikeUTXODatabaseHelper.h | 7 ++++ .../accounts_public_interfaces_test.cpp | 20 ++++++++++ 4 files changed, 79 insertions(+), 17 deletions(-) diff --git a/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp b/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp index 12d8670f3..8af52c382 100644 --- a/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp +++ b/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp @@ -64,6 +64,13 @@ namespace { using namespace ledger::core; + + BitcoinLikeUTXODatabaseHelper::UTXOOrderType getUtxoOrder(const std::shared_ptr& conf) { + bool confirmFirst = conf->getBoolean(api::Configuration::QUERY_CONFIRMED_UTXO_FIRST).value_or(true); + return confirmFirst ? BitcoinLikeUTXODatabaseHelper::UTXOOrderType::CONFIRMED_FIRST : BitcoinLikeUTXODatabaseHelper::UTXOOrderType::UNCONFIRMED_FIRST; + } + + /// Constructor of BitcoinLikeBlockchainExplorerTransaction that initializes some fields /// with a just signed transaction. /// @@ -461,8 +468,13 @@ namespace ledger { self->logger()->info(fmt::format("Worthless utxo value is {}", worthlessUtxoAmount)); soci::session sql(self->getWallet()->getDatabase()->getReadonlyPool()); std::vector utxo; - - BitcoinLikeUTXODatabaseHelper::queryUTXO(sql, self->getAccountUid(), from, to - from, worthlessUtxoAmount, utxo); + const auto& config = self->getWallet()->getConfig(); + const auto& utxoOrder = getUtxoOrder(config); + BitcoinLikeUTXODatabaseHelper::queryUTXO( + sql, + self->getAccountUid(), from, to - from, + utxoOrder, + worthlessUtxoAmount, utxo); return functional::map>(utxo, [¤cy](const BitcoinLikeBlockchainExplorerOutput &output) -> std::shared_ptr { return std::make_shared(output, currency); }); @@ -546,7 +558,14 @@ namespace ledger { auto keychain = self->getKeychain(); const auto worthlessUtxoAmount = BitcoinLikeTransactionApi::computeWorthlessUtxoValue(self->getWallet()->getCurrency(), keychain->getKeychainEngine(), fees); self->logger()->info(fmt::format("Worthless utxo value is {}", worthlessUtxoAmount)); - BitcoinLikeUTXODatabaseHelper::queryUTXO(sql, uid, 0, std::numeric_limits::max(), worthlessUtxoAmount, utxos); + const auto& config = self->getWallet()->getConfig(); + const auto& utxoOrder = getUtxoOrder(config); + BitcoinLikeUTXODatabaseHelper::queryUTXO( + sql, uid, 0, std::numeric_limits::max(), + utxoOrder, + worthlessUtxoAmount, + utxos + ); switch (strategy) { case api::BitcoinLikePickingStrategy::DEEP_OUTPUTS_FIRST: case api::BitcoinLikePickingStrategy::MERGE_OUTPUTS: @@ -754,7 +773,10 @@ namespace ledger { const auto worthlessUtxoAmount = BitcoinLikeTransactionApi::computeWorthlessUtxoValue(self->getWallet()->getCurrency(), keychain->getKeychainEngine(), fees); self->logger()->info(fmt::format("Worthless utxo value is {}", worthlessUtxoAmount)); - return BitcoinLikeUTXODatabaseHelper::queryAllUtxos(session, self->getAccountUid(), self->getWallet()->getCurrency(), worthlessUtxoAmount); + + const auto& config = self->getWallet()->getConfig(); + const auto& utxoOrder = getUtxoOrder(config); + return BitcoinLikeUTXODatabaseHelper::queryAllUtxos(session, self->getAccountUid(), self->getWallet()->getCurrency(), utxoOrder, worthlessUtxoAmount); }); }; auto getTransaction = [self](const std::string &hash) -> FuturePtr { diff --git a/core/src/wallet/bitcoin/database/BitcoinLikeUTXODatabaseHelper.cpp b/core/src/wallet/bitcoin/database/BitcoinLikeUTXODatabaseHelper.cpp index 3bb89e491..42bf1a690 100644 --- a/core/src/wallet/bitcoin/database/BitcoinLikeUTXODatabaseHelper.cpp +++ b/core/src/wallet/bitcoin/database/BitcoinLikeUTXODatabaseHelper.cpp @@ -41,6 +41,10 @@ using namespace soci; namespace ledger { namespace core { + std::string serializeOrder(const BitcoinLikeUTXODatabaseHelper::UTXOOrderType& order) { + return order==BitcoinLikeUTXODatabaseHelper::UTXOOrderType::UNCONFIRMED_FIRST ? "ASC": "DESC"; + } + std::size_t BitcoinLikeUTXODatabaseHelper::UTXOcount(soci::session &sql, const std::string &accountUid, int64_t dustAmount) { const rowset rows = (sql.prepare << "SELECT o.address FROM bitcoin_outputs AS o " " LEFT OUTER JOIN bitcoin_inputs AS i ON i.previous_tx_uid = o.transaction_uid " @@ -57,14 +61,18 @@ namespace ledger { } std::size_t - BitcoinLikeUTXODatabaseHelper::queryUTXO(soci::session &sql, const std::string &accountUid, int32_t offset, int32_t count, int64_t dustAmount, std::vector &out) { - const rowset rows = (sql.prepare << "SELECT o.address, o.idx, o.transaction_hash, o.amount, o.script, o.block_height," - "replaceable" - " FROM bitcoin_outputs AS o " - " LEFT OUTER JOIN bitcoin_inputs AS i ON i.previous_tx_uid = o.transaction_uid " - " AND i.previous_output_idx = o.idx" - " WHERE i.previous_tx_uid IS NULL AND o.account_uid = :uid AND o.amount > :dustAmount" - " ORDER BY block_height LIMIT :count OFFSET :off", + BitcoinLikeUTXODatabaseHelper::queryUTXO(soci::session &sql, const std::string &accountUid, int32_t offset, int32_t count, UTXOOrderType order, int64_t dustAmount, std::vector &out) { + const std::string orderStr = serializeOrder(order); + auto queryFmt = fmt::format( + "SELECT o.address, o.idx, o.transaction_hash, o.amount, o.script, o.block_height," + "replaceable" + " FROM bitcoin_outputs AS o " + " LEFT OUTER JOIN bitcoin_inputs AS i ON i.previous_tx_uid = o.transaction_uid " + " AND i.previous_output_idx = o.idx" + " WHERE i.previous_tx_uid IS NULL AND o.account_uid = :uid AND o.amount > :dustAmount" + " ORDER BY block_height {} LIMIT :count OFFSET :off", + orderStr); + const rowset rows = (sql.prepare << queryFmt, use(accountUid), use(dustAmount), use(count), use(offset)); for (auto &row : rows) { @@ -106,12 +114,17 @@ namespace ledger { soci::session &session, std::string const &accountUid, api::Currency const ¤cy, + UTXOOrderType order, int64_t dustAmount) { - const soci::rowset rows = (session.prepare << "SELECT o.address, o.idx, o.transaction_hash, o.amount, o.script, o.block_height " - "FROM bitcoin_outputs AS o " - "LEFT OUTER JOIN bitcoin_inputs AS i ON i.previous_tx_uid = o.transaction_uid AND i.previous_output_idx = o.idx " - "WHERE i.previous_tx_uid IS NULL AND o.account_uid = :uid AND o.amount > :dustAmount " - "ORDER BY o.block_height", + const std::string orderStr = serializeOrder(order); + auto queryFmt = fmt::format( + "SELECT o.address, o.idx, o.transaction_hash, o.amount, o.script, o.block_height " + "FROM bitcoin_outputs AS o " + "LEFT OUTER JOIN bitcoin_inputs AS i ON i.previous_tx_uid = o.transaction_uid AND i.previous_output_idx = o.idx " + "WHERE i.previous_tx_uid IS NULL AND o.account_uid = :uid AND o.amount > :dustAmount " + "ORDER BY o.block_height {} ", + orderStr); + const soci::rowset rows = (session.prepare << queryFmt, use(accountUid), use(dustAmount)); std::vector utxos; diff --git a/core/src/wallet/bitcoin/database/BitcoinLikeUTXODatabaseHelper.h b/core/src/wallet/bitcoin/database/BitcoinLikeUTXODatabaseHelper.h index a03db8fa7..dd2c5c8df 100644 --- a/core/src/wallet/bitcoin/database/BitcoinLikeUTXODatabaseHelper.h +++ b/core/src/wallet/bitcoin/database/BitcoinLikeUTXODatabaseHelper.h @@ -44,10 +44,16 @@ namespace ledger { ~BitcoinLikeUTXODatabaseHelper() = delete; public: + enum class UTXOOrderType { + UNCONFIRMED_FIRST, + CONFIRMED_FIRST + }; + static std::size_t queryUTXO(soci::session &sql, const std::string &accountUid, int32_t offset, int32_t count, + UTXOOrderType order, int64_t dustAmount, std::vector &out); @@ -63,6 +69,7 @@ namespace ledger { soci::session &session, std::string const &accountUid, api::Currency const ¤cy, + UTXOOrderType order, int64_t dustAmount); }; } // namespace core diff --git a/core/test/integration/accounts_public_interfaces_test.cpp b/core/test/integration/accounts_public_interfaces_test.cpp index 63406267f..dad989d8b 100644 --- a/core/test/integration/accounts_public_interfaces_test.cpp +++ b/core/test/integration/accounts_public_interfaces_test.cpp @@ -86,6 +86,26 @@ TEST_F(AccountsPublicInterfaceTest, GetBalanceOnAccountWithSomeTxs) { EXPECT_EQ(uxtoCount, 8); } +TEST_F(AccountsPublicInterfaceTest, UtxoOrderConfirmedFirstByDefault) { + auto account = ledger::testing::medium_xpub::inflate(pool, wallet); + auto utxos = uv::wait(account->getUTXO()); + const auto size = utxos.size(); + for(unsigned int i = 0; i < size-1; ++i) { + EXPECT_GE(utxos[i]->getBlockHeight().value(), utxos[i+1]->getBlockHeight().value()); + } +} + +TEST_F(AccountsPublicInterfaceTest, UtxoOrderUnConfirmedFirst) { + auto account = ledger::testing::medium_xpub::inflate(pool, wallet); + auto config = account->getWallet()->getConfig(); + config->putBoolean(api::Configuration::QUERY_CONFIRMED_UTXO_FIRST, false); + auto utxos = uv::wait(account->getUTXO()); + const auto size = utxos.size(); + for(unsigned int i = 0; i < size-1; ++i) { + EXPECT_LE(utxos[i]->getBlockHeight().value(), utxos[i+1]->getBlockHeight().value()); + } +} + TEST_F(AccountsPublicInterfaceTest, GetBalanceHistoryOnAccountWithSomeTxs) { auto account = ledger::testing::medium_xpub::inflate(pool, wallet); auto fromDate = "2017-10-12T13:38:23Z"; From 96c1e491796bb9c3728ee8785bd33f99f5f162c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Coatelen?= Date: Tue, 21 Feb 2023 13:47:19 +0100 Subject: [PATCH 03/13] use confirmed-first approach in BitcoinLikeStrategyUtxoPicker --- .../src/wallet/bitcoin/BitcoinLikeAccount.cpp | 6 +- .../BitcoinLikeStrategyUtxoPicker.cpp | 67 ++++++++++++++++--- .../BitcoinLikeStrategyUtxoPicker.h | 16 +++-- 3 files changed, 73 insertions(+), 16 deletions(-) diff --git a/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp b/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp index 8af52c382..648c22101 100644 --- a/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp +++ b/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp @@ -192,7 +192,11 @@ 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::QUERY_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..67b8cc0cc 100644 --- a/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp +++ b/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp @@ -43,10 +43,28 @@ 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 {}; + } else { + // 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 +89,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 +191,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 +291,14 @@ 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 +373,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 +440,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, @@ -516,7 +543,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 +619,19 @@ 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(); }); } @@ -603,6 +645,7 @@ namespace ledger { auto pickedUtxos = std::vector{}; auto pickedInputs = 0; + pickedUtxos.reserve(utxos.size()); std::sort(utxos.begin(), utxos.end(), functor); @@ -631,5 +674,7 @@ 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..6542a8339 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 From 4b603e86067692a99dfe7017073e8829768b9907 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Coatelen?= Date: Tue, 21 Feb 2023 13:48:26 +0100 Subject: [PATCH 04/13] Revert "use utxo ordering at queryutxo" This reverts commit 0c48d5ccc5abf0112548083e38d95dc16b76ccee. --- .../src/wallet/bitcoin/BitcoinLikeAccount.cpp | 30 ++------------ .../BitcoinLikeUTXODatabaseHelper.cpp | 39 +++++++------------ .../database/BitcoinLikeUTXODatabaseHelper.h | 7 ---- .../accounts_public_interfaces_test.cpp | 20 ---------- 4 files changed, 17 insertions(+), 79 deletions(-) diff --git a/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp b/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp index 648c22101..cf8203ebe 100644 --- a/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp +++ b/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp @@ -64,13 +64,6 @@ namespace { using namespace ledger::core; - - BitcoinLikeUTXODatabaseHelper::UTXOOrderType getUtxoOrder(const std::shared_ptr& conf) { - bool confirmFirst = conf->getBoolean(api::Configuration::QUERY_CONFIRMED_UTXO_FIRST).value_or(true); - return confirmFirst ? BitcoinLikeUTXODatabaseHelper::UTXOOrderType::CONFIRMED_FIRST : BitcoinLikeUTXODatabaseHelper::UTXOOrderType::UNCONFIRMED_FIRST; - } - - /// Constructor of BitcoinLikeBlockchainExplorerTransaction that initializes some fields /// with a just signed transaction. /// @@ -472,13 +465,8 @@ namespace ledger { self->logger()->info(fmt::format("Worthless utxo value is {}", worthlessUtxoAmount)); soci::session sql(self->getWallet()->getDatabase()->getReadonlyPool()); std::vector utxo; - const auto& config = self->getWallet()->getConfig(); - const auto& utxoOrder = getUtxoOrder(config); - BitcoinLikeUTXODatabaseHelper::queryUTXO( - sql, - self->getAccountUid(), from, to - from, - utxoOrder, - worthlessUtxoAmount, utxo); + + BitcoinLikeUTXODatabaseHelper::queryUTXO(sql, self->getAccountUid(), from, to - from, worthlessUtxoAmount, utxo); return functional::map>(utxo, [¤cy](const BitcoinLikeBlockchainExplorerOutput &output) -> std::shared_ptr { return std::make_shared(output, currency); }); @@ -562,14 +550,7 @@ namespace ledger { auto keychain = self->getKeychain(); const auto worthlessUtxoAmount = BitcoinLikeTransactionApi::computeWorthlessUtxoValue(self->getWallet()->getCurrency(), keychain->getKeychainEngine(), fees); self->logger()->info(fmt::format("Worthless utxo value is {}", worthlessUtxoAmount)); - const auto& config = self->getWallet()->getConfig(); - const auto& utxoOrder = getUtxoOrder(config); - BitcoinLikeUTXODatabaseHelper::queryUTXO( - sql, uid, 0, std::numeric_limits::max(), - utxoOrder, - worthlessUtxoAmount, - utxos - ); + BitcoinLikeUTXODatabaseHelper::queryUTXO(sql, uid, 0, std::numeric_limits::max(), worthlessUtxoAmount, utxos); switch (strategy) { case api::BitcoinLikePickingStrategy::DEEP_OUTPUTS_FIRST: case api::BitcoinLikePickingStrategy::MERGE_OUTPUTS: @@ -777,10 +758,7 @@ namespace ledger { const auto worthlessUtxoAmount = BitcoinLikeTransactionApi::computeWorthlessUtxoValue(self->getWallet()->getCurrency(), keychain->getKeychainEngine(), fees); self->logger()->info(fmt::format("Worthless utxo value is {}", worthlessUtxoAmount)); - - const auto& config = self->getWallet()->getConfig(); - const auto& utxoOrder = getUtxoOrder(config); - return BitcoinLikeUTXODatabaseHelper::queryAllUtxos(session, self->getAccountUid(), self->getWallet()->getCurrency(), utxoOrder, worthlessUtxoAmount); + return BitcoinLikeUTXODatabaseHelper::queryAllUtxos(session, self->getAccountUid(), self->getWallet()->getCurrency(), worthlessUtxoAmount); }); }; auto getTransaction = [self](const std::string &hash) -> FuturePtr { diff --git a/core/src/wallet/bitcoin/database/BitcoinLikeUTXODatabaseHelper.cpp b/core/src/wallet/bitcoin/database/BitcoinLikeUTXODatabaseHelper.cpp index 42bf1a690..3bb89e491 100644 --- a/core/src/wallet/bitcoin/database/BitcoinLikeUTXODatabaseHelper.cpp +++ b/core/src/wallet/bitcoin/database/BitcoinLikeUTXODatabaseHelper.cpp @@ -41,10 +41,6 @@ using namespace soci; namespace ledger { namespace core { - std::string serializeOrder(const BitcoinLikeUTXODatabaseHelper::UTXOOrderType& order) { - return order==BitcoinLikeUTXODatabaseHelper::UTXOOrderType::UNCONFIRMED_FIRST ? "ASC": "DESC"; - } - std::size_t BitcoinLikeUTXODatabaseHelper::UTXOcount(soci::session &sql, const std::string &accountUid, int64_t dustAmount) { const rowset rows = (sql.prepare << "SELECT o.address FROM bitcoin_outputs AS o " " LEFT OUTER JOIN bitcoin_inputs AS i ON i.previous_tx_uid = o.transaction_uid " @@ -61,18 +57,14 @@ namespace ledger { } std::size_t - BitcoinLikeUTXODatabaseHelper::queryUTXO(soci::session &sql, const std::string &accountUid, int32_t offset, int32_t count, UTXOOrderType order, int64_t dustAmount, std::vector &out) { - const std::string orderStr = serializeOrder(order); - auto queryFmt = fmt::format( - "SELECT o.address, o.idx, o.transaction_hash, o.amount, o.script, o.block_height," - "replaceable" - " FROM bitcoin_outputs AS o " - " LEFT OUTER JOIN bitcoin_inputs AS i ON i.previous_tx_uid = o.transaction_uid " - " AND i.previous_output_idx = o.idx" - " WHERE i.previous_tx_uid IS NULL AND o.account_uid = :uid AND o.amount > :dustAmount" - " ORDER BY block_height {} LIMIT :count OFFSET :off", - orderStr); - const rowset rows = (sql.prepare << queryFmt, + BitcoinLikeUTXODatabaseHelper::queryUTXO(soci::session &sql, const std::string &accountUid, int32_t offset, int32_t count, int64_t dustAmount, std::vector &out) { + const rowset rows = (sql.prepare << "SELECT o.address, o.idx, o.transaction_hash, o.amount, o.script, o.block_height," + "replaceable" + " FROM bitcoin_outputs AS o " + " LEFT OUTER JOIN bitcoin_inputs AS i ON i.previous_tx_uid = o.transaction_uid " + " AND i.previous_output_idx = o.idx" + " WHERE i.previous_tx_uid IS NULL AND o.account_uid = :uid AND o.amount > :dustAmount" + " ORDER BY block_height LIMIT :count OFFSET :off", use(accountUid), use(dustAmount), use(count), use(offset)); for (auto &row : rows) { @@ -114,17 +106,12 @@ namespace ledger { soci::session &session, std::string const &accountUid, api::Currency const ¤cy, - UTXOOrderType order, int64_t dustAmount) { - const std::string orderStr = serializeOrder(order); - auto queryFmt = fmt::format( - "SELECT o.address, o.idx, o.transaction_hash, o.amount, o.script, o.block_height " - "FROM bitcoin_outputs AS o " - "LEFT OUTER JOIN bitcoin_inputs AS i ON i.previous_tx_uid = o.transaction_uid AND i.previous_output_idx = o.idx " - "WHERE i.previous_tx_uid IS NULL AND o.account_uid = :uid AND o.amount > :dustAmount " - "ORDER BY o.block_height {} ", - orderStr); - const soci::rowset rows = (session.prepare << queryFmt, + const soci::rowset rows = (session.prepare << "SELECT o.address, o.idx, o.transaction_hash, o.amount, o.script, o.block_height " + "FROM bitcoin_outputs AS o " + "LEFT OUTER JOIN bitcoin_inputs AS i ON i.previous_tx_uid = o.transaction_uid AND i.previous_output_idx = o.idx " + "WHERE i.previous_tx_uid IS NULL AND o.account_uid = :uid AND o.amount > :dustAmount " + "ORDER BY o.block_height", use(accountUid), use(dustAmount)); std::vector utxos; diff --git a/core/src/wallet/bitcoin/database/BitcoinLikeUTXODatabaseHelper.h b/core/src/wallet/bitcoin/database/BitcoinLikeUTXODatabaseHelper.h index dd2c5c8df..a03db8fa7 100644 --- a/core/src/wallet/bitcoin/database/BitcoinLikeUTXODatabaseHelper.h +++ b/core/src/wallet/bitcoin/database/BitcoinLikeUTXODatabaseHelper.h @@ -44,16 +44,10 @@ namespace ledger { ~BitcoinLikeUTXODatabaseHelper() = delete; public: - enum class UTXOOrderType { - UNCONFIRMED_FIRST, - CONFIRMED_FIRST - }; - static std::size_t queryUTXO(soci::session &sql, const std::string &accountUid, int32_t offset, int32_t count, - UTXOOrderType order, int64_t dustAmount, std::vector &out); @@ -69,7 +63,6 @@ namespace ledger { soci::session &session, std::string const &accountUid, api::Currency const ¤cy, - UTXOOrderType order, int64_t dustAmount); }; } // namespace core diff --git a/core/test/integration/accounts_public_interfaces_test.cpp b/core/test/integration/accounts_public_interfaces_test.cpp index dad989d8b..63406267f 100644 --- a/core/test/integration/accounts_public_interfaces_test.cpp +++ b/core/test/integration/accounts_public_interfaces_test.cpp @@ -86,26 +86,6 @@ TEST_F(AccountsPublicInterfaceTest, GetBalanceOnAccountWithSomeTxs) { EXPECT_EQ(uxtoCount, 8); } -TEST_F(AccountsPublicInterfaceTest, UtxoOrderConfirmedFirstByDefault) { - auto account = ledger::testing::medium_xpub::inflate(pool, wallet); - auto utxos = uv::wait(account->getUTXO()); - const auto size = utxos.size(); - for(unsigned int i = 0; i < size-1; ++i) { - EXPECT_GE(utxos[i]->getBlockHeight().value(), utxos[i+1]->getBlockHeight().value()); - } -} - -TEST_F(AccountsPublicInterfaceTest, UtxoOrderUnConfirmedFirst) { - auto account = ledger::testing::medium_xpub::inflate(pool, wallet); - auto config = account->getWallet()->getConfig(); - config->putBoolean(api::Configuration::QUERY_CONFIRMED_UTXO_FIRST, false); - auto utxos = uv::wait(account->getUTXO()); - const auto size = utxos.size(); - for(unsigned int i = 0; i < size-1; ++i) { - EXPECT_LE(utxos[i]->getBlockHeight().value(), utxos[i+1]->getBlockHeight().value()); - } -} - TEST_F(AccountsPublicInterfaceTest, GetBalanceHistoryOnAccountWithSomeTxs) { auto account = ledger::testing::medium_xpub::inflate(pool, wallet); auto fromDate = "2017-10-12T13:38:23Z"; From 8a66d6eb42487d4f76efa127b4fecfafe614ecc4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Coatelen?= Date: Tue, 21 Feb 2023 13:53:42 +0100 Subject: [PATCH 05/13] rename new configuration --- core/idl/wallet/configuration.djinni | 4 ++-- core/src/api/Configuration.cpp | 2 +- core/src/api/Configuration.hpp | 4 ++-- core/src/wallet/bitcoin/BitcoinLikeAccount.cpp | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/core/idl/wallet/configuration.djinni b/core/idl/wallet/configuration.djinni index a905737e0..00e7512ef 100644 --- a/core/idl/wallet/configuration.djinni +++ b/core/idl/wallet/configuration.djinni @@ -128,8 +128,8 @@ Configuration = interface +c { # Allow the generation of the P2TR (Taproot) outputs const ALLOW_P2TR: string = "ALLOW_P2TR"; - # Order when querying utxos - const QUERY_CONFIRMED_UTXO_FIRST: string = "QUERY_CONFIRMED_UTXO_FIRST"; + # 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 9860323fa..9913b505a 100644 --- a/core/src/api/Configuration.cpp +++ b/core/src/api/Configuration.cpp @@ -43,6 +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::QUERY_CONFIRMED_UTXO_FIRST = {"QUERY_CONFIRMED_UTXO_FIRST"}; +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 6c4169e47..2b621de37 100644 --- a/core/src/api/Configuration.hpp +++ b/core/src/api/Configuration.hpp @@ -80,8 +80,8 @@ class LIBCORE_EXPORT Configuration { /** Allow the generation of the P2TR (Taproot) outputs */ static std::string const ALLOW_P2TR; - /** Order when querying utxos */ - static std::string const QUERY_CONFIRMED_UTXO_FIRST; + /** 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 cf8203ebe..3c18d3c54 100644 --- a/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp +++ b/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp @@ -188,7 +188,7 @@ namespace ledger { _picker = std::make_shared( getWallet()->getPool()->getThreadPoolExecutionContext(), getWallet()->getCurrency(), - getWallet()->getConfig()->getBoolean(api::Configuration::QUERY_CONFIRMED_UTXO_FIRST).value_or(true) + getWallet()->getConfig()->getBoolean(api::Configuration::CONFIRMED_UTXO_FIRST).value_or(true) ); _currentBlockHeight = 0; } From 83906e04372a6c9bf059f7592b70eedcbb0f77c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Coatelen?= Date: Tue, 21 Feb 2023 13:57:03 +0100 Subject: [PATCH 06/13] code improvement --- .../transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp b/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp index 67b8cc0cc..fe8f1b8e3 100644 --- a/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp +++ b/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp @@ -51,10 +51,10 @@ namespace ledger { if( (isU1Confirmed && isU2Confirmed) || (!isU1Confirmed && !isU2Confirmed) ) { // ignore the case where confirmation status is similar return {}; - } else { - // otherwise confirmed utxo should be first - return isU1Confirmed; } + + // otherwise confirmed utxo should be first + return isU1Confirmed; } From 6297034ced7ac8f91996bf2352b2d00ac2692b5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Coatelen?= Date: Tue, 21 Feb 2023 15:07:07 +0100 Subject: [PATCH 07/13] test OptimizeSize with confirmed first utxo --- .../bitcoin/bitcoin_utxo_picket_tests.cpp | 63 ++++++++++++++----- 1 file changed, 48 insertions(+), 15 deletions(-) diff --git a/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp b/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp index 7397a2efe..b4a73b0e7 100644 --- a/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp +++ b/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp @@ -71,27 +71,33 @@ 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 +131,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 +155,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 +179,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 +191,33 @@ TEST(OptimizeSize, ApproximationShouldTookEnough) { EXPECT_GE(buddy->changeAmount.toInt64(), inputSizeInBytes * feesPerByte); } +TEST(OptimizeSize, UtxoOrderingShouldUseConfirmedFirst) { + const api::Currency currency = currencies::BITCOIN; + const int64_t feesPerByte = 20; + const int64_t inputSizeInBytes = 148; + const int64_t outputSizeInBytes = 34; + const int64_t emtyTransactionSizeInBytes = 10; + int64_t outputAmount = 25000; + std::vector inputAmounts{15000, 15000, 15000, 15000}; + std::vector> blockHeights = { Option{}, 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, 2); + } + { + auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithOptimizeSize(buddy, utxos, BigInt(-1), currency, false); + EXPECT_EQ(pickedUtxos.size(), 3); + EXPECT_NE(pickedUtxos[0].index, 2); + } +} + + + 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 +232,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(); From 1d76d2b5758a93a22e5244eb5a5681d913aa123e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Coatelen?= Date: Tue, 21 Feb 2023 18:05:51 +0100 Subject: [PATCH 08/13] add tests for confirmed-first utxo picking --- .../bitcoin/bitcoin_utxo_picket_tests.cpp | 58 ++++++++++++++++--- 1 file changed, 49 insertions(+), 9 deletions(-) diff --git a/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp b/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp index b4a73b0e7..5e4b6711c 100644 --- a/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp +++ b/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp @@ -193,13 +193,10 @@ TEST(OptimizeSize, ApproximationShouldTookEnough) { TEST(OptimizeSize, UtxoOrderingShouldUseConfirmedFirst) { const api::Currency currency = currencies::BITCOIN; - const int64_t feesPerByte = 20; - const int64_t inputSizeInBytes = 148; - const int64_t outputSizeInBytes = 34; - const int64_t emtyTransactionSizeInBytes = 10; + const int64_t feesPerByte = 5; int64_t outputAmount = 25000; - std::vector inputAmounts{15000, 15000, 15000, 15000}; - std::vector> blockHeights = { Option{}, Option{}, 12000, Option{} } ; + std::vector inputAmounts{10000, 10000, 10000}; + std::vector> blockHeights = { Option{}, 12000, Option{} } ; auto buddy = createBuddy(feesPerByte, outputAmount, currency); @@ -207,16 +204,55 @@ TEST(OptimizeSize, UtxoOrderingShouldUseConfirmedFirst) { { auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithOptimizeSize(buddy, utxos, BigInt(-1), currency, true); EXPECT_EQ(pickedUtxos.size(), 3); - EXPECT_EQ(pickedUtxos[0].index, 2); + 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::filterWithOptimizeSize(buddy, utxos, BigInt(-1), currency, false); + auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithDeepFirst(buddy, utxos, BigInt(-1), currency); EXPECT_EQ(pickedUtxos.size(), 3); - EXPECT_NE(pickedUtxos[0].index, 2); + 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, 1000, 1000, 1000, 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; @@ -277,3 +313,7 @@ TEST(OptimizeSize, FeeIsEnoughForP2TR) { for (int64_t feesPerByte = 1; feesPerByte < 1000000; feesPerByte *= 10) feeIsEnoughFor(address, targetOutputSizeInBytes, feesPerByte); } + + + + From 79abf88c1fec77455a89eaee099ccd4d4fae73b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Coatelen?= Date: Tue, 21 Feb 2023 18:06:52 +0100 Subject: [PATCH 09/13] fix useConfirmedFirst implementation in filterWithKnapsackSolver --- .../BitcoinLikeStrategyUtxoPicker.cpp | 39 +++++++++++++++++-- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp b/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp index fe8f1b8e3..efa4bd5f2 100644 --- a/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp +++ b/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp @@ -500,11 +500,42 @@ 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; + indexes.reserve(sz); + { + auto const seed = std::chrono::system_clock::now().time_since_epoch().count(); - auto const seed = std::chrono::system_clock::now().time_since_epoch().count(); - std::shuffle(indexes.begin(), indexes.end(), std::default_random_engine(seed)); + if(useConfirmedFirst) { + + std::vector indexesTmp(sz, 0); + std::iota(indexesTmp.begin(), indexesTmp.end(), 0); + std::shuffle(indexesTmp.begin(), indexesTmp.end(), std::default_random_engine(seed)); + + std::list allindexes; + std::list confirmed; + std::list unconfirmed; + + for(const auto idx: indexesTmp) { + + if(utxos[idx].blockHeight.hasValue()) { + confirmed.emplace_back(idx); + } else { + unconfirmed.emplace_back(idx); + } + } + + allindexes.splice(allindexes.end(), confirmed); + allindexes.splice(allindexes.end(), unconfirmed); + std::move(std::begin(allindexes), std::end(allindexes), std::back_inserter(indexes)); + + } else { + + indexes.resize(sz); + std::iota(indexes.begin(), indexes.end(), 0); + std::shuffle(indexes.begin(), indexes.end(), std::default_random_engine(seed)); + } + } // Add fees for a signed input to amount for (auto index : indexes) { From b0f0c17c6003e22ffc5d5b109c91e5f888856746 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Coatelen?= Date: Wed, 22 Feb 2023 09:01:19 +0100 Subject: [PATCH 10/13] fix side effect in test --- core/test/integration/transactions/coin_selection_P2PKH.cpp | 1 + 1 file changed, 1 insertion(+) 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; From cf33fd960160801dc6c9b867eb988557d1390d2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Coatelen?= Date: Wed, 22 Feb 2023 09:27:24 +0100 Subject: [PATCH 11/13] fine-tune a unit test --- core/test/bitcoin/bitcoin_utxo_picket_tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp b/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp index 5e4b6711c..79152b1e7 100644 --- a/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp +++ b/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp @@ -232,7 +232,7 @@ 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, 1000, 1000, 1000, 3000}; + std::vector inputAmounts{15000, 5000, 5000, 1000, 1001, 1002, 1003, 3000}; std::vector> blockHeights(inputAmounts.size(), Option{}); blockHeights[1] = 1000; blockHeights[6] = 2000; From 69e6374c04db3f7823f5a7fd3b800550151de549 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Coatelen?= Date: Wed, 22 Feb 2023 09:42:26 +0100 Subject: [PATCH 12/13] fix code formatting --- .../src/wallet/bitcoin/BitcoinLikeAccount.cpp | 9 ++-- .../BitcoinLikeStrategyUtxoPicker.cpp | 39 +++++--------- .../BitcoinLikeStrategyUtxoPicker.h | 2 +- .../bitcoin/bitcoin_utxo_picket_tests.cpp | 54 +++++++++---------- 4 files changed, 43 insertions(+), 61 deletions(-) diff --git a/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp b/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp index 3c18d3c54..e63eac73f 100644 --- a/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp +++ b/core/src/wallet/bitcoin/BitcoinLikeAccount.cpp @@ -185,11 +185,10 @@ namespace ledger { _synchronizer = synchronizer; _keychain = keychain; _keychain->getAllObservableAddresses(0, 40); - _picker = std::make_shared( - getWallet()->getPool()->getThreadPoolExecutionContext(), - getWallet()->getCurrency(), - getWallet()->getConfig()->getBoolean(api::Configuration::CONFIRMED_UTXO_FIRST).value_or(true) - ); + _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 efa4bd5f2..329715ae5 100644 --- a/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp +++ b/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp @@ -43,12 +43,11 @@ namespace ledger { namespace core { - - std::optional compareConfirmation(const BitcoinLikeUtxo& u1,const BitcoinLikeUtxo& u2) { + std::optional compareConfirmation(const BitcoinLikeUtxo &u1, const BitcoinLikeUtxo &u2) { bool isU1Confirmed = u1.blockHeight.hasValue(); bool isU2Confirmed = u2.blockHeight.hasValue(); - if( (isU1Confirmed && isU2Confirmed) || (!isU1Confirmed && !isU2Confirmed) ) { + if ((isU1Confirmed && isU2Confirmed) || (!isU1Confirmed && !isU2Confirmed)) { // ignore the case where confirmation status is similar return {}; } @@ -57,13 +56,10 @@ namespace ledger { return isU1Confirmed; } - - BitcoinLikeStrategyUtxoPicker::BitcoinLikeStrategyUtxoPicker(const std::shared_ptr &context, const api::Currency ¤cy, - bool useConfirmedFirst) - : BitcoinLikeUtxoPicker(context, currency), _useConfirmedFirst{ useConfirmedFirst } - {} + bool useConfirmedFirst) + : BitcoinLikeUtxoPicker(context, currency), _useConfirmedFirst{useConfirmedFirst} {} Future> BitcoinLikeStrategyUtxoPicker::filterInputs(const std::shared_ptr &buddy) { @@ -291,11 +287,10 @@ namespace ledger { throw make_exception(api::ErrorCode::NOT_ENOUGH_FUNDS, "Cannot gather enough funds."); } - auto descendingEffectiveValue = [useConfirmedFirst](const EffectiveUtxo &lhs, const EffectiveUtxo &rhs) -> bool { - if(useConfirmedFirst) { + if (useConfirmedFirst) { std::optional comp = compareConfirmation(*lhs.utxo, *rhs.utxo); - if(comp.has_value()) { + if (comp.has_value()) { return comp.value(); } } @@ -500,14 +495,13 @@ namespace ledger { std::vector out; // Random shuffle utxos - const auto& sz = utxos.size(); + const auto &sz = utxos.size(); std::vector indexes; indexes.reserve(sz); { auto const seed = std::chrono::system_clock::now().time_since_epoch().count(); - if(useConfirmedFirst) { - + if (useConfirmedFirst) { std::vector indexesTmp(sz, 0); std::iota(indexesTmp.begin(), indexesTmp.end(), 0); std::shuffle(indexesTmp.begin(), indexesTmp.end(), std::default_random_engine(seed)); @@ -516,9 +510,8 @@ namespace ledger { std::list confirmed; std::list unconfirmed; - for(const auto idx: indexesTmp) { - - if(utxos[idx].blockHeight.hasValue()) { + for (const auto idx : indexesTmp) { + if (utxos[idx].blockHeight.hasValue()) { confirmed.emplace_back(idx); } else { unconfirmed.emplace_back(idx); @@ -530,7 +523,6 @@ namespace ledger { std::move(std::begin(allindexes), std::end(allindexes), std::back_inserter(indexes)); } else { - indexes.resize(sz); std::iota(indexes.begin(), indexes.end(), 0); std::shuffle(indexes.begin(), indexes.end(), std::default_random_engine(seed)); @@ -575,9 +567,9 @@ namespace ledger { // Sort vUTXOs descending std::sort(vUTXOs.begin(), vUTXOs.end(), [useConfirmedFirst](auto const &lhs, auto const &rhs) { - if(useConfirmedFirst) { + if (useConfirmedFirst) { std::optional comp = compareConfirmation(lhs, rhs); - if(comp.has_value()) { + if (comp.has_value()) { return comp.value(); } } @@ -655,10 +647,9 @@ namespace ledger { buddy->logger->debug("Start filterWithMergeOutputs"); return filterWithSort(buddy, utxos, aggregatedAmount, currency, [useConfirmedFirst](auto &lhs, auto &rhs) { - - if(useConfirmedFirst) { + if (useConfirmedFirst) { std::optional comp = compareConfirmation(lhs, rhs); - if(comp.has_value()) { + if (comp.has_value()) { return comp.value(); } } @@ -676,7 +667,6 @@ namespace ledger { auto pickedUtxos = std::vector{}; auto pickedInputs = 0; - pickedUtxos.reserve(utxos.size()); std::sort(utxos.begin(), utxos.end(), functor); @@ -706,6 +696,5 @@ 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 6542a8339..410bbce4b 100644 --- a/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.h +++ b/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.h @@ -100,7 +100,7 @@ namespace ledger { const api::Currency ¤cy, std::function const &functor); - bool _useConfirmedFirst { true }; + 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 79152b1e7..7708bf9ac 100644 --- a/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp +++ b/core/test/bitcoin/bitcoin_utxo_picket_tests.cpp @@ -71,13 +71,13 @@ class MockBitcoinLikeOutput : public api::BitcoinLikeOutput { std::shared_ptr _amount; }; -std::vector createUtxos(const std::vector &values, const std::vector>& blockHeights) { +std::vector createUtxos(const std::vector &values, const std::vector> &blockHeights) { std::vector utxos; const auto sz = values.size(); assert(sz == blockHeights.size()); utxos.reserve(sz); - for(unsigned int i = 0; i < sz; ++i) { + for (unsigned int i = 0; i < sz; ++i) { auto amount = Amount(currencies::BITCOIN, 0, BigInt(values[i])); utxos.emplace_back(BitcoinLikeUtxo{ i, @@ -86,8 +86,7 @@ std::vector createUtxos(const std::vector &values, con Option{}, Option{}, "", - blockHeights[i] - }); + blockHeights[i]}); } return utxos; @@ -192,17 +191,17 @@ TEST(OptimizeSize, ApproximationShouldTookEnough) { } TEST(OptimizeSize, UtxoOrderingShouldUseConfirmedFirst) { - const api::Currency currency = currencies::BITCOIN; - const int64_t feesPerByte = 5; - int64_t outputAmount = 25000; + 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{} } ; + std::vector> blockHeights = {Option{}, 12000, Option{}}; - auto buddy = createBuddy(feesPerByte, outputAmount, currency); + auto buddy = createBuddy(feesPerByte, outputAmount, currency); - auto utxos = createUtxos(inputAmounts, blockHeights); + auto utxos = createUtxos(inputAmounts, blockHeights); { - auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithOptimizeSize(buddy, utxos, BigInt(-1), currency, true); + auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithOptimizeSize(buddy, utxos, BigInt(-1), currency, true); EXPECT_EQ(pickedUtxos.size(), 3); EXPECT_EQ(pickedUtxos[0].index, 1); } @@ -210,44 +209,43 @@ TEST(OptimizeSize, UtxoOrderingShouldUseConfirmedFirst) { } TEST(DeepFirst, UtxoOrderingShouldUseConfirmedFirst) { - const api::Currency currency = currencies::BITCOIN; - const int64_t feesPerByte = 20; - int64_t outputAmount = 25000; + 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{} } ; + std::vector> blockHeights = {12000, Option{}, 1000, Option{}}; - auto buddy = createBuddy(feesPerByte, outputAmount, currency); + auto buddy = createBuddy(feesPerByte, outputAmount, currency); - auto utxos = createUtxos(inputAmounts, blockHeights); + auto utxos = createUtxos(inputAmounts, blockHeights); { - auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithDeepFirst(buddy, utxos, BigInt(-1), currency); + 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; + 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 buddy = createBuddy(feesPerByte, outputAmount, currency); - auto utxos = createUtxos(inputAmounts, blockHeights); + auto utxos = createUtxos(inputAmounts, blockHeights); { - auto pickedUtxos = BitcoinLikeStrategyUtxoPicker::filterWithMergeOutputs(buddy, utxos, BigInt(-1), currency, true); + 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); + 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); @@ -313,7 +311,3 @@ TEST(OptimizeSize, FeeIsEnoughForP2TR) { for (int64_t feesPerByte = 1; feesPerByte < 1000000; feesPerByte *= 10) feeIsEnoughFor(address, targetOutputSizeInBytes, feesPerByte); } - - - - From 15cc192b0c9fbb0e9ac50616bc4fac3252194e0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Coatelen?= Date: Wed, 22 Feb 2023 10:36:33 +0100 Subject: [PATCH 13/13] improve code --- .../BitcoinLikeStrategyUtxoPicker.cpp | 40 +++++-------------- 1 file changed, 9 insertions(+), 31 deletions(-) diff --git a/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp b/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp index 329715ae5..f0ad88408 100644 --- a/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp +++ b/core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp @@ -496,37 +496,15 @@ namespace ledger { // Random shuffle utxos const auto &sz = utxos.size(); - std::vector indexes; - indexes.reserve(sz); - { - auto const seed = std::chrono::system_clock::now().time_since_epoch().count(); - - if (useConfirmedFirst) { - std::vector indexesTmp(sz, 0); - std::iota(indexesTmp.begin(), indexesTmp.end(), 0); - std::shuffle(indexesTmp.begin(), indexesTmp.end(), std::default_random_engine(seed)); - - std::list allindexes; - std::list confirmed; - std::list unconfirmed; - - for (const auto idx : indexesTmp) { - if (utxos[idx].blockHeight.hasValue()) { - confirmed.emplace_back(idx); - } else { - unconfirmed.emplace_back(idx); - } - } - - allindexes.splice(allindexes.end(), confirmed); - allindexes.splice(allindexes.end(), unconfirmed); - std::move(std::begin(allindexes), std::end(allindexes), std::back_inserter(indexes)); - - } else { - indexes.resize(sz); - std::iota(indexes.begin(), indexes.end(), 0); - std::shuffle(indexes.begin(), indexes.end(), std::default_random_engine(seed)); - } + 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