Skip to content

Commit f37bd15

Browse files
committed
Merge bitcoin/bitcoin#25685: wallet: Faster transaction creation by removing pre-set-inputs fetching responsibility from Coin Selection
3fcb545 bench: benchmark transaction creation process (furszy) a8a7534 wallet: SelectCoins, return early if target is covered by preset-inputs (furszy) f41712a wallet: simplify preset inputs selection target check (furszy) 5baedc3 wallet: remove fetch pre-selected-inputs responsibility from SelectCoins (furszy) 295852f wallet: encapsulate pre-selected-inputs lookup into its own function (furszy) 37e7887 wallet: skip manually selected coins from 'AvailableCoins' result (furszy) 94c0766 wallet: skip available coins fetch if "other inputs" are disallowed (furszy) Pull request description: #### # Context (Current Flow on Master) In the transaction creation process, in order to select which coins the new transaction will spend, we first obtain all the available coins known by the wallet, which means walking-through the wallet txes map, gathering the ones that fulfill certain spendability requirements in a vector. This coins vector is then provided to the Coin Selection process, which first checks if the user has manually selected any input (which could be internal, aka known by the wallet, or external), and if it does, it fetches them by searching each of them inside the wallet and/or inside the Coin Control external tx data. Then, after finding the pre-selected-inputs and gathering them in a vector, the Coin Selection process walks-through the entire available coins vector once more just to erase coins that are in both vectors. So the Coin Selection process doesn’t pick them twice (duplicate inputs inside the same transaction). #### # Process Workflow Changes Now, a new method, `FetchCoins` will be responsible for: 1) Lookup the user pre-selected-inputs (which can be internal or external). 2) And, fetch the available coins in the wallet (excluding the already fetched ones). Which will occur prior to the Coin Selection process. Which allows us to never include the pre-selected-inputs inside the available coins vector in the first place, as well as doing other nice improvements (written below). So, Coin Selection can perform its main responsibility without mixing it with having to fetch internal/external coins nor any slow and unneeded duplicate coins verification. #### # Summarizing the Improvements: 1) If any pre-selected-input lookup fail, the process will return the error right away. (before, the wallet was fetching all the wallet available coins, walking through the entire txes map, and then failing for an invalid pre-selected-input inside SelectCoins) 2) The pre-selected-inputs lookup failure causes are properly described on the return error. (before, we were returning an "Insufficient Funds" error for everything, even if the failure was due a not solvable external input) 3) **Faster Coin Selection**: no longer need to "remove the pre-set inputs from the available coins vector so that Coin Selection doesn't pick them" (which meant to loop-over the entire available coins vector at Coin Selection time, erasing duplicate coins that were pre-selected). Now, the available coins vector, which is built after the pre-selected-inputs fetching, doesn’t include the already selected inputs in the first place. 4) **Faster transaction creation** for transactions that only use manually selected inputs. We now will return early, as soon as we finish fetching the pre-selected-inputs and not perform the resources expensive calculation of walking-through the entire wallet txes map to obtain the available coins (coins that we will not use). --------------------------- Added a new bench (f6d0bb2) measuring the transaction creation process, for a wallet with ~250k UTXO, only using the pre-selected-inputs inside coin control. Setting `m_allow_other_inputs=false` to disallow the wallet to include coins automatically. #### Result on this PR (tip f6d0bb2d): | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 1,048,675.00 | 953.58 | 0.3% | 0.06 | `WalletCreateTransaction` vs #### Result on master (tip 4a4289e): | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 96,373,458.20 | 10.38 | 0.2% | 5.30 | `WalletCreateTransaction` The benchmark took to run in master: **96.37 milliseconds**, while in this PR: **1 millisecond** 🚀 . ACKs for top commit: S3RK: Code Review ACK 3fcb545 achow101: ACK 3fcb545 aureleoules: reACK 3fcb545 Tree-SHA512: 42f833e92f40c348007ca565a4c98039e6f1ff25d8322bc2b27115824744779baf0b0a38452e4e2cdcba45076473f1028079bbd0f670020481ec5d3db42e4731
2 parents 551c8e9 + 3fcb545 commit f37bd15

14 files changed

+330
-109
lines changed

src/Makefile.bench.include

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ if ENABLE_WALLET
7979
bench_bench_bitcoin_SOURCES += bench/coin_selection.cpp
8080
bench_bench_bitcoin_SOURCES += bench/wallet_balance.cpp
8181
bench_bench_bitcoin_SOURCES += bench/wallet_loading.cpp
82+
bench_bench_bitcoin_SOURCES += bench/wallet_create_tx.cpp
8283
bench_bench_bitcoin_LDADD += $(BDB_LIBS) $(SQLITE_LIBS)
8384
endif
8485

src/bench/wallet_create_tx.cpp

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
// Copyright (c) 2022 The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or https://www.opensource.org/licenses/mit-license.php.
4+
5+
#include <bench/bench.h>
6+
#include <chainparams.h>
7+
#include <wallet/coincontrol.h>
8+
#include <consensus/merkle.h>
9+
#include <kernel/chain.h>
10+
#include <node/context.h>
11+
#include <test/util/setup_common.h>
12+
#include <test/util/wallet.h>
13+
#include <validation.h>
14+
#include <wallet/spend.h>
15+
#include <wallet/wallet.h>
16+
17+
using wallet::CWallet;
18+
using wallet::CreateMockWalletDatabase;
19+
using wallet::DBErrors;
20+
using wallet::WALLET_FLAG_DESCRIPTORS;
21+
22+
struct TipBlock
23+
{
24+
uint256 prev_block_hash;
25+
int64_t prev_block_time;
26+
int tip_height;
27+
};
28+
29+
TipBlock getTip(const CChainParams& params, const node::NodeContext& context)
30+
{
31+
auto tip = WITH_LOCK(::cs_main, return context.chainman->ActiveTip());
32+
return (tip) ? TipBlock{tip->GetBlockHash(), tip->GetBlockTime(), tip->nHeight} :
33+
TipBlock{params.GenesisBlock().GetHash(), params.GenesisBlock().GetBlockTime(), 0};
34+
}
35+
36+
void generateFakeBlock(const CChainParams& params,
37+
const node::NodeContext& context,
38+
CWallet& wallet,
39+
const CScript& coinbase_out_script)
40+
{
41+
TipBlock tip{getTip(params, context)};
42+
43+
// Create block
44+
CBlock block;
45+
CMutableTransaction coinbase_tx;
46+
coinbase_tx.vin.resize(1);
47+
coinbase_tx.vin[0].prevout.SetNull();
48+
coinbase_tx.vout.resize(2);
49+
coinbase_tx.vout[0].scriptPubKey = coinbase_out_script;
50+
coinbase_tx.vout[0].nValue = 49 * COIN;
51+
coinbase_tx.vin[0].scriptSig = CScript() << ++tip.tip_height << OP_0;
52+
coinbase_tx.vout[1].scriptPubKey = coinbase_out_script; // extra output
53+
coinbase_tx.vout[1].nValue = 1 * COIN;
54+
block.vtx = {MakeTransactionRef(std::move(coinbase_tx))};
55+
56+
block.nVersion = VERSIONBITS_LAST_OLD_BLOCK_VERSION;
57+
block.hashPrevBlock = tip.prev_block_hash;
58+
block.hashMerkleRoot = BlockMerkleRoot(block);
59+
block.nTime = ++tip.prev_block_time;
60+
block.nBits = params.GenesisBlock().nBits;
61+
block.nNonce = 0;
62+
63+
{
64+
LOCK(::cs_main);
65+
// Add it to the index
66+
CBlockIndex* pindex{context.chainman->m_blockman.AddToBlockIndex(block, context.chainman->m_best_header)};
67+
// add it to the chain
68+
context.chainman->ActiveChain().SetTip(*pindex);
69+
}
70+
71+
// notify wallet
72+
const auto& pindex = WITH_LOCK(::cs_main, return context.chainman->ActiveChain().Tip());
73+
wallet.blockConnected(kernel::MakeBlockInfo(pindex, &block));
74+
}
75+
76+
struct PreSelectInputs {
77+
// How many coins from the wallet the process should select
78+
int num_of_internal_inputs;
79+
// future: this could have external inputs as well.
80+
};
81+
82+
static void WalletCreateTx(benchmark::Bench& bench, const OutputType output_type, bool allow_other_inputs, std::optional<PreSelectInputs> preset_inputs)
83+
{
84+
const auto test_setup = MakeNoLogFileContext<const TestingSetup>();
85+
86+
CWallet wallet{test_setup->m_node.chain.get(), "", gArgs, CreateMockWalletDatabase()};
87+
{
88+
LOCK(wallet.cs_wallet);
89+
wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
90+
wallet.SetupDescriptorScriptPubKeyMans();
91+
if (wallet.LoadWallet() != DBErrors::LOAD_OK) assert(false);
92+
}
93+
94+
// Generate destinations
95+
CScript dest = GetScriptForDestination(getNewDestination(wallet, output_type));
96+
97+
// Generate chain; each coinbase will have two outputs to fill-up the wallet
98+
const auto& params = Params();
99+
unsigned int chain_size = 5000; // 5k blocks means 10k UTXO for the wallet (minus 200 due COINBASE_MATURITY)
100+
for (unsigned int i = 0; i < chain_size; ++i) {
101+
generateFakeBlock(params, test_setup->m_node, wallet, dest);
102+
}
103+
104+
// Check available balance
105+
auto bal = wallet::GetAvailableBalance(wallet); // Cache
106+
assert(bal == 50 * COIN * (chain_size - COINBASE_MATURITY));
107+
108+
wallet::CCoinControl coin_control;
109+
coin_control.m_allow_other_inputs = allow_other_inputs;
110+
111+
CAmount target = 0;
112+
if (preset_inputs) {
113+
// Select inputs, each has 49 BTC
114+
const auto& res = WITH_LOCK(wallet.cs_wallet,
115+
return wallet::AvailableCoins(wallet, nullptr, std::nullopt, 1, MAX_MONEY,
116+
MAX_MONEY, preset_inputs->num_of_internal_inputs));
117+
for (int i=0; i < preset_inputs->num_of_internal_inputs; i++) {
118+
const auto& coin{res.coins.at(output_type)[i]};
119+
target += coin.txout.nValue;
120+
coin_control.Select(coin.outpoint);
121+
}
122+
}
123+
124+
// If automatic coin selection is enabled, add the value of another UTXO to the target
125+
if (coin_control.m_allow_other_inputs) target += 50 * COIN;
126+
std::vector<wallet::CRecipient> recipients = {{dest, target, true}};
127+
128+
bench.epochIterations(5).run([&] {
129+
LOCK(wallet.cs_wallet);
130+
const auto& tx_res = CreateTransaction(wallet, recipients, -1, coin_control);
131+
assert(tx_res);
132+
});
133+
}
134+
135+
static void WalletCreateTxUseOnlyPresetInputs(benchmark::Bench& bench) { WalletCreateTx(bench, OutputType::BECH32, /*allow_other_inputs=*/false,
136+
{{/*num_of_internal_inputs=*/4}}); }
137+
138+
static void WalletCreateTxUsePresetInputsAndCoinSelection(benchmark::Bench& bench) { WalletCreateTx(bench, OutputType::BECH32, /*allow_other_inputs=*/true,
139+
{{/*num_of_internal_inputs=*/4}}); }
140+
141+
BENCHMARK(WalletCreateTxUseOnlyPresetInputs, benchmark::PriorityLevel::LOW)
142+
BENCHMARK(WalletCreateTxUsePresetInputsAndCoinSelection, benchmark::PriorityLevel::LOW)

src/qt/sendcoinsdialog.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -289,7 +289,9 @@ bool SendCoinsDialog::PrepareSendText(QString& question_string, QString& informa
289289

290290
updateCoinControlState();
291291

292-
prepareStatus = model->prepareTransaction(*m_current_transaction, *m_coin_control);
292+
CCoinControl coin_control = *m_coin_control;
293+
coin_control.m_allow_other_inputs = !coin_control.HasSelected(); // future, could introduce a checkbox to customize this value.
294+
prepareStatus = model->prepareTransaction(*m_current_transaction, coin_control);
293295

294296
// process prepareStatus and on error generate message shown to user
295297
processSendCoinsReturn(prepareStatus,

src/test/util/wallet.cpp

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,12 @@ const std::string ADDRESS_BCRT1_UNSPENDABLE = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqq
2121
std::string getnewaddress(CWallet& w)
2222
{
2323
constexpr auto output_type = OutputType::BECH32;
24-
return EncodeDestination(*Assert(w.GetNewDestination(output_type, "")));
24+
return EncodeDestination(getNewDestination(w, output_type));
25+
}
26+
27+
CTxDestination getNewDestination(CWallet& w, OutputType output_type)
28+
{
29+
return *Assert(w.GetNewDestination(output_type, ""));
2530
}
2631

2732
#endif // ENABLE_WALLET

src/test/util/wallet.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#ifndef BITCOIN_TEST_UTIL_WALLET_H
66
#define BITCOIN_TEST_UTIL_WALLET_H
77

8+
#include <outputtype.h>
89
#include <string>
910

1011
namespace wallet {
@@ -19,8 +20,10 @@ extern const std::string ADDRESS_BCRT1_UNSPENDABLE;
1920

2021
/** Import the address to the wallet */
2122
void importaddress(wallet::CWallet& wallet, const std::string& address);
22-
/** Returns a new address from the wallet */
23+
/** Returns a new encoded destination from the wallet (hardcoded to BECH32) */
2324
std::string getnewaddress(wallet::CWallet& w);
25+
/** Returns a new destination, of an specific type, from the wallet */
26+
CTxDestination getNewDestination(wallet::CWallet& w, OutputType output_type);
2427

2528

2629
#endif // BITCOIN_TEST_UTIL_WALLET_H

src/wallet/coincontrol.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ class CCoinControl
3737
bool m_include_unsafe_inputs = false;
3838
//! If true, the selection process can add extra unselected inputs from the wallet
3939
//! while requires all selected inputs be used
40-
bool m_allow_other_inputs = false;
40+
bool m_allow_other_inputs = true;
4141
//! Includes watch only addresses which are solvable
4242
bool fAllowWatchOnly = false;
4343
//! Override automatic min/max checks on fee, m_feerate must be set if true

src/wallet/coinselection.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -444,6 +444,12 @@ void SelectionResult::AddInput(const OutputGroup& group)
444444
m_use_effective = !group.m_subtract_fee_outputs;
445445
}
446446

447+
void SelectionResult::AddInputs(const std::set<COutput>& inputs, bool subtract_fee_outputs)
448+
{
449+
util::insert(m_selected_inputs, inputs);
450+
m_use_effective = !subtract_fee_outputs;
451+
}
452+
447453
void SelectionResult::Merge(const SelectionResult& other)
448454
{
449455
m_target += other.m_target;

src/wallet/coinselection.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,7 @@ struct SelectionResult
308308
void Clear();
309309

310310
void AddInput(const OutputGroup& group);
311+
void AddInputs(const std::set<COutput>& inputs, bool subtract_fee_outputs);
311312

312313
/** Calculates and stores the waste for this selection via GetSelectionWaste */
313314
void ComputeAndSetWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee);

0 commit comments

Comments
 (0)