From 912f1ed181161b0365776cd490b63137aaad708a Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 21 Mar 2022 14:19:10 -0400 Subject: [PATCH 1/5] wallet: track which coin selection algorithm produced a SelectionResult --- src/wallet/coinselection.cpp | 19 ++++++++++++++++--- src/wallet/coinselection.h | 16 ++++++++++++++-- src/wallet/spend.cpp | 4 ++-- src/wallet/test/coinselector_tests.cpp | 2 +- 4 files changed, 33 insertions(+), 8 deletions(-) diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 433759e086f..0b236e2e484 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -64,7 +64,7 @@ static const size_t TOTAL_TRIES = 100000; std::optional SelectCoinsBnB(std::vector& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change) { - SelectionResult result(selection_target); + SelectionResult result(selection_target, SelectionAlgorithm::BNB); CAmount curr_value = 0; std::vector curr_selection; // selected utxo indexes @@ -167,7 +167,7 @@ std::optional SelectCoinsBnB(std::vector& utxo_poo std::optional SelectCoinsSRD(const std::vector& utxo_pool, CAmount target_value, FastRandomContext& rng) { - SelectionResult result(target_value); + SelectionResult result(target_value, SelectionAlgorithm::SRD); std::vector indexes; indexes.resize(utxo_pool.size()); @@ -249,7 +249,7 @@ static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::v std::optional KnapsackSolver(std::vector& groups, const CAmount& nTargetValue, CAmount change_target, FastRandomContext& rng) { - SelectionResult result(nTargetValue); + SelectionResult result(nTargetValue, SelectionAlgorithm::KNAPSACK); // List of values less than target std::optional lowest_larger; @@ -460,4 +460,17 @@ std::string COutput::ToString() const { return strprintf("COutput(%s, %d, %d) [%s]", outpoint.hash.ToString(), outpoint.n, depth, FormatMoney(txout.nValue)); } + +std::string GetAlgorithmName(const SelectionAlgorithm algo) +{ + switch (algo) + { + case SelectionAlgorithm::BNB: return "bnb"; + case SelectionAlgorithm::KNAPSACK: return "knapsack"; + case SelectionAlgorithm::SRD: return "srd"; + case SelectionAlgorithm::MANUAL: return "manual"; + // No default case to allow for compiler to warn + } + assert(false); +} } // namespace wallet diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index c1484c0a577..44eec9a4172 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -239,6 +239,16 @@ struct OutputGroup */ [[nodiscard]] CAmount GenerateChangeTarget(CAmount payment_value, FastRandomContext& rng); +enum class SelectionAlgorithm : uint8_t +{ + BNB = 0, + KNAPSACK = 1, + SRD = 2, + MANUAL = 3, +}; + +std::string GetAlgorithmName(const SelectionAlgorithm algo); + struct SelectionResult { private: @@ -250,10 +260,12 @@ private: bool m_use_effective{false}; /** The computed waste */ std::optional m_waste; + /** The algorithm used to produce this result */ + SelectionAlgorithm m_algo; public: - explicit SelectionResult(const CAmount target) - : m_target(target) {} + explicit SelectionResult(const CAmount target, SelectionAlgorithm algo) + : m_target(target), m_algo(algo) {} SelectionResult() = delete; diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 9e508f3a32f..b4b726d4fd9 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -435,7 +435,7 @@ std::optional SelectCoins(const CWallet& wallet, const std::vec */ preset_inputs.Insert(out, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false); } - SelectionResult result(nTargetValue); + SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL); result.AddInput(preset_inputs); if (result.GetSelectedValue() < nTargetValue) return std::nullopt; return result; @@ -519,7 +519,7 @@ std::optional SelectCoins(const CWallet& wallet, const std::vec // permissive CoinEligibilityFilter. std::optional res = [&] { // Pre-selected inputs already cover the target amount. - if (value_to_select <= 0) return std::make_optional(SelectionResult(nTargetValue)); + if (value_to_select <= 0) return std::make_optional(SelectionResult(nTargetValue, SelectionAlgorithm::MANUAL)); // If possible, fund the transaction with confirmed UTXOs only. Prefer at least six // confirmations on outputs received from other wallets and only spend confirmed change. diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 2a08c8ab572..72e749477b1 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -168,7 +168,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) FastRandomContext rand{}; // Setup std::vector utxo_pool; - SelectionResult expected_result(CAmount(0)); + SelectionResult expected_result(CAmount(0), SelectionAlgorithm::BNB); ///////////////////////// // Known Outcome tests // From 15b58383d0029c4ae7b487e03cd451e1580eb91d Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 23 Mar 2022 13:17:07 -0400 Subject: [PATCH 2/5] wallet: compute waste for SelectionResults of preset inputs When we use only manually specified inputs, we should still calculate the waste so that if anything later on calls GetWaste (in order to log it), there won't be an error. --- src/wallet/spend.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index b4b726d4fd9..233478af41e 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -438,6 +438,7 @@ std::optional SelectCoins(const CWallet& wallet, const std::vec SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL); result.AddInput(preset_inputs); if (result.GetSelectedValue() < nTargetValue) return std::nullopt; + result.ComputeAndSetWaste(coin_selection_params.m_cost_of_change); return result; } @@ -573,6 +574,9 @@ std::optional SelectCoins(const CWallet& wallet, const std::vec // Add preset inputs to result res->AddInput(preset_inputs); + if (res->m_algo == SelectionAlgorithm::MANUAL) { + res->ComputeAndSetWaste(coin_selection_params.m_cost_of_change); + } return res; } From 8e3f39e4fa2d8c63bc697c9ebd303965fcccea55 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 21 Mar 2022 14:33:29 -0400 Subject: [PATCH 3/5] wallet: Add some tracepoints for coin selection --- src/wallet/coinselection.h | 9 +++++---- src/wallet/spend.cpp | 5 +++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 44eec9a4172..25c672eda04 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -254,16 +254,17 @@ struct SelectionResult private: /** Set of inputs selected by the algorithm to use in the transaction */ std::set m_selected_inputs; - /** The target the algorithm selected for. Note that this may not be equal to the recipient amount as it can include non-input fees */ - const CAmount m_target; /** Whether the input values for calculations should be the effective value (true) or normal value (false) */ bool m_use_effective{false}; /** The computed waste */ std::optional m_waste; - /** The algorithm used to produce this result */ - SelectionAlgorithm m_algo; public: + /** The target the algorithm selected for. Note that this may not be equal to the recipient amount as it can include non-input fees */ + const CAmount m_target; + /** The algorithm used to produce this result */ + const SelectionAlgorithm m_algo; + explicit SelectionResult(const CAmount target, SelectionAlgorithm algo) : m_target(target), m_algo(algo) {} diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 233478af41e..55c0a2cb7ff 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -792,6 +793,7 @@ static bool CreateTransactionInternal( error = _("Insufficient funds"); return false; } + TRACE5(coin_selection, selected_coins, wallet.GetName().c_str(), GetAlgorithmName(result->m_algo).c_str(), result->m_target, result->GetWaste(), result->GetSelectedValue()); // Always make a change output // We will reduce the fee from this change output later, and remove the output if it is too small. @@ -982,8 +984,10 @@ bool CreateTransaction( int nChangePosIn = nChangePosInOut; Assert(!tx); // tx is an out-param. TODO change the return type from bool to tx (or nullptr) bool res = CreateTransactionInternal(wallet, vecSend, tx, nFeeRet, nChangePosInOut, error, coin_control, fee_calc_out, sign); + TRACE4(coin_selection, normal_create_tx_internal, wallet.GetName().c_str(), res, nFeeRet, nChangePosInOut); // try with avoidpartialspends unless it's enabled already if (res && nFeeRet > 0 /* 0 means non-functional fee rate estimation */ && wallet.m_max_aps_fee > -1 && !coin_control.m_avoid_partial_spends) { + TRACE1(coin_selection, attempting_aps_create_tx, wallet.GetName().c_str()); CCoinControl tmp_cc = coin_control; tmp_cc.m_avoid_partial_spends = true; CAmount nFeeRet2; @@ -994,6 +998,7 @@ bool CreateTransaction( // if fee of this alternative one is within the range of the max fee, we use this one const bool use_aps = nFeeRet2 <= nFeeRet + wallet.m_max_aps_fee; wallet.WalletLogPrintf("Fee non-grouped = %lld, grouped = %lld, using %s\n", nFeeRet, nFeeRet2, use_aps ? "grouped" : "non-grouped"); + TRACE5(coin_selection, aps_create_tx_internal, wallet.GetName().c_str(), use_aps, res, nFeeRet2, nChangePosInOut2); if (use_aps) { tx = tx2; nFeeRet = nFeeRet2; From ca02b68e8a7147f80cbe84b0742908b0b0faa04d Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 14 Apr 2022 12:53:17 -0400 Subject: [PATCH 4/5] doc: document coin selection tracepoints --- doc/tracing.md | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/doc/tracing.md b/doc/tracing.md index 6ec6a6c218a..b6e3b9263a9 100644 --- a/doc/tracing.md +++ b/doc/tracing.md @@ -168,6 +168,49 @@ Arguments passed: 4. Value of the coin as `int64` 5. If the coin is a coinbase as `bool` +### Context `coin_selection` + +#### Tracepoint `coin_selection:selected_coins` + +Is called when `SelectCoins` completes. + +Arguments passed: +1. Wallet name as `pointer to C-style string` +2. Coin selection algorithm name as `pointer to C-style string` +3. Selection target value as `int64` +4. Calculated waste metric of the solution as `int64` +5. Total value of the selected inputs as `int64` + +#### Tracepoint `coin_selection:normal_create_tx_internal` + +Is called when the first `CreateTransactionInternal` completes. + +Arguments passed: +1. Wallet name as `pointer to C-style string` +2. Whether `CreateTransactionInternal` succeeded as `bool` +3. The expected transaction fee as an `int64` +4. The position of the change output as an `int32` + +#### Tracepoint `coin_selection:attempting_aps_create_tx` + +Is called when `CreateTransactionInternal` is called the second time for the optimistic +Avoid Partial Spends selection attempt. This is used to determine whether the next +tracepoints called are for the Avoid Partial Spends solution, or a different transaction. + +Arguments passed: +1. Wallet name as `pointer to C-style string` + +#### Tracepoint `coin_selection:aps_create_tx_internal` + +Is called when the second `CreateTransactionInternal` with Avoid Partial Spends enabled completes. + +Arguments passed: +1. Wallet name as `pointer to C-style string` +2. Whether the Avoid Partial Spends solution will be used as `bool` +3. Whether `CreateTransactionInternal` succeeded as` bool` +4. The expected transaction fee as an `int64` +5. The position of the change output as an `int32` + ## Adding tracepoints to Bitcoin Core To add a new tracepoint, `#include ` in the compilation unit where From ab5af9ca7293239ffc24ea7e23159b8184543f94 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 14 Apr 2022 13:41:18 -0400 Subject: [PATCH 5/5] test: Add test for coinselection tracepoints --- .../interface_usdt_coinselection.py | 208 ++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 209 insertions(+) create mode 100755 test/functional/interface_usdt_coinselection.py diff --git a/test/functional/interface_usdt_coinselection.py b/test/functional/interface_usdt_coinselection.py new file mode 100755 index 00000000000..ef32feda991 --- /dev/null +++ b/test/functional/interface_usdt_coinselection.py @@ -0,0 +1,208 @@ +#!/usr/bin/env python3 +# Copyright (c) 2022 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +""" Tests the coin_selection:* tracepoint API interface. + See https://github.com/bitcoin/bitcoin/blob/master/doc/tracing.md#context-coin_selection +""" + +# Test will be skipped if we don't have bcc installed +try: + from bcc import BPF, USDT # type: ignore[import] +except ImportError: + pass +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, + assert_greater_than, + assert_raises_rpc_error, +) + +coinselection_tracepoints_program = """ +#include + +#define WALLET_NAME_LENGTH 16 +#define ALGO_NAME_LENGTH 16 + +struct event_data +{ + u8 type; + char wallet_name[WALLET_NAME_LENGTH]; + + // selected coins event + char algo[ALGO_NAME_LENGTH]; + s64 target; + s64 waste; + s64 selected_value; + + // create tx event + bool success; + s64 fee; + s32 change_pos; + + // aps create tx event + bool use_aps; +}; + +BPF_QUEUE(coin_selection_events, struct event_data, 1024); + +int trace_selected_coins(struct pt_regs *ctx) { + struct event_data data; + __builtin_memset(&data, 0, sizeof(data)); + data.type = 1; + bpf_usdt_readarg_p(1, ctx, &data.wallet_name, WALLET_NAME_LENGTH); + bpf_usdt_readarg_p(2, ctx, &data.algo, ALGO_NAME_LENGTH); + bpf_usdt_readarg(3, ctx, &data.target); + bpf_usdt_readarg(4, ctx, &data.waste); + bpf_usdt_readarg(5, ctx, &data.selected_value); + coin_selection_events.push(&data, 0); + return 0; +} + +int trace_normal_create_tx(struct pt_regs *ctx) { + struct event_data data; + __builtin_memset(&data, 0, sizeof(data)); + data.type = 2; + bpf_usdt_readarg_p(1, ctx, &data.wallet_name, WALLET_NAME_LENGTH); + bpf_usdt_readarg(2, ctx, &data.success); + bpf_usdt_readarg(3, ctx, &data.fee); + bpf_usdt_readarg(4, ctx, &data.change_pos); + coin_selection_events.push(&data, 0); + return 0; +} + +int trace_attempt_aps(struct pt_regs *ctx) { + struct event_data data; + __builtin_memset(&data, 0, sizeof(data)); + data.type = 3; + bpf_usdt_readarg_p(1, ctx, &data.wallet_name, WALLET_NAME_LENGTH); + coin_selection_events.push(&data, 0); + return 0; +} + +int trace_aps_create_tx(struct pt_regs *ctx) { + struct event_data data; + __builtin_memset(&data, 0, sizeof(data)); + data.type = 4; + bpf_usdt_readarg_p(1, ctx, &data.wallet_name, WALLET_NAME_LENGTH); + bpf_usdt_readarg(2, ctx, &data.use_aps); + bpf_usdt_readarg(3, ctx, &data.success); + bpf_usdt_readarg(4, ctx, &data.fee); + bpf_usdt_readarg(5, ctx, &data.change_pos); + coin_selection_events.push(&data, 0); + return 0; +} +""" + + +class CoinSelectionTracepointTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 1 + self.setup_clean_chain = True + + def skip_test_if_missing_module(self): + self.skip_if_platform_not_linux() + self.skip_if_no_bitcoind_tracepoints() + self.skip_if_no_python_bcc() + self.skip_if_no_bpf_permissions() + self.skip_if_no_wallet() + + def get_tracepoints(self, expected_types): + events = [] + try: + for i in range(0, len(expected_types) + 1): + event = self.bpf["coin_selection_events"].pop() + assert_equal(event.wallet_name.decode(), self.default_wallet_name) + assert_equal(event.type, expected_types[i]) + events.append(event) + else: + # If the loop exits successfully instead of throwing a KeyError, then we have had + # more events than expected. There should be no more than len(expected_types) events. + assert False + except KeyError: + assert_equal(len(events), len(expected_types)) + return events + + + def determine_selection_from_usdt(self, events): + success = None + use_aps = None + algo = None + waste = None + change_pos = None + + is_aps = False + sc_events = [] + for event in events: + if event.type == 1: + if not is_aps: + algo = event.algo.decode() + waste = event.waste + sc_events.append(event) + elif event.type == 2: + success = event.success + if not is_aps: + change_pos = event.change_pos + elif event.type == 3: + is_aps = True + elif event.type == 4: + assert is_aps + if event.use_aps: + use_aps = True + assert_equal(len(sc_events), 2) + algo = sc_events[1].algo.decode() + waste = sc_events[1].waste + change_pos = event.change_pos + return success, use_aps, algo, waste, change_pos + + def run_test(self): + self.log.info("hook into the coin_selection tracepoints") + ctx = USDT(pid=self.nodes[0].process.pid) + ctx.enable_probe(probe="coin_selection:selected_coins", fn_name="trace_selected_coins") + ctx.enable_probe(probe="coin_selection:normal_create_tx_internal", fn_name="trace_normal_create_tx") + ctx.enable_probe(probe="coin_selection:attempting_aps_create_tx", fn_name="trace_attempt_aps") + ctx.enable_probe(probe="coin_selection:aps_create_tx_internal", fn_name="trace_aps_create_tx") + self.bpf = BPF(text=coinselection_tracepoints_program, usdt_contexts=[ctx], debug=0) + + self.log.info("Prepare wallets") + self.generate(self.nodes[0], 101) + wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + + self.log.info("Sending a transaction should result in all tracepoints") + # We should have 5 tracepoints in the order: + # 1. selected_coins (type 1) + # 2. normal_create_tx_internal (type 2) + # 3. attempting_aps_create_tx (type 3) + # 4. selected_coins (type 1) + # 5. aps_create_tx_internal (type 4) + wallet.sendtoaddress(wallet.getnewaddress(), 10) + events = self.get_tracepoints([1, 2, 3, 1, 4]) + success, use_aps, algo, waste, change_pos = self.determine_selection_from_usdt(events) + assert_equal(success, True) + assert_greater_than(change_pos, -1) + + self.log.info("Failing to fund results in 1 tracepoint") + # We should have 1 tracepoints in the order + # 1. normal_create_tx_internal (type 2) + assert_raises_rpc_error(-6, "Insufficient funds", wallet.sendtoaddress, wallet.getnewaddress(), 102 * 50) + events = self.get_tracepoints([2]) + success, use_aps, algo, waste, change_pos = self.determine_selection_from_usdt(events) + assert_equal(success, False) + + self.log.info("Explicitly enabling APS results in 2 tracepoints") + # We should have 2 tracepoints in the order + # 1. selected_coins (type 1) + # 2. normal_create_tx_internal (type 2) + wallet.setwalletflag("avoid_reuse") + wallet.sendtoaddress(address=wallet.getnewaddress(), amount=10, avoid_reuse=True) + events = self.get_tracepoints([1, 2]) + success, use_aps, algo, waste, change_pos = self.determine_selection_from_usdt(events) + assert_equal(success, True) + assert_equal(use_aps, None) + + self.bpf.cleanup() + + +if __name__ == '__main__': + CoinSelectionTracepointTest().main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 1f0f806d919..7a1d1a97765 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -168,6 +168,7 @@ BASE_SCRIPTS = [ 'wallet_reorgsrestore.py', 'interface_http.py', 'interface_rpc.py', + 'interface_usdt_coinselection.py', 'interface_usdt_net.py', 'interface_usdt_utxocache.py', 'interface_usdt_validation.py',