Alternative to #9152.
This takes <n> pubkeys and optionally <n> xpubs (together with a definable lookup windows where the default is 0-1000) and looks up common scripts in the UTXO set of all given or derived keys.
The output will be an array similar to listunspent. That array is compatible with createrawtransaction as well as with signrawtransaction.
This makes it possible to prepare sweeps and have them signed in a secure (cold) space.
1674+ result.pushKV("progress", g_scan_progress);
1675+ }
1676+ else if (request.params[0].get_str() == "abort") {
1677+ CoinsViewScanReserver reserver;
1678+ if (reserver.reserve()) {
1679+ throw JSONRPCError(RPC_INVALID_PARAMETER, "No scan in progress");
bool in the response is enough? Otherwise, missing test.
1631+
1632+UniValue scantxoutset(const JSONRPCRequest& request)
1633+{
1634+ if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
1635+ throw std::runtime_error(
1636+ "scantxoutset <action> {\"pubkeys\": [\"pubkey\",...], \"xpubs\":[{\"xpub\": \"<xpub>\"}], other options}\n"
other options? There are none.
1629+ }
1630+};
1631+
1632+UniValue scantxoutset(const JSONRPCRequest& request)
1633+{
1634+ if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
request.params.size() != 2?
scantxoutset status
Commit “Blockchain/RPC: Add scantxoutset method to scan UTXO set”
Nah. You can also call scantxoutset status
Right.
However should be request.params.size() > 3 (instead of 2)?
1642+ " \"status\" for progress report (in %) of the current scan\n"
1643+ "2. \"options\" (object, optional)\n"
1644+ " \"pubkeys\":[\"pubkey\",...] (array of strings, optional) An array of HEX encoded public keys\n"
1645+ " \"xpubs\": (array of xpub objects that will be used to derive child keys with the given lookup window after m/0/k and m/1/k scheme)\n"
1646+ " [\n"
1647+ " {\"xpub\":\"<xpub>\", (base58check encoded extended public key (xpub)\n"
{.
1667+
1668+ UniValue result(UniValue::VOBJ);
1669+ if (request.params[0].get_str() == "status") {
1670+ CoinsViewScanReserver reserver;
1671+ if (reserver.reserve()) {
1672+ throw JSONRPCError(RPC_INVALID_PARAMETER, "No scan in progress");
-testslowdown or similar). Otherwise I guess it’s hard to properly test this.
status should not raise an error when no scan is in progress.
1820+ unspents.push_back(unspent);
1821+ }
1822+ }
1823+ result.push_back(Pair("unspents", unspents));
1824+ result.push_back(Pair("total_amount", ValueFromAmount(total_in)));
1825+ }
else { here.
1677+ CoinsViewScanReserver reserver;
1678+ if (reserver.reserve()) {
1679+ throw JSONRPCError(RPC_INVALID_PARAMETER, "No scan in progress");
1680+ }
1681+ g_should_abourt_scan = true;
1682+ }
else if () { here.
1598@@ -1597,6 +1599,236 @@ UniValue savemempool(const JSONRPCRequest& request)
1599 return NullUniValue;
1600 }
1601
1602+static std::mutex g_utxosetscan;
1603+static std::atomic<int> g_scan_progress;
1604+static std::atomic<bool> g_scan_in_progress;
1605+static std::atomic<bool> g_should_abourt_scan;
1620+ m_could_reserve = true;
1621+ return true;
1622+ }
1623+
1624+ ~CoinsViewScanReserver() {
1625+ std::lock_guard<std::mutex> lock(g_utxosetscan);
Lock only when changing g_scan_in_progress?
0if (m_could_reserve) {
1 std::lock_guard<std::mutex> lock(g_utxosetscan);
2 g_scan_in_progress = false;
3}
1681+ g_should_abourt_scan = true;
1682+ }
1683+ else if (request.params[0].get_str() == "start") {
1684+ CoinsViewScanReserver reserver;
1685+ if (!reserver.reserve()) {
1686+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Scan already in progress, use action \"abort\" \"status\"");
or between actions.
1801+ bool res = pcoinsdbview->FindScriptPubKey(g_scan_progress, g_should_abourt_scan, count, needles, coins);
1802+ result.push_back(Pair("success", res ? "yes" : "no"));
1803+ result.push_back(Pair("searched_items", count));
1804+ }
1805+
1806+ for (auto& it : coins) {
1610+ bool m_could_reserve;
1611+public:
1612+ explicit CoinsViewScanReserver() : m_could_reserve(false) {}
1613+
1614+ bool reserve() {
1615+ std::lock_guard<std::mutex> lock(g_utxosetscan);
Assert not reserved?
0assert(!m_could_reserve);
1std::lock_guard<std::mutex> lock(g_utxosetscan);
2if (g_scan_in_progress) return false;
3g_scan_in_progress = true;
4m_could_reserve = true;
5return true;
1842+ g_scan_progress = 0;
1843+ int64_t count = 0;
1844+
1845+ // flush utxo state and start the scan
1846+ FlushStateToDisk();
1847+ bool res = pcoinsdbview->FindScriptPubKey(g_scan_progress, g_should_abort_scan, count, needles, coins);
I think due to the time required to perform a scan, it’s something that may be tolerated (although it should be mentioned in the docs). Not sure, but I guess it’s the same with gettxoutsetinfo.
Not sure if you can scan a CCoinsView of a snapshot state… I guess no.
Locking cs_main would be “meh”.
fundrawtx
@gmaxwell @jonasschnelli The cursor iterates over the state of the CCoinsView at the time it was created; modifying it during iteration is fine. This only works because GetCursor is not implemented for CCoinsViewCache, and is invoked directly on the CCoinsViewDB LevelDB wrapper.
The downside is that this requires a full flush of the database, hurting performance for all of the process (including validation).
Does it support mempool/unconfirmed utxos? I had a quick look and didn’t seem to, I think it would be useful to have mempool too.
Would it make sense to avoid the background job? maybe by keeping the utxo set sorted by scriptPubKey and binary search on it or perhaps some utxo set limited indexing?
Does it support mempool/unconfirmed utxos? I had a quick look and didn’t seem to, I think it would be useful to have mempool too.
It currently does not scan the mempool (hence the command name scantxoutset), but I agree, that would be useful. But, since scans take a while, timing may be a problem for scanning the mempool.
Maybe an additional RPC call would make sense (scanmempool)?
Would it make sense to avoid the background job? maybe by keeping the utxo set sorted by scriptPubKey and binary search on it or perhaps some utxo set limited indexing?
You mean speeding up the scan? I don’t think its worth to keep an extra index (changing the sort order would probably slow down verification a lot). My local tests did show a mainnet scan takes about ~30seconds (SSD, fast CPU). I don’t know what the space requirements for a by-scriptPubKey-index would be, …. maybe we can look into this once this PR did proceed.
coins.cpp
22@@ -19,6 +23,41 @@ bool CCoinsView::HaveCoin(const COutPoint &outpoint) const
23 return GetCoin(outpoint, coin);
24 }
25
26+bool CCoinsView::FindScriptPubKey(std::atomic<int>& scanProgress, std::atomic<bool>& shouldAbort, int64_t& count, CCoinsViewCursor& cursor, const std::set<CScript>& needles, std::map<COutPoint, Coin>& out_results) {
CCoinsView()::Cursor()), wouldn’t it then require to access pcoinsdbview from within coins.cpp which seems not ideal?
159@@ -157,6 +160,10 @@ class CCoinsView
160 //! Retrieve the block hash whose state this CCoinsView currently represents
161 virtual uint256 GetBestBlock() const;
162
163+ //! Search for a given set of pubkey scripts
164+ static bool FindScriptPubKey(std::atomic<int>& scanProgress, std::atomic<bool>& shouldAbort, int64_t& count, CCoinsViewCursor& cursor, const std::set<CScript>& needles, std::map<COutPoint, Coin>& out_results);
1790+ pubkeys.push_back(k.pubkey);
1791+ }
1792+ }
1793+ }
1794+
1795+ // add all common scripts for the given and derived pubkeys
It’s unfortunate that this introduces yet another “default set of addresses derived from a given key”. Maintaining this will lead to incompatibilities between implementations as new types of scripts get added.
It would be better to have an explicit way to describe a set of scripts to watch for that could be reused in multiple places. I’m working towards that (see https://gist.github.com/sipa/125cfa1615946d0c3f3eec2ad7f250a2), but it may be a while before it’s usable.
However, would you consider changing the RPC interface to be more easily adaptable to such a design? I think an interface which just takes a list of descriptions, each of which is a JSON object with either an address, script, or xpub (where the xpub one also takes a window size in a separate field). That means we can later add extra fields to describe what derivations to use, for example.
GitHub doesn’t seem to let me add more comments on specific line numbers, so I’ll put it here:
CPubKey pub_key should be const, and styled like a constant (all uppercase).
Overall, I’m unsure about this. This is functionality that is more easily provided by software that maintains a UTXO index by script, and is not possible in general if we’d move to a design like UHF (see mailinglist) or other UTXO avoidance techniques. Those are far away of course, and features like this can be made optional (like txindex is) if needed. I’m just generally unconvinced a full node is the best place to put this.
Overall, I’m unsure about this. This is functionality that is more easily provided by software that maintains a UTXO index by script, […]
I think the scantxoutset approach is a low hanging fruit (relatively simple to implement) and may be replaced later with an (optional) UXTO index approach.
It will immediately allow pruned peers to sweep seeds/wallets with a process that takes less then a minute. Also, pruned & non pruned peers could import a wallet without a rescan (if just interested in the unspent outputs rather then the whole transaction history).
Additionally, it would allow to safely create a raw-sweep-transaction that can be later used to sign via a cold-store instance/HWW.
I think it worth to finish the implementation because it fulfils direct user needs, even if we replace it later with an index based approach or even if we have to drop it completely once we move to a non-UTXO-set-based validation approach (far in future probably).
1717+ }
1718+ result.pushKV("progress", g_scan_progress);
1719+ } else if (request.params[0].get_str() == "abort") {
1720+ CoinsViewScanReserver reserver;
1721+ if (reserver.reserve()) {
1722+ return false;
Since this return value is subject to several races [1], I believe you could just return NullUniValue;
[1]:
1713+ if (request.params[0].get_str() == "status") {
1714+ CoinsViewScanReserver reserver;
1715+ if (reserver.reserve()) {
1716+ throw JSONRPCError(RPC_INVALID_PARAMETER, "No scan in progress");
1717+ }
1718+ result.pushKV("progress", g_scan_progress);
return result (early return) to clarify that nothing else happens in the hundred lines below for this case.
1835+ CTxDestination dest = DecodeDestination(address_uni.get_str());
1836+ if (!IsValidDestination(dest)) {
1837+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
1838+ }
1839+ CScript script = GetScriptForDestination(dest);
1840+ if (!script.empty()) {
assert(!script.empty())?
7+from test_framework.util import *
8+
9+import shutil
10+import os
11+
12+class ScanrxoutsetTest(BitcoinTestFramework):
15+ self.setup_clean_chain = True
16+ def run_test(self):
17+ self.log.info("Mining blocks...")
18+ self.nodes[0].generate(110)
19+
20+ addr_P2SH_SEGWIT = self.nodes[0].getnewaddress()
p2sh-segwit, so the tests don’t break when the defaults change.
28+ self.nodes[0].sendtoaddress(addr_BECH32, 3)
29+ self.nodes[0].generate(1)
30+
31+ self.log.info("Stop node, remove wallet, mine again some blocks...")
32+ self.stop_node(0)
33+ shutil.rmtree(os.path.join(self.options.tmpdir, "node0/regtest/wallets"))
os.path.join(self.nodes[0].datadir, "regtest", 'wallets')
49+ self.nodes[0].sendtoaddress(k_bip44_child3_ard, 0.4)
50+ self.nodes[0].sendtoaddress(k_bip44_child4_ard, 0.5)
51+ self.nodes[0].generate(1)
52+
53+ self.stop_node(0)
54+ os.remove(os.path.join(self.options.tmpdir, "node0/regtest/wallet.dat"))
os.path.join(self.nodes[1].datadir, "regtest", 'wallet.dat'
1728@@ -1704,6 +1729,9 @@ UniValue scantxoutset(const JSONRPCRequest& request)
1729 " }\n"
1730 " ,...], \n"
1731 " \"total_amount\" : x.xxx, (numeric) The total amount of all found unspent outputs in " + CURRENCY_UNIT + "\n"
1732+ " \"rawsweep_tx\" : \"value\", (string) The hex-encoded raw transaction of the optional sweep transaction\n"
1733+ " \"rawsweep_vsize\" : \"value\", (numeric) virtual transaction size of the sweep transaction including signatures\n"
1734+ " \"rawsweep_fee\" : \"value\", (numeric) Estimated fee for the sweep transaction in " + CURRENCY_UNIT + "\n"
1673@@ -1671,6 +1674,23 @@ class CoinsViewScanReserver
1674 }
1675 };
1676
1677+/** A dummy keystore for the txout-set scan in order to calculate the right fees for the sweep transaction */
1678+static CPubKey pub_key(std::vector<unsigned char>(33)); // always use a compress pubkey
1679+class CCoinsViewScanDummySignKeyStore : public CBasicKeyStore
1680+{
1681+public:
1682+ bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const {
1682+ bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const {
1683+ // return dummy pubkey
1684+ vchPubKeyOut = pub_key;
1685+ return true;
1686+ }
1687+ bool GetCScript(const CScriptID &hash, CScript& redeemScriptOut) const {
1728@@ -1704,6 +1729,9 @@ UniValue scantxoutset(const JSONRPCRequest& request)
1729 " }\n"
1730 " ,...], \n"
1731 " \"total_amount\" : x.xxx, (numeric) The total amount of all found unspent outputs in " + CURRENCY_UNIT + "\n"
1732+ " \"rawsweep_tx\" : \"value\", (string) The hex-encoded raw transaction of the optional sweep transaction\n"
1733+ " \"rawsweep_vsize\" : \"value\", (numeric) virtual transaction size of the sweep transaction including signatures\n"
1711@@ -1692,6 +1712,11 @@ UniValue scantxoutset(const JSONRPCRequest& request)
1712 " \"lookupwindow\": [<startindex>, <stopindex>] (An array with two integers that does define the range of keys that will be deriven for the given xpubs, default is 0 to 1000)\n"
1713 " }\n"
1714 " ]\n"
1715+ " \"rawsweep\": {\n (object, optional) Optionally creates a raw sweep transaction\n"
1716+ " \"address\": \"address\", (string, optional) Address where the funds should be sent to\n"
1717+ " \"feerate\": n, (numeric, optional, default not set: makes wallet determine the fee) Set a specific fee rate in " + CURRENCY_UNIT + "/kB\n"
conf_target instead?
1711@@ -1692,6 +1712,11 @@ UniValue scantxoutset(const JSONRPCRequest& request)
1712 " \"lookupwindow\": [<startindex>, <stopindex>] (An array with two integers that does define the range of keys that will be deriven for the given xpubs, default is 0 to 1000)\n"
1713 " }\n"
1714 " ]\n"
1715+ " \"rawsweep\": {\n (object, optional) Optionally creates a raw sweep transaction\n"
1716+ " \"address\": \"address\", (string, optional) Address where the funds should be sent to\n"
1717+ " \"feerate\": n, (numeric, optional, default not set: makes wallet determine the fee) Set a specific fee rate in " + CURRENCY_UNIT + "/kB\n"
1718+ " \"conf_target\": n, (numeric, optional) Confirmation target (in blocks), has no effect if feerate is provided\n"
utACK 94c727074b7aaac71f1560cdfc5b7d2a427d373f
Left inline comments on some of the commits.
1693+
1694+UniValue scantxoutset(const JSONRPCRequest& request)
1695+{
1696+ if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
1697+ throw std::runtime_error(
1698+ "scantxoutset <action> {\"pubkeys\": [\"pubkey\",...], \"xpubs\":[{\"xpub\": \"<xpub>\"}]}\n"
Maybe change this to
0 "scantxoutset \"action\" ( option )\n"
1710+ " {\n"
1711+ " \"xpub\":\"<xpub>\", (base58check encoded extended public key (xpub)\n"
1712+ " \"lookupwindow\": [<startindex>, <stopindex>] (An array with two integers that does define the range of keys that will be deriven for the given xpubs, default is 0 to 1000)\n"
1713+ " }\n"
1714+ " ]\n"
1715+ " \"rawsweep\": {\n (object, optional) Optionally creates a raw sweep transaction\n"
\n.
1707+ " \"addresses\":[\"address\",...] (array of strings, optional) An array of bitcoin addresses\n"
1708+ " \"xpubs\": (array of xpub objects that will be used to derive child keys with the given lookup window after m/0/k and m/1/k scheme)\n"
1709+ " [\n"
1710+ " {\n"
1711+ " \"xpub\":\"<xpub>\", (base58check encoded extended public key (xpub)\n"
1712+ " \"lookupwindow\": [<startindex>, <stopindex>] (An array with two integers that does define the range of keys that will be deriven for the given xpubs, default is 0 to 1000)\n"
1730+ " ,...], \n"
1731+ " \"total_amount\" : x.xxx, (numeric) The total amount of all found unspent outputs in " + CURRENCY_UNIT + "\n"
1732+ " \"rawsweep_tx\" : \"value\", (string) The hex-encoded raw transaction of the optional sweep transaction\n"
1733+ " \"rawsweep_vsize\" : \"value\", (numeric) virtual transaction size of the sweep transaction including signatures\n"
1734+ " \"rawsweep_fee\" : \"value\", (numeric) Estimated fee for the sweep transaction in " + CURRENCY_UNIT + "\n"
1735+ "]\n"
] with }.
Commit “Commit “Blockchain/RPC: Add scantxoutset method to scan UTXO set”
The response is an object, not an array.
1727+ " \"amount\" : x.xxx, (numeric) The total amount in " + CURRENCY_UNIT + " received by the address\n"
1728+ " \"height\" : n, (numeric) Height of the unspent transaction output\n"
1729+ " }\n"
1730+ " ,...], \n"
1731+ " \"total_amount\" : x.xxx, (numeric) The total amount of all found unspent outputs in " + CURRENCY_UNIT + "\n"
1732+ " \"rawsweep_tx\" : \"value\", (string) The hex-encoded raw transaction of the optional sweep transaction\n"
"rawsweep": { }?
1728+ " \"height\" : n, (numeric) Height of the unspent transaction output\n"
1729+ " }\n"
1730+ " ,...], \n"
1731+ " \"total_amount\" : x.xxx, (numeric) The total amount of all found unspent outputs in " + CURRENCY_UNIT + "\n"
1732+ " \"rawsweep_tx\" : \"value\", (string) The hex-encoded raw transaction of the optional sweep transaction\n"
1733+ " \"rawsweep_vsize\" : \"value\", (numeric) virtual transaction size of the sweep transaction including signatures\n"
1729+ " }\n"
1730+ " ,...], \n"
1731+ " \"total_amount\" : x.xxx, (numeric) The total amount of all found unspent outputs in " + CURRENCY_UNIT + "\n"
1732+ " \"rawsweep_tx\" : \"value\", (string) The hex-encoded raw transaction of the optional sweep transaction\n"
1733+ " \"rawsweep_vsize\" : \"value\", (numeric) virtual transaction size of the sweep transaction including signatures\n"
1734+ " \"rawsweep_fee\" : \"value\", (numeric) Estimated fee for the sweep transaction in " + CURRENCY_UNIT + "\n"
2090+ CScript script = GetScriptForDestination(address);
2091+ if (!script.empty()) {
2092+ needles.insert(script);
2093+ temp_keystore.AddWatchOnly(script);
2094+ }
2095+ // add P2SH-P2WPKH script
51+ }
52+ scan_progress = 100;
53+ return true;
54+}
55+
56+bool CCoinsView::FindScriptPubKey(std::atomic<int>& scan_progress, std::atomic<bool>& should_abort, int64_t& searchItems, const std::set<CScript>& needles, std::map<COutPoint, Coin>& out_results) {
searchItems should be search_items to match the .h file.
1977+ " \"xpubs\": (array of xpub objects that will be used to derive child keys with the given lookup window after m/0/k and m/1/k scheme)\n"
1978+ " [\n"
1979+ " {\n"
1980+ " \"xpub\":\"<xpub>\", (base58check encoded extended public key (xpub)\n"
1981+ " \"lookupwindow\": [<startindex>, <stopindex>] (An array with two integers that does define the range of keys that will be deriven for the given xpubs, default is 0 to 1000)\n"
1982+ " }\n"
,... after this.
1989+ " \"vout\": n, (numeric) the vout value\n"
1990+ " \"scriptPubKey\" : \"script\", (string) the script key\n"
1991+ " \"amount\" : x.xxx, (numeric) The total amount in " + CURRENCY_UNIT + " received by the address\n"
1992+ " \"height\" : n, (numeric) Height of the unspent transaction output\n"
1993+ " }\n"
1994+ " ,...], \n"
Nit, remove fee handling? The caller could do:
0tx = scantxoutset("start", { "sweep_to": address })["sweep_tx"]
1fundrawtransaction(tx, { "subtractFeeFromOutputs": [0] })
2...
So scantxoutset should just add the unspents and one output.
tx = scantxoutset(“start”, { “sweep_to”: address })[“sweep_tx”] fundrawtransaction(tx, { “subtractFeeFromOutputs”: [0] }) @promag: would that also work without the wallet? My idea was to make
scantxoutsetwork without a wallet.
Just noting that GitHub decides to hide half of my review comments by default and they were not addressed (mostly feedback on the tests). @MarcoFalke: Have not forgotten. Still working on it.
Concept ACK
I tested against a blockchain.info testnet wallet and was able to get the correct balance for an account including change (there’s an xpub export feature buried in settings).
I also tested against a Ledger wallet with p2sh segwit addresses.
Also tested status and abort.
Background question: what index could be added to make this much faster? I’m guessing scriptPubKey -> unspents?
Issues:
src/qt/bitcoin-qt it says the RPC method doesn’t exist~I can’t figure out what’s wrong with this syntax (and it only throws the error after doing the scan bit):
0src/bitcoin-cli scantxoutset start '{"xpubs": [ {"xpub": "tpub..." } ]}, "rawsweep": {}}'
1error: Error parsing JSON:{"xpubs": [ {"xpub": "tpub..." } ]}, "rawsweep": {}}
When I do add an address I get Min relay fee not met (code -26), even if I set "conf_target": 6}, setting conf_target to 1 did help.
I’m assuming that calling rawsweep without the optional address field makes it use the wallet? (if so, that should be in the help)
Suggestions:
scantxoutset to the PR descriptionSuggestions for followup PR:
total_amount doesn’t include unconfirmed funds, maybe add a flag to show those?xpriv (with or without importing keys)rawsweep should have an option to make 1 transaction per output
Followed @sipa advice and changed the API. The script type derivation is now controllable via the API.
The command takes now scan objects that are either an address, script, pubkey or xpub.
Xpub and pubkey have the option to pass in the script type (P2PKH, P2SH-P2WPKH, P2WPKH).
The commit history is also more clean now.
Thanks for reviewing again.
@MarcoFalke: your review is lost in the nirvana.. can’t find the comments on the test anymore. Can you try to review again?
@Sjors: the Min relay fee not met (code -26) is due to non initialise fee estimation. Use a feerate if you test on regtest without fee estimation available.
0"scanobjects" (array, required) Array of scan objects (only one object type per scan object allowed)
1 [
2 { "address" : "<address>" }, (string, optional) Bitcoin address
3 { "script" : "<scriptPubKey>" }, (string, optional) HEX encoded script (scriptPubKey)
4 { "pubkey" : (object, optional) Public key
5 {
6 "pubkey" : "<pubkey">, (string, required) HEX encoded public key
7 "script_types" : [ ... ], (array, optional) Array of script-types to derive from the pubkey (possible values: "P2PKH", "P2SH-P2WPKH", "P2WPKH")
8 }
9 },
10 { "xpub" : (object, optional) Use an extended public key child key range (m/0/k & m/1/k) to derive scripts from
11 {
12 "xpub" : "<xpub">, (string, required) Base58check encoded extended public key (xpub)
13 "range" : [ <s>, <e> ], (array, optional) Range of keys that will be deriven from the given xpubs (default is 0 to 1000)
14 "script_types" : [ ... ], (array, optional) Array of derivation type (possible values: "P2PKH", "P2SH-P2WPKH", "P2WPKH")
15 }
16 },
17 ]
total_amount doesn’t include unconfirmed funds, maybe add a flag to show those?
This should be done in a separate command (scanmempool which would be a great follow up).
a minimum block height / date for faster scans (if only to make RPC dev lives happier)
Not possible AFAIK. We scan the UXTO set, not blocks.
it would be really cool to sweep using an xpriv (with or without importing keys)
IMO the command should not include any private keys. Ideally we add xpriv support similar to this command to signrawtransaction. But I agree that this would be a great.
rawsweep should have an option to make 1 transaction per output
Maybe, but could/should be added later.
I’m assuming that calling rawsweep without the optional address field makes it use the wallet? (if so, that should be in the help)
No. This command is totally de-coupled from the wallet (runs without wallet).
2047+ " }\n"
2048+ " },\n"
2049+ " ]\n"
2050+ "3. \"options\" (object, optional)\n"
2051+ " \"rawsweep\": { (object, optional) Optionally creates a raw sweep transaction\n"
2052+ " \"address\": \"address\", (string, optional) Address where the funds should be sent to\n"
@jonasschnelli the Min relay fee not met error was on testnet, not regtest. Maybe that doesn’t happen on mainnet much. Maybe it should detect the lack of fee estimation and throw a specific error (if no fee is specified by the user)?
We scan the UXTO set, not blocks
I wish there an easy way to “translate” Bitcoin Core’s internal data structures to SQL table descriptions, so it’s easier to understand what “columns” and “indexes” exist. I haven’t had much luck using external tools to load the various db files.
I’m assuming that calling rawsweep without the optional address field makes it use the wallet? (if so, that should be in the help) No. This command is totally de-coupled from the wallet (runs without wallet).
Ok, then I don’t understand why the address field is documented as optional. @promag 1 transaction per output (to a unique address) improves privacy when sweeping an existing wallet. Especially if you don’t broadcast them all at the same time.
I have an unspent txout (on testnet) like this:
0 {
1 "txid": "473abbc4eb4768d1fdad21f05d3485bcd96cb33f883dce3dd8bccebced3f4efc",
2 "vout": 0,
3 "address": "n2PaqLena7QZtpKowZok8FMPqCY21Xrp4x",
4 "label": "null",
5 "scriptPubKey": "21029b861186b49793708a4e00ecf3cec62c027dccfc12509b7e6ac2538b4b19b3abac",
6 "amount": 25.00317000,
7 "confirmations": 980258,
8 "spendable": true,
9 "solvable": true,
10 "safe": true
11 }
By address gives me no output:
0$ src/bitcoin-cli -testnet scantxoutset start '[{"address":"n2PaqLena7QZtpKowZok8FMPqCY21Xrp4x"}]'
1{
2 "success": "yes",
3 "searched_items": 19143564,
4 "unspents": [
5 ],
6 "total_amount": 0.00000000
7}
8$
By script it works, however
0$ src/bitcoin-cli -testnet scantxoutset start '[{"script":"21029b861186b49793708a4e00ecf3cec62c027dccfc12509b7e6ac2538b4b19b3abac"}]'
1{
2 "success": "yes",
3 "searched_items": 19162149,
4 "unspents": [
5 {
6 "txid": "473abbc4eb4768d1fdad21f05d3485bcd96cb33f883dce3dd8bccebced3f4efc",
7 "vout": 0,
8 "scriptPubKey": "21029b861186b49793708a4e00ecf3cec62c027dccfc12509b7e6ac2538b4b19b3abac",
9 "amount": 25.00317000,
10 "height": 330282
11 }
12 ],
13 "total_amount": 25.00317000
14}
@laanwj:
If I execute getaddressinfo n2PaqLena7QZtpKowZok8FMPqCY21Xrp4x I get
0{
1 "address": "n2PaqLena7QZtpKowZok8FMPqCY21Xrp4x",
2 "scriptPubKey": "76a914e4f5d0b9353f7715e1bfed11358a5b84fd7c4e3488ac",
3 "ismine": false,
4 "iswatchonly": false,
5 "isscript": false,
6 "iswitness": false,
7 "labels": [
8 ]
9}
AFAIK your script (P2PK) 21029b861186b49793708a4e00ecf3cec62c027dccfc12509b7e6ac2538b4b19b3abac can only be derived if one has the according pubkey.
Added two commits:
https://github.com/bitcoin/bitcoin/pull/12196/commits/fdd09d34a14ca4f416b9101b37187098f9145301
adds support for P2PK in script_types
https://github.com/bitcoin/bitcoin/pull/12196/commits/3c546fd38e2bdb6957fa134f8847e2a7c5c7e23
P2PK and P2PKH share the same address (two scripts result in the same address). Deriving a P2PK script requires the pubkey which is not available when the user provides an address.
Since we not want to miss funds, we have to derive the P2PKH equivalent of every P2PK unspent in the txoutset and compare it against the provided address (only required when providing addresses).
159@@ -157,6 +160,10 @@ class CCoinsView
160 //! Retrieve the block hash whose state this CCoinsView currently represents
161 virtual uint256 GetBestBlock() const;
162
163+ //! Search for a given set of pubkey scripts
164+ static bool FindScriptPubKey(std::atomic<int>& scan_progress, std::atomic<bool>& should_abort, int64_t& count, CCoinsViewCursor& cursor, std::function<bool(const CScript& script)> match_func, std::map<COutPoint, Coin>& out_results);
165+ bool FindScriptPubKey(std::atomic<int>& scan_progress, std::atomic<bool>& should_abort, int64_t& search_items, std::function<bool(const CScript& script)> match_func, std::map<COutPoint, Coin>& out_results);
std::function is defined in header <functional>, which needs to be included.
22@@ -19,6 +23,41 @@ bool CCoinsView::HaveCoin(const COutPoint &outpoint) const
23 return GetCoin(outpoint, coin);
24 }
25
26+bool CCoinsView::FindScriptPubKey(std::atomic<int>& scan_progress, std::atomic<bool>& should_abort, int64_t& count, CCoinsViewCursor& cursor, const std::set<CScript>& needles, std::map<COutPoint, Coin>& out_results) {
27+ scan_progress = 0;
count = 0;?
22@@ -19,6 +23,41 @@ bool CCoinsView::HaveCoin(const COutPoint &outpoint) const
23 return GetCoin(outpoint, coin);
24 }
25
26+bool CCoinsView::FindScriptPubKey(std::atomic<int>& scan_progress, std::atomic<bool>& should_abort, int64_t& count, CCoinsViewCursor& cursor, const std::set<CScript>& needles, std::map<COutPoint, Coin>& out_results) {
27+ scan_progress = 0;
28+ while (cursor.Valid()) {
29+ COutPoint key;
30+ Coin coin;
31+ if (cursor.GetKey(key) && cursor.GetValue(coin)) {
Nit,
0if (!cursor.GetKey(key) || !cursor.GetValue(coin) return false;
and reindent below.
22@@ -19,6 +23,41 @@ bool CCoinsView::HaveCoin(const COutPoint &outpoint) const
23 return GetCoin(outpoint, coin);
24 }
25
26+bool CCoinsView::FindScriptPubKey(std::atomic<int>& scan_progress, std::atomic<bool>& should_abort, int64_t& count, CCoinsViewCursor& cursor, const std::set<CScript>& needles, std::map<COutPoint, Coin>& out_results) {
const std::atomic<bool>& should_abort.
In commit “Add CCoinsView::FindScriptPubKey to search the UTXO set”.
There is no need for this function to burden the CCoinsView interface I think, as it only uses publicly available data from that class. The implementation can just move to where scantxoutset is defined.
29+ COutPoint key;
30+ Coin coin;
31+ if (cursor.GetKey(key) && cursor.GetValue(coin)) {
32+ if (count++ % 8192 == 0) {
33+ boost::this_thread::interruption_point();
34+ if (should_abort) {
|| ShutdownRequested()?
2011+
2012+UniValue scantxoutset(const JSONRPCRequest& request)
2013+{
2014+ if (request.fHelp || request.params.size() < 1 || request.params.size() > 3)
2015+ throw std::runtime_error(
2016+ "scantxoutset <action> <scanobjects> (<options>)\n"
( )
48+ self.nodes[0].sendtoaddress(k_bip44_child2_ard, 0.3)
49+ self.nodes[0].sendtoaddress(k_bip44_child3_ard, 0.4)
50+ self.nodes[0].sendtoaddress(k_bip44_child4_ard, 0.5)
51+ self.nodes[0].generate(1)
52+
53+ self.stop_node(0)
0self.restart_node(0, ['-nowallet'])
and remove next 2 lines.
54+ os.remove(os.path.join(self.nodes[0].datadir, "regtest", "wallet.dat"))
55+ self.start_node(0)
56+ self.log.info("Test if we have found the non HD unspent outputs.")
57+ assert_equal(self.nodes[0].scantxoutset("start", [ {"pubkey": {"pubkey": pubk1}}, {"pubkey": {"pubkey": pubk2}}, {"pubkey": {"pubkey": pubk3}}])['total_amount'], 6)
58+ decodedsweeptx = self.nodes[0].decoderawtransaction(self.nodes[0].scantxoutset("start", [ {"pubkey": {"pubkey": pubk1}}, {"pubkey": {"pubkey": pubk2}}, {"pubkey": {"pubkey": pubk3}}], {"rawsweep" : {"address": addr_BECH32, "feerate": 0.00025000}})['rawsweep_tx'])
59+ assert_equal(len(decodedsweeptx['vin']), 3)
Could improve coverage:
0 result = self.nodes[1].scantxoutset("start", [ {"pubkey": {"pubkey": pubk1}}, {"pubkey": {"pubkey": pubk2}}, {"pubkey": {"pubkey": pubk3}}], {"rawsweep" : {"address": addr_BECH32, "feerate": 0.00025000}})
1 decodedsweeptx = self.nodes[1].decoderawtransaction(result['rawsweep_tx'])
2 assert_equal(len(decodedsweeptx['vout']), 1)
3 assert_equal(len(decodedsweeptx['vin']), 3)
4 assert_equal(decodedsweeptx['vout'][0]['scriptPubKey']['addresses'], [addr_BECH32])
2345+ }
2346+
2347+ result.push_back(Pair("unspents", unspents));
2348+ result.push_back(Pair("total_amount", ValueFromAmount(total_in)));
2349+ } else {
2350+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid command");
RPC_INVALID_PARAMETER instead?
2316+ }
2317+
2318+ // look for user provided feerate
2319+ UniValue feerate_uni = find_value(rawsweep_uni, "feerate");
2320+ CFeeRate feerate;
2321+ if(feerate_uni.isNum()) {
if.
2013+{
2014+ if (request.fHelp || request.params.size() < 1 || request.params.size() > 3)
2015+ throw std::runtime_error(
2016+ "scantxoutset <action> <scanobjects> ( <options> )\n"
2017+ "\nScans the unspent transaction output set for possible entries that matches common scripts of given public keys.\n"
2018+ "Using addresses as scanobjects will _not_ detect unspent P2PK txouts\n"
2050+ " \"unspents\": [\n"
2051+ " {\n"
2052+ " \"txid\" : \"transactionid\", (string) The transaction id\n"
2053+ " \"vout\": n, (numeric) the vout value\n"
2054+ " \"scriptPubKey\" : \"script\", (string) the script key\n"
2055+ " \"amount\" : x.xxx, (numeric) The total amount in " + CURRENCY_UNIT + " received by the address\n"
It seems like that the scans based on addresses are useful. Other developers opinions would be welcome.
Yes, that is useful, and for better or worse, likely how most people will be using this in practice.
utACK 4782d23938a8a5297319f024aed3fc6e2c1651ac
Addresses are opaque identifiers for a given invoice. That they are currently implemented by encoding a scriptPubKey is irrelevant.
This makes no sense to me: per the wiki “A Bitcoin address […] is an identifier […] that represents a possible destination for a bitcoin payment”, but if you want to send a bitcoin payment somewhere you have to be able to deduce the scriptPubKey from the address, or you can’t make a transaction. I don’t think it makes sense to generalise “bitcoin addresses” to things that you can’t generate a scriptPubKey from (eg, bech32 encoded lightning invoices, or some new address format you don’t yet understand).
Finding UTXOs by scriptPubKey seems like it makes sense, and per the above finding the scriptPubKeys from an address should also always be straightforward. So ConceptACK fwiw, unless I’m missing something major here…
0@@ -0,0 +1,76 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2018 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test the scantxoutset rpc call."""
6+from test_framework.test_framework import BitcoinTestFramework
7+from test_framework.util import *
2101+ UniValue xpub_uni = find_value(scanobject, "xpub");
2102+
2103+ // make sure only one object type is present
2104+ if (1 != !address_uni.isNull() + !pubkey_uni.isNull() + !script_uni.isNull() + !xpub_uni.isNull()) {
2105+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Only one object type is allowed per scan object");
2106+ }
nit, } else if (...) {
There are a couple of more cases throughout.
2286+
2287+ input_txos.push_back(txo);
2288+ total_in += txo.nValue;
2289+
2290+ UniValue unspent(UniValue::VOBJ);
2291+ unspent.push_back(Pair("txid", outpoint.hash.GetHex()));
pushKV instead, push_back(Pair(...)) occurrences were replaced recently.
2280+
2281+ // Fill in dummy signatures for fee calculation, ignore signature verification.
2282+ const CScript& scriptPubKey = txo.scriptPubKey;
2283+ SignatureData sigdata;
2284+ ProduceSignature(temp_keystore, DUMMY_SIGNATURE_CREATOR, scriptPubKey, sigdata);
2285+ UpdateInput(tx.vin.at(nIn), sigdata);
nIn and use tx.vin.back() instead (like above).
Some nits and comments. Tests LGTM. Could squash last commit.
utACK.
28+ count = 0;
29+ while (cursor.Valid()) {
30+ COutPoint key;
31+ Coin coin;
32+ if (!cursor.GetKey(key) || !cursor.GetValue(coin)) return false;
33+ if (count++ % 8192 == 0) {
++count
1934+ bool m_could_reserve;
1935+public:
1936+ explicit CoinsViewScanReserver() : m_could_reserve(false) {}
1937+
1938+ bool reserve() {
1939+ assert (!m_could_reserve);
include <assert.h>
nit: whitespace could benefit from clang-format
Commit “Blockchain/RPC: Add scantxoutset method to scan UTXO set”
Missing fix :trollface:
1968+{
1969+ if (outputtype == "P2PK") return OutputScriptType::P2PK;
1970+ else if (outputtype == "P2PKH") return OutputScriptType::P2PKH;
1971+ else if (outputtype == "P2SH_P2WPKH") return OutputScriptType::P2SH_P2WPKH;
1972+ else if (outputtype == "P2WPKH") return OutputScriptType::P2WPKH;
1973+ else return OutputScriptType::UNKNOWN;
case statement. :P
2113+ else if (!xpub_uni.isNull() && !xpub_uni.isObject()) {
2114+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Scanobject \"xpub\" must contain an object as value");
2115+ }
2116+ else if (!script_uni.isNull() && !script_uni.isStr()) {
2117+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Scanobject \"xpub\" must contain a single string as value");
2118+ }
RPCTypeCheckObj for some of this here and elsewhere. E.g. https://github.com/bitcoin/bitcoin/blob/686e97a0c7358291d628213447cf33e99cde7ce8/src/rpc/rawtransaction.cpp#L786-L791
2152+ if (script_type == OutputScriptType::UNKNOWN) throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid script type");
2153+ CScript script;
2154+ if (script_type == OutputScriptType::P2PK) {
2155+ // support legacy P2PK scripts
2156+ script << ToByteVector(pubkey) << OP_CHECKSIG;
2157+ }
} else {
(some other instances too)
While testing I hit an edge case:
0./src/bitcoin-cli -regtest scantxoutset start '[]' & ; ./src/bitcoin-cli -regtest scantxoutset abort
1[1] 90388
2true
3{
4 "success": "yes",
5 "searched_items": 6,
6 "unspents": [
7 ],
8 "total_amount": 0.00000000
9}
10[1] + 90388 done ./src/bitcoin-cli -regtest scantxoutset start '[]'
So abort returned true but the scan finished successfully.
Not sure but if the use case is to call abort after status — because it’s taking too long — so maybe abort could just return null:
0if (request.params[0].get_str() == "abort") {
1 g_should_abort_scan = true;
2 return NullUniValue;
3}
43- uint32_t high = 0x100 * *key.hash.begin() + *(key.hash.begin() + 1);
44- scan_progress = (int)(high * 100.0 / 65536.0 + 0.5);
45- }
46- if (needles.count(coin.out.scriptPubKey)) {
47- out_results.emplace(key, coin);
48+ if (!cursor.GetKey(key) || !cursor.GetValue(coin)) return false;
In commit “Blockchain/RPC: Add scantxoutset method to scan UTXO set”.
The implementation of “CCoinsView::FindScriptPubKey” is being changed significantly in this commit. Can you move the changes to the previous commit that introduces the function?
2002+ " \"abort\" for aborting the current scan (returns true when abort was successful)\n"
2003+ " \"status\" for progress report (in %) of the current scan\n"
2004+ "2. \"scanobjects\" (array, required) Array of scan objects (only one object type per scan object allowed)\n"
2005+ " [\n"
2006+ " { \"address\" : \"<address>\" }, (string, optional) Bitcoin address\n"
2007+ " { \"pubkey\" : (object, optional) Public key\n"
I would leave this out as well, as it’ll be handled equally by the descriptor based approach. It’s a lot of complexity to maintain which will be handled more generically later.
Also, I don’t think it’s very useful and add - if you know the pubkey and derivation type you generally also know the address already.
2031@@ -2012,6 +2032,12 @@ UniValue scantxoutset(const JSONRPCRequest& request)
2032 " }\n"
2033 " },\n"
2034 " ]\n"
2035+ "3. \"options\" (object, optional)\n"
2036+ " \"rawsweep\": { (object, optional) Optionally creates a raw sweep transaction\n"
In commit “scantxoutset: Add optional raw sweep transaction”.
Is this the best place to implement this? As a separate wallet RPC it could work much more correctly (as it would know the actual scripts involved and be able to give a vsize estimate that’s more than a dumb guess), and be more useful too by being combinable with whatever way to find inputs.
I feel this makes the RPC look like a kitchen sink.
From #12196 (comment)
tx = scantxoutset(“start”, { “sweep_to”: address })[“sweep_tx”] fundrawtransaction(tx, { “subtractFeeFromOutputs”: [0] })
@promag: would that also work without the wallet? My idea was to make
scantxoutsetwork without a wallet. @sipa not sure if you are suggesting something similar to:
0unspents = scantxoutset "start" ...
1tx = createrawtransaction unspents ...
2tx = fundrawtransaction ...
And how could it work without the wallet?
@promag, I just mean scantxoutset + createrawtransaction. Then a separate wallet RPC like “walletestimatesignedrawtransactionsize” (shorter names welcome…) can be used to guess the size of the tx ahead of time. “fundrawtransaction” already does this estimation internally.
Would that also work without the wallet?
No, that’s the point. You cannot implement that without knowledge of the scripts involved.
It may be better to integrate this into PSBT, as it means there doesn’t need to a single wallet that knows about all scripts in a transaction.
FindScriptPubKey() method to rpc/blockchain.cpp (changes are no longer spread in multiple commits).I kept the pubkey search option since I think this is very helpful. I agree that we should move to the descriptors specified by @sipa. IMO we define that we wanted to have xpub/pub support (according those descriptors) in the same release to avoid API breaks and @laanwj agreed to let this in after the code freeze in form of a bugfix. Additionally, there is always the option to mark this RPC command “experimental” (API changes possible).
6@@ -7,6 +7,7 @@
7 #include <consensus/consensus.h>
8 #include <random.h>
9
10+
Commit “Add FindScriptPubKey() to search the UTXO set”
nit, unrelated change.
1915@@ -1916,6 +1916,35 @@ static UniValue savemempool(const JSONRPCRequest& request)
1916 return NullUniValue;
1917 }
1918
1919+//! Search for a given set of pubkey scripts
1920+bool FindScriptPubKey(std::atomic<int>& scan_progress, const std::atomic<bool>& should_abort, int64_t& count, CCoinsViewCursor *cursor, const std::set<CScript>& needles, std::map<COutPoint, Coin>& out_results) {
Commit “Add FindScriptPubKey() to search the UTXO set”
nit, CCoinsViewCursor* cursor.
2178+
2179+ std::unique_ptr<CCoinsViewCursor> pcursor(pcoinsdbview->Cursor());
2180+ assert(pcursor);
2181+ bool res = FindScriptPubKey(g_scan_progress, g_should_abort_scan, count, pcursor.get(), needles, coins);
2182+
2183+ result.push_back(Pair("success", res ? "yes" : "no"));
2006+ case OutputScriptType::P2PKH: return key.GetID();
2007+ case OutputScriptType::P2SH_P2WPKH:
2008+ case OutputScriptType::P2WPKH: {
2009+ if (!key.IsCompressed()) return key.GetID();
2010+ CTxDestination witdest = WitnessV0KeyHash(key.GetID());
2011+ CScript witprog = GetScriptForDestination(witdest);
Commit “Blockchain/RPC: Add scantxoutset method to scan UTXO set”
This could be in the block below:
0CTxDestination witdest = WitnessV0KeyHash(key.GetID());
1if (type == OutputScriptType::P2SH_P2WPKH) {
2 CScript witprog = GetScriptForDestination(witdest);
3 return CScriptID(witprog);
4} else {
5 ...
2021+
2022+UniValue scantxoutset(const JSONRPCRequest& request)
2023+{
2024+ if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
2025+ throw std::runtime_error(
2026+ "scantxoutset <action> <scanobjects> ( <options> )\n"
Commit “Blockchain/RPC: Add scantxoutset method to scan UTXO set”
nit, add ( ) to <scanobjects> because status action has no more arguments.
2157+ g_should_abort_scan = false;
2158+ g_scan_progress = 0;
2159+ int64_t count = 0;
2160+ FlushStateToDisk();
2161+
2162+ std::unique_ptr<CCoinsViewCursor> pcursor(pcoinsdbview->Cursor());
2077+ CoinsViewScanReserver reserver;
2078+ if (!reserver.reserve()) {
2079+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Scan already in progress, use action \"abort\" or \"status\"");
2080+ }
2081+ std::set<CScript> needles;
2082+ CBasicKeyStore temp_keystore;
Fixed issues:
cs_main during coins view cursor creation and FlushToDisk()temp_keystore (relict from raw-sweep-tx).1936+ }
1937+ }
1938+ if (count % 256 == 0) {
1939+ // update progress reference every 256 item
1940+ uint32_t high = 0x100 * *key.hash.begin() + *(key.hash.begin() + 1);
1941+ scan_progress = (int)(high * 100.0 / 65536.0 + 0.5);
Nit: code for this seems like it’s cropping up in a few different places, might be nice to have an abstraction for it.
0 $ git grep -C 1 "/ 65536" | cat
1
2src/index/txindex.cpp- (static_cast<uint32_t>(*(txid.begin() + 1)) << 0);
3src/index/txindex.cpp: int percentage_done = (int)(high_nibble * 100.0 / 65536.0 + 0.5);
4src/index/txindex.cpp-
5--
6src/txdb.cpp- uint32_t high = 0x100 * *key.second.begin() + *(key.second.begin() + 1);
7src/txdb.cpp: int percentageDone = (int)(high * 100.0 / 65536.0 + 0.5);
8src/txdb.cpp- uiInterface.ShowProgress(_("Upgrading UTXO database"), percentageDone, true);
2020+
2021+UniValue scantxoutset(const JSONRPCRequest& request)
2022+{
2023+ if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
2024+ throw std::runtime_error(
2025+ "scantxoutset <action> ( <scanobjects> ) ( <options> )\n"
2172+ {
2173+ LOCK(cs_main);
2174+ FlushStateToDisk();
2175+ std::unique_ptr<CCoinsViewCursor> pcursor(pcoinsdbview->Cursor());
2176+ assert(pcursor);
2177+ bool res = FindScriptPubKey(g_scan_progress, g_should_abort_scan, count, pcursor.get(), needles, coins);
cs_main after initializing pcursor? My impression is that CCoinsViewCursor can be used for read-only ops without holding it.
Concept ACK
Happy to do a more in-depth review. PR description looks in need of an update.
2175+ FlushStateToDisk();
2176+ pcursor = std::unique_ptr<CCoinsViewCursor>(pcoinsdbview->Cursor());
2177+ assert(pcursor);
2178+ }
2179+ bool res = FindScriptPubKey(g_scan_progress, g_should_abort_scan, count, pcursor.get(), needles, coins);
2180+ result.pushKV("success", res ? true : false);
res is already a boolean…
2140@@ -2141,8 +2141,13 @@ UniValue scantxoutset(const JSONRPCRequest& request)
2141 for (const UniValue& script_type_uni : script_types_uni.get_array().getValues()) {
2142 OutputScriptType script_type = GetOutputScriptTypeFromString(script_type_uni.get_str());
2143 if (script_type == OutputScriptType::UNKNOWN) throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid script type");
2144-
2145- CScript script = GetScriptForDestination(GetDestinationForKey(pubkey, script_type));
2146+ CScript script;
2147+ if (script_type == OutputScriptType::P2PK) {
2148+ // support legacy P2PK scripts
2149+ script << ToByteVector(pubkey) << OP_CHECKSIG;
GetScriptForRawPubKey.
2072+ if (reserver.reserve()) {
2073+ return false;
2074+ }
2075+ g_should_abort_scan = true;
2076+ return true;
2077+ } else if (request.params[0].get_str() == "start") {
2097+ } else if (!address_uni.isNull() && !address_uni.isStr()) {
2098+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Scanobject \"address\" must contain a single string as value");
2099+ } else if (!pubkey_uni.isNull() && !pubkey_uni.isObject()) {
2100+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Scanobject \"pubkey\" must contain an object as value");
2101+ } else if (!script_uni.isNull() && !script_uni.isStr()) {
2102+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Scanobject \"script\" must contain a single string as value");
You could remove most of the above with:
0RPCTypeCheckObj(scanobject,
1 {
2 {"address", UniValueType(UniValue::VSTR)},
3 {"pubkey", UniValueType(UniValue::VOBJ)},
4 {"script", UniValueType(UniValue::VSTR)},
5 }, true /* fAllowNull */);
2090+ UniValue address_uni = find_value(scanobject, "address");
2091+ UniValue pubkey_uni = find_value(scanobject, "pubkey");
2092+ UniValue script_uni = find_value(scanobject, "script");
2093+
2094+ // make sure only one object type is present
2095+ if (1 != !address_uni.isNull() + !pubkey_uni.isNull() + !script_uni.isNull()) {