Skip to content

Commit

Permalink
VG-13274 - Confirmed-first ordering of utxos when crafting a transact…
Browse files Browse the repository at this point in the history 
…ion + add configuration (#933)

* add CONFIRMED_UTXO_FIRST configuration to select the order for querying utxos
* use confirmed-first approach in BitcoinLikeStrategyUtxoPicker
* test OptimizeSize with confirmed first utxo
* add tests for confirmed-first utxo picking
* fix side effect in test
  • Loading branch information
@jcoatelen-ledger
jcoatelen-ledger authored Feb 22, 2023
1 parent b60d548 commit e35f2ba
Show file tree
Hide file tree
Showing 8 changed files with 164 additions and 34 deletions.
3 changes: 3 additions & 0 deletions core/idl/wallet/configuration.djinni
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 2 additions & 0 deletions core/src/api/Configuration.cpp
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions core/src/api/Configuration.hpp
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion core/src/wallet/bitcoin/BitcoinLikeAccount.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,10 @@ namespace ledger {
_synchronizer = synchronizer;
_keychain = keychain;
_keychain->getAllObservableAddresses(0, 40);
_picker = std::make_shared<BitcoinLikeStrategyUtxoPicker>(getWallet()->getPool()->getThreadPoolExecutionContext(), getWallet()->getCurrency());
_picker = std::make_shared<BitcoinLikeStrategyUtxoPicker>(
getWallet()->getPool()->getThreadPoolExecutionContext(),
getWallet()->getCurrency(),
getWallet()->getConfig()->getBoolean(api::Configuration::CONFIRMED_UTXO_FIRST).value_or(true));
_currentBlockHeight = 0;
}

Expand Down
71 changes: 57 additions & 14 deletions core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,24 @@
namespace ledger {
namespace core {

BitcoinLikeStrategyUtxoPicker::BitcoinLikeStrategyUtxoPicker(const std::shared_ptr<api::ExecutionContext> &context,
const api::Currency &currency) : BitcoinLikeUtxoPicker(context, currency) {
std::optional<bool> 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<api::ExecutionContext> &context,
const api::Currency &currency,
bool useConfirmedFirst)
: BitcoinLikeUtxoPicker(context, currency), _useConfirmedFirst{useConfirmedFirst} {}

Future<std::vector<BitcoinLikeUtxo>>
BitcoinLikeStrategyUtxoPicker::filterInputs(const std::shared_ptr<BitcoinLikeUtxoPicker::Buddy> &buddy) {
return computeAggregatedAmount(buddy).flatMap<std::vector<BitcoinLikeUtxo>>(getContext(), [=](BigInt const &amount) {
Expand All @@ -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.");
Expand Down Expand Up @@ -173,7 +187,8 @@ namespace ledger {
BitcoinLikeStrategyUtxoPicker::filterWithOptimizeSize(const std::shared_ptr<BitcoinLikeUtxoPicker::Buddy> &buddy,
const std::vector<BitcoinLikeUtxo> &utxos,
const BigInt &aggregatedAmount,
const api::Currency &currency) {
const api::Currency &currency,
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) {
Expand Down Expand Up @@ -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<bool> comp = compareConfirmation(*lhs.utxo, *rhs.utxo);
if (comp.has_value()) {
return comp.value();
}
}
return lhs.effectiveValue > rhs.effectiveValue;
};

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -414,7 +435,8 @@ namespace ledger {
const std::shared_ptr<BitcoinLikeUtxoPicker::Buddy> &buddy,
const std::vector<BitcoinLikeUtxo> &utxos,
const BigInt &aggregatedAmount,
const api::Currency &currency) {
const api::Currency &currency,
bool useConfirmedFirst) {
// Tx fixed size
auto const fixedSize = BitcoinLikeTransactionApi::estimateSize(0,
0,
Expand Down Expand Up @@ -473,12 +495,18 @@ namespace ledger {
std::vector<BitcoinLikeUtxo> out;

// Random shuffle utxos
std::vector<size_t> indexes(utxos.size());
std::iota(indexes.begin(), indexes.end(), 0);

const auto &sz = utxos.size();
std::vector<size_t> 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];
Expand Down Expand Up @@ -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<bool> comp = compareConfirmation(lhs, rhs);
if (comp.has_value()) {
return comp.value();
}
}
return lhs.value.toLong() > rhs.value.toLong();
});

Expand Down Expand Up @@ -586,10 +620,18 @@ namespace ledger {
const std::shared_ptr<BitcoinLikeUtxoPicker::Buddy> &buddy,
const std::vector<BitcoinLikeUtxo> &utxos,
const BigInt &aggregatedAmount,
const api::Currency &currency) {
const api::Currency &currency,
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<bool> comp = compareConfirmation(lhs, rhs);
if (comp.has_value()) {
return comp.value();
}
}

return lhs.value.toLong() < rhs.value.toLong();
});
}
Expand Down Expand Up @@ -631,5 +673,6 @@ namespace ledger {

return pickedUtxos;
}

} // namespace core
} // namespace ledger
16 changes: 12 additions & 4 deletions core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,27 +46,33 @@ namespace ledger {
class BitcoinLikeStrategyUtxoPicker : public BitcoinLikeUtxoPicker {
public:
BitcoinLikeStrategyUtxoPicker(const std::shared_ptr<api::ExecutionContext> &context,
const api::Currency &currency);
const api::Currency &currency,
bool useConfirmedFirst);

public:
static std::vector<BitcoinLikeUtxo> filterWithKnapsackSolver(const std::shared_ptr<Buddy> &buddy,
const std::vector<BitcoinLikeUtxo> &utxos,
const BigInt &aggregatedAmount,
const api::Currency &currrency);
const api::Currency &currrency,
bool useConfirmedFirst);

static std::vector<BitcoinLikeUtxo> filterWithOptimizeSize(const std::shared_ptr<Buddy> &buddy,
const std::vector<BitcoinLikeUtxo> &utxos,
const BigInt &aggregatedAmount,
const api::Currency &currrency);
const api::Currency &currrency,
bool useConfirmedFirst);

static std::vector<BitcoinLikeUtxo> filterWithMergeOutputs(const std::shared_ptr<Buddy> &buddy,
const std::vector<BitcoinLikeUtxo> &utxos,
const BigInt &aggregatedAmount,
const api::Currency &currrency);
const api::Currency &currrency,
bool useConfirmedFirst);

static std::vector<BitcoinLikeUtxo> filterWithDeepFirst(const std::shared_ptr<Buddy> &buddy,
const std::vector<BitcoinLikeUtxo> &utxo,
const BigInt &aggregatedAmount,
const api::Currency &currrency);

static bool hasEnough(const std::shared_ptr<Buddy> &buddy,
const BigInt &aggregatedAmount,
int inputCount,
Expand All @@ -93,6 +99,8 @@ namespace ledger {
BigInt amount,
const api::Currency &currency,
std::function<bool(BitcoinLikeUtxo &, BitcoinLikeUtxo &)> const &functor);

bool _useConfirmedFirst{true};
};
} // namespace core
} // namespace ledger
Expand Down
Loading

0 comments on commit e35f2ba

Please sign in to comment.