diff options
author | Andrew Chow <github@achow101.com> | 2022-10-27 17:48:51 -0400 |
---|---|---|
committer | Andrew Chow <github@achow101.com> | 2022-10-27 17:48:58 -0400 |
commit | f37bd15d472fdc7dd3d40cafaba9e8dfddd6b530 (patch) | |
tree | 1fd269b38a457713caf52c44e2e977b68b3885e3 /test | |
parent | 551c8e9526d2502f857e1ef6348c7f1380f37443 (diff) | |
parent | 3fcb545ab26be3e785b5e5654be0bdc77099d827 (diff) | |
download | bitcoin-f37bd15d472fdc7dd3d40cafaba9e8dfddd6b530.tar.xz |
Merge bitcoin/bitcoin#25685: wallet: Faster transaction creation by removing pre-set-inputs fetching responsibility from Coin Selection
3fcb545ab26be3e785b5e5654be0bdc77099d827 bench: benchmark transaction creation process (furszy)
a8a75346d7e7247596c8a580d65ceaad49c97b97 wallet: SelectCoins, return early if target is covered by preset-inputs (furszy)
f41712a734dc119f8a5e053a9cfa1f0411b5e8f1 wallet: simplify preset inputs selection target check (furszy)
5baedc33519661af9d19efcefd23dca8998d2547 wallet: remove fetch pre-selected-inputs responsibility from SelectCoins (furszy)
295852f61998a025b0b28a0671e6e1cf0dc08d0d wallet: encapsulate pre-selected-inputs lookup into its own function (furszy)
37e7887cb4bfd7db6eb462ed0741c45aea22a990 wallet: skip manually selected coins from 'AvailableCoins' result (furszy)
94c0766b0cd1990c1399a745c88c2ba4c685d8d1 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 4a4289e2):
| 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 3fcb545ab26be3e785b5e5654be0bdc77099d827
achow101:
ACK 3fcb545ab26be3e785b5e5654be0bdc77099d827
aureleoules:
reACK 3fcb545ab26be3e785b5e5654be0bdc77099d827
Tree-SHA512: 42f833e92f40c348007ca565a4c98039e6f1ff25d8322bc2b27115824744779baf0b0a38452e4e2cdcba45076473f1028079bbd0f670020481ec5d3db42e4731
Diffstat (limited to 'test')
-rwxr-xr-x | test/functional/rpc_fundrawtransaction.py | 16 | ||||
-rwxr-xr-x | test/functional/rpc_psbt.py | 2 | ||||
-rwxr-xr-x | test/functional/wallet_send.py | 2 |
3 files changed, 16 insertions, 4 deletions
diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 17c6fce9c2..54b42667bb 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -406,7 +406,9 @@ class RawTransactionsTest(BitcoinTestFramework): def test_invalid_input(self): self.log.info("Test fundrawtxn with an invalid vin") - inputs = [ {'txid' : "1c7f966dab21119bac53213a2bc7532bff1fa844c124fd750a7d0b1332440bd1", 'vout' : 0} ] #invalid vin! + txid = "1c7f966dab21119bac53213a2bc7532bff1fa844c124fd750a7d0b1332440bd1" + vout = 0 + inputs = [ {'txid' : txid, 'vout' : vout} ] #invalid vin! outputs = { self.nodes[0].getnewaddress() : 1.0} rawtx = self.nodes[2].createrawtransaction(inputs, outputs) assert_raises_rpc_error(-4, "Unable to find UTXO for external input", self.nodes[2].fundrawtransaction, rawtx) @@ -1011,7 +1013,7 @@ class RawTransactionsTest(BitcoinTestFramework): # An external input without solving data should result in an error raw_tx = wallet.createrawtransaction([ext_utxo], {self.nodes[0].getnewaddress(): ext_utxo["amount"] / 2}) - assert_raises_rpc_error(-4, "Insufficient funds", wallet.fundrawtransaction, raw_tx) + assert_raises_rpc_error(-4, "Not solvable pre-selected input COutPoint(%s, %s)" % (ext_utxo["txid"][0:10], ext_utxo["vout"]), wallet.fundrawtransaction, raw_tx) # Error conditions assert_raises_rpc_error(-5, "'not a pubkey' is not hex", wallet.fundrawtransaction, raw_tx, {"solving_data": {"pubkeys":["not a pubkey"]}}) @@ -1095,6 +1097,8 @@ class RawTransactionsTest(BitcoinTestFramework): # Expect: only preset inputs are used. # 5. Explicit add_inputs=true, no preset inputs (same as (1) but with an explicit set): # Expect: include inputs from the wallet. + # 6. Explicit add_inputs=false, no preset inputs: + # Expect: failure as we did not provide inputs and the process cannot automatically select coins. # Case (1), 'send' command # 'add_inputs' value is true unless "inputs" are specified, in such case, add_inputs=false. @@ -1146,6 +1150,10 @@ class RawTransactionsTest(BitcoinTestFramework): tx = wallet.send(outputs=[{addr1: 8}], options=options) assert tx["complete"] + # 6. Explicit add_inputs=false, no preset inputs: + options = {"add_inputs": False} + assert_raises_rpc_error(-4, "Insufficient funds", wallet.send, outputs=[{addr1: 3}], options=options) + ################################################ # Case (1), 'walletcreatefundedpsbt' command @@ -1187,6 +1195,10 @@ class RawTransactionsTest(BitcoinTestFramework): } assert "psbt" in wallet.walletcreatefundedpsbt(inputs=[], outputs=outputs, options=options) + # Case (6). Explicit add_inputs=false, no preset inputs: + options = {"add_inputs": False} + assert_raises_rpc_error(-4, "Insufficient funds", wallet.walletcreatefundedpsbt, inputs=[], outputs=outputs, options=options) + self.nodes[2].unloadwallet("test_preset_inputs") def test_weight_calculation(self): diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 3b78a7d095..b79b8f5187 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -657,7 +657,7 @@ class PSBTTest(BitcoinTestFramework): ext_utxo = self.nodes[0].listunspent(addresses=[addr])[0] # An external input without solving data should result in an error - assert_raises_rpc_error(-4, "Insufficient funds", wallet.walletcreatefundedpsbt, [ext_utxo], {self.nodes[0].getnewaddress(): 15}) + assert_raises_rpc_error(-4, "Not solvable pre-selected input COutPoint(%s, %s)" % (ext_utxo["txid"][0:10], ext_utxo["vout"]), wallet.walletcreatefundedpsbt, [ext_utxo], {self.nodes[0].getnewaddress(): 15}) # But funding should work when the solving data is provided psbt = wallet.walletcreatefundedpsbt([ext_utxo], {self.nodes[0].getnewaddress(): 15}, 0, {"add_inputs": True, "solving_data": {"pubkeys": [addr_info['pubkey']], "scripts": [addr_info["embedded"]["scriptPubKey"], addr_info["embedded"]["embedded"]["scriptPubKey"]]}}) diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py index 07baa0595e..fb759c153d 100755 --- a/test/functional/wallet_send.py +++ b/test/functional/wallet_send.py @@ -508,7 +508,7 @@ class WalletSendTest(BitcoinTestFramework): ext_utxo = ext_fund.listunspent(addresses=[addr])[0] # An external input without solving data should result in an error - self.test_send(from_wallet=ext_wallet, to_wallet=self.nodes[0], amount=15, inputs=[ext_utxo], add_inputs=True, psbt=True, include_watching=True, expect_error=(-4, "Insufficient funds")) + self.test_send(from_wallet=ext_wallet, to_wallet=self.nodes[0], amount=15, inputs=[ext_utxo], add_inputs=True, psbt=True, include_watching=True, expect_error=(-4, "Not solvable pre-selected input COutPoint(%s, %s)" % (ext_utxo["txid"][0:10], ext_utxo["vout"]))) # But funding should work when the solving data is provided res = self.test_send(from_wallet=ext_wallet, to_wallet=self.nodes[0], amount=15, inputs=[ext_utxo], add_inputs=True, psbt=True, include_watching=True, solving_data={"pubkeys": [addr_info['pubkey']], "scripts": [addr_info["embedded"]["scriptPubKey"], addr_info["embedded"]["embedded"]["scriptPubKey"]]}) |