Fix fee calculation for OptimizeSize UTXO picking strategy (#924)

* FeeIsEnoughFor{P2WPKH, P2WSH, P2TR} tests
* Fix for fee calculation in `OptimizeSize` strategy
* `highest_first_limit_utxo` and `limit_utxo` strategies are removed because aren't used by WD

Author: viktorb-ledger

core/idl/wallet/bitcoin/bitcoin_like_wallet.djinni

@@ -230,8 +230,6 @@ BitcoinLikePickingStrategy = enum {
     deep_outputs_first;
     optimize_size;
     merge_outputs;
-    highest_first_limit_utxo;
-    limit_utxo;
 }
 
 BitcoinLikeTransactionBuilder = interface +c {

core/src/api/BitcoinLikePickingStrategy.cpp

@@ -569,15 +569,6 @@ namespace ledger {
                         sum = sum + utxo.value;
                     }
                 } break;
-                case api::BitcoinLikePickingStrategy::HIGHEST_FIRST_LIMIT_UTXO:
-                case api::BitcoinLikePickingStrategy::LIMIT_UTXO: {
-                    std::sort(utxos.begin(), utxos.end(), [](auto &lhs, auto &rhs) {
-                        return lhs.value.toInt64() > rhs.value.toInt64();
-                    });
-                    for (int i = 0; i < utxos.size() && i < maxUtxos.value_or(0); ++i) {
-                        sum = sum + utxos[i].value;
-                    }
-                } break;
                 default: {
                     throw make_exception(api::ErrorCode::INVALID_ARGUMENT, "unknown strategy");
                 }

core/src/api/BitcoinLikePickingStrategy.hpp

@@ -39,6 +39,7 @@
 #include <math/bech32/Bech32Factory.h>
 #include <utils/hex.h>
 #include <wallet/bitcoin/networks.hpp>
+#include <wallet/currencies.hpp>
 
 namespace ledger {
     namespace core {
@@ -127,6 +128,13 @@ namespace ledger {
             BitcoinLikeScript script;
             if (a->isP2WPKH() || a->isP2WSH()) {
                 script << btccore::OP_0 << a->getHash160();
+            } else if (a->isP2TR()) {
+                if (currency.name != currencies::BITCOIN.name &&
+                    currency.name != currencies::BITCOIN_TESTNET.name &&
+                    currency.name != currencies::BITCOIN_REGTEST.name) {
+                    throw make_exception(api::ErrorCode::UNSUPPORTED_OPERATION, "Can't generate script from Taproot address ({}) in currency {}", address, currency.name);
+                }
+                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;

core/src/wallet/bitcoin/BitcoinLikeAccount.cpp

@@ -67,18 +67,6 @@ namespace ledger {
                                                                     const std::vector<BitcoinLikeUtxo> &utxo,
                                                                     const BigInt &aggregatedAmount,
                                                                     const api::Currency &currrency);
-            static std::vector<BitcoinLikeUtxo> filterWithHighestFirstLimitUtxo(
-                const std::shared_ptr<BitcoinLikeUtxoPicker::Buddy> &buddy,
-                std::vector<BitcoinLikeUtxo> utxos,
-                const BigInt &aggregatedAmount,
-                const api::Currency &currency,
-                const optional<int32_t> &maxUtxo);
-            static std::vector<BitcoinLikeUtxo> filterWithLimitUtxo(
-                const std::shared_ptr<BitcoinLikeUtxoPicker::Buddy> &buddy,
-                std::vector<BitcoinLikeUtxo> utxos,
-                const BigInt &aggregatedAmount,
-                const api::Currency &currency,
-                const optional<int32_t> &maxUtxo);
             static bool hasEnough(const std::shared_ptr<Buddy> &buddy,
                                   const BigInt &aggregatedAmount,
                                   int inputCount,

core/src/wallet/bitcoin/scripts/BitcoinLikeScript.cpp

@@ -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>
@@ -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))){};
@@ -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");
@@ -185,3 +184,63 @@ 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 int64_t feesPerByte) {
+    const api::Currency currency             = currencies::BITCOIN_TESTNET;
+    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;
+
+    std::cerr << "transactionFees     == " << transactionFees << std::endl;
+    std::cerr << "minimumRequiredFees == " << minimumRequiredFees << std::endl;
+
+    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
+
+    for (int64_t feesPerByte = 1; feesPerByte < 1000000; feesPerByte *= 10)
+        feeIsEnoughFor(address, targetOutputSizeInBytes, feesPerByte);
+}
+
+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
+
+    for (int64_t feesPerByte = 1; feesPerByte < 1000000; feesPerByte *= 10)
+        feeIsEnoughFor(address, targetOutputSizeInBytes, feesPerByte);
+}
+
+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
+
+    for (int64_t feesPerByte = 1; feesPerByte < 1000000; feesPerByte *= 10)
+        feeIsEnoughFor(address, targetOutputSizeInBytes, feesPerByte);
+}

core/src/wallet/bitcoin/transaction_builders/BitcoinLikeStrategyUtxoPicker.cpp

@@ -104,68 +104,6 @@ TEST_F(CoinSelectionP2PKH, PickMultipleUTXO) {
     EXPECT_EQ(tx->getOutputs().at(0)->getValue()->toLong(), 70000000);
 }
 
-TEST_F(CoinSelectionP2PKH, HighestFirstEnough) {
-    auto builder = tx_builder();
-    builder->sendToAddress(api::Amount::fromLong(currency, 70000000), "2MvuUMAG1NFQmmM69Writ6zTsYCnQHFG9BF");
-    builder->pickInputs(api::BitcoinLikePickingStrategy::HIGHEST_FIRST_LIMIT_UTXO, 0xFFFFFFFF, optional<int32_t>(2));
-    builder->setFeesPerByte(api::Amount::fromLong(currency, 0));
-    auto f  = builder->build();
-    auto tx = uv::wait(f);
-
-    EXPECT_LE(tx->getInputs().size(), 2);
-    EXPECT_EQ(tx->getInputs().at(0)->getValue()->toLong(), 50000000);
-    EXPECT_EQ(tx->getInputs().at(1)->getValue()->toLong(), 40000000);
-
-    EXPECT_EQ(tx->getOutputs().size(), 2);
-    EXPECT_EQ(tx->getOutputs().at(0)->getValue()->toLong(), 70000000);
-}
-
-TEST_F(CoinSelectionP2PKH, HighestFirstNotEnough) {
-    auto builder = tx_builder();
-    builder->sendToAddress(api::Amount::fromLong(currency, 100000000), "2MvuUMAG1NFQmmM69Writ6zTsYCnQHFG9BF");
-    builder->pickInputs(api::BitcoinLikePickingStrategy::HIGHEST_FIRST_LIMIT_UTXO, 0xFFFFFFFF, optional<int32_t>(2));
-    builder->setFeesPerByte(api::Amount::fromLong(currency, 0));
-    auto f = builder->build();
-    EXPECT_THROW(uv::wait(f), Exception);
-}
-
-TEST_F(CoinSelectionP2PKH, LimitUtxoPickStrategy) {
-    auto build = [=](int64_t amount) {
-        auto builder = tx_builder();
-        builder->sendToAddress(api::Amount::fromLong(currency, amount), "2MvuUMAG1NFQmmM69Writ6zTsYCnQHFG9BF");
-        builder->pickInputs(api::BitcoinLikePickingStrategy::LIMIT_UTXO, 0xFFFFFFFF, optional<int32_t>(2));
-        builder->setFeesPerByte(api::Amount::fromLong(currency, 0));
-        auto f = builder->build();
-        return uv::wait(f);
-    };
-
-    { // test 1
-        auto tx = build(70000000);
-        EXPECT_LE(tx->getInputs().size(), 2);
-        EXPECT_EQ(tx->getInputs().at(0)->getValue()->toLong(), 50000000);
-        EXPECT_EQ(tx->getInputs().at(1)->getValue()->toLong(), 40000000);
-        EXPECT_EQ(tx->getOutputs().size(), 2);
-        EXPECT_EQ(tx->getOutputs().at(0)->getValue()->toLong(), 70000000);
-    }
-    { // test 2
-        auto tx = build(50000000);
-        EXPECT_LE(tx->getInputs().size(), 1);
-        EXPECT_EQ(tx->getInputs().at(0)->getValue()->toLong(), 50000000);
-        EXPECT_EQ(tx->getOutputs().size(), 1);
-        EXPECT_EQ(tx->getOutputs().at(0)->getValue()->toLong(), 50000000);
-    }
-    { // test 3
-        EXPECT_THROW(build(100000000), Exception);
-    }
-    { // test 4
-        auto tx = build(10000000);
-        EXPECT_LE(tx->getInputs().size(), 1);
-        EXPECT_EQ(tx->getInputs().at(0)->getValue()->toLong(), 10000000);
-        EXPECT_EQ(tx->getOutputs().size(), 1);
-        EXPECT_EQ(tx->getOutputs().at(0)->getValue()->toLong(), 10000000);
-    }
-}
-
 TEST_F(CoinSelectionP2PKH, maxSpendable) {
     auto balance = uv::wait(account->getBalance());
     EXPECT_EQ(balance->toLong(), uv::wait(
@@ -177,15 +115,6 @@ TEST_F(CoinSelectionP2PKH, maxSpendable) {
     EXPECT_EQ(balance->toLong(), uv::wait(
                                      account->getMaxSpendable(api::BitcoinLikePickingStrategy::MERGE_OUTPUTS, optional<int32_t>()))
                                      ->toLong());
-    EXPECT_EQ(120000000, uv::wait(
-                             account->getMaxSpendable(api::BitcoinLikePickingStrategy::HIGHEST_FIRST_LIMIT_UTXO, optional<int32_t>(3)))
-                             ->toLong());
-    EXPECT_EQ(120000000, uv::wait(
-                             account->getMaxSpendable(api::BitcoinLikePickingStrategy::LIMIT_UTXO, optional<int32_t>(3)))
-                             ->toLong());
-    EXPECT_EQ(150000000, uv::wait(
-                             account->getMaxSpendable(api::BitcoinLikePickingStrategy::LIMIT_UTXO, optional<int32_t>(7)))
-                             ->toLong());
 }
 
 TEST_F(CoinSelectionP2PKH, PickAllUTXO) {