New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add scantxoutset RPC method #12196

Merged
merged 6 commits into from Jul 17, 2018

Conversation

@jonasschnelli
Member

jonasschnelli commented Jan 16, 2018

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.

@gmaxwell

This comment has been minimized.

Member

gmaxwell commented Jan 16, 2018

Why is it pubkeys and not addresses for the pubkey part? (obviously xpubs are xpubs and need to be)

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Jan 16, 2018

After a short discussion on IRC (https://botbot.me/freenode/bitcoin-core-dev/2018-01-16/?msg=95804115&page=2) support for addresses and pubkeys makes most sense. Will add support for an array of addresses.

else if (request.params[0].get_str() == "abort") {
CoinsViewScanReserver reserver;
if (reserver.reserve()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "No scan in progress");

This comment has been minimized.

@promag

promag Jan 16, 2018

Member

IMO there is no need to throw, a bool in the response is enough? Otherwise, missing test.

{
if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
throw std::runtime_error(
"scantxoutset <action> {\"pubkeys\": [\"pubkey\",...], \"xpubs\":[{\"xpub\": \"<xpub>\"}], other options}\n"

This comment has been minimized.

@promag

promag Jan 16, 2018

Member

Remove other options? There are none.

UniValue scantxoutset(const JSONRPCRequest& request)
{
if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)

This comment has been minimized.

@promag

promag Jan 16, 2018

Member

request.params.size() != 2?

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 19, 2018

Member

Nah. You can also call scantxoutset status

This comment has been minimized.

@promag

promag Jul 12, 2018

Member

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)?

" \"pubkeys\":[\"pubkey\",...] (array of strings, optional) An array of HEX encoded public keys\n"
" \"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"
" [\n"
" {\"xpub\":\"<xpub>\", (base58check encoded extended public key (xpub)\n"

This comment has been minimized.

@promag

promag Jan 16, 2018

Member

Nit, newline after {.

if (request.params[0].get_str() == "status") {
CoinsViewScanReserver reserver;
if (reserver.reserve()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "No scan in progress");

This comment has been minimized.

@promag

promag Jan 16, 2018

Member

Missing test for error.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 19, 2018

Member

You mean missing a functional test for that case. Yes. I though about it, but would require a mockup-slowdown argument (-testslowdown or similar). Otherwise I guess it's hard to properly test this.

This comment has been minimized.

@MarcoFalke

MarcoFalke May 21, 2018

Member

Why would this be an exception, considering that the scan could normally finish between the last call and this one? Generally I think we should avoid exceptions for control flow in our code (and especially in third party code that uses the rpc interface)

This comment has been minimized.

@promag

promag May 22, 2018

Member

Yes, agree that status should not raise an error when no scan is in progress.

static std::mutex g_utxosetscan;
static std::atomic<int> g_scan_progress;
static std::atomic<bool> g_scan_in_progress;
static std::atomic<bool> g_should_abourt_scan;

This comment has been minimized.

@promag

promag Jan 16, 2018

Member

Typo, abort.

}
~CoinsViewScanReserver() {
std::lock_guard<std::mutex> lock(g_utxosetscan);

This comment has been minimized.

@promag

promag Jan 16, 2018

Member

Lock only when changing g_scan_in_progress?

if (m_could_reserve) {
    std::lock_guard<std::mutex> lock(g_utxosetscan);
    g_scan_in_progress = false;
}
else if (request.params[0].get_str() == "start") {
CoinsViewScanReserver reserver;
if (!reserver.reserve()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Scan already in progress, use action \"abort\" \"status\"");

This comment has been minimized.

@promag

promag Jan 16, 2018

Member

Remove "status"? Otherwise add or between actions.

result.push_back(Pair("searched_items", count));
}
for (auto& it : coins) {

This comment has been minimized.

@promag

promag Jan 16, 2018

Member

const auto&

explicit CoinsViewScanReserver() : m_could_reserve(false) {}
bool reserve() {
std::lock_guard<std::mutex> lock(g_utxosetscan);

This comment has been minimized.

@promag

promag Jan 16, 2018

Member

Assert not reserved?

assert(!m_could_reserve);
std::lock_guard<std::mutex> lock(g_utxosetscan);
if (g_scan_in_progress) return false;
g_scan_in_progress = true;
m_could_reserve = true;
return true;

This comment has been minimized.

@jonasschnelli
@laanwj

This comment has been minimized.

Member

laanwj commented Jan 16, 2018

Concept ACK, nice! I've wished for UTXO scanning functionality many times, much faster than importing into a watchonly wallet if you only care about spendable UTXOs.

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Jan 20, 2018

  • Added support for addresses (can scan unspent outputs after given addresses)
  • Added support for an optional raw sweep transaction including optional feerate or optional confirmation target
@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Jan 20, 2018

The raw sweep fee calculation is currently WIP (misses the dummy signer part)... will fix soon.

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Jan 21, 2018

Overhauled the fee calculation logic (see the dummy sign keystore).

// flush utxo state and start the scan
FlushStateToDisk();
bool res = pcoinsdbview->FindScriptPubKey(g_scan_progress, g_should_abort_scan, count, needles, coins);

This comment has been minimized.

@gmaxwell

gmaxwell Jan 22, 2018

Member

What prevents the state from being mutated out from under this scan?

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 22, 2018

Member

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".

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 22, 2018

Member

Also, the rawtx (sweep) is in the same way "outdated" the moment you have received it... maybe you could argue that this is the same for fundrawtx

This comment has been minimized.

@sipa

sipa May 18, 2018

Member

@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).

@luke-jr

This comment has been minimized.

Member

luke-jr commented Jan 24, 2018

NACK supporting addresses. Addresses have no relation to the UTXOs.

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Jan 24, 2018

@luke-jr: Why? Addresses are encoded output scripts (scriptPubKey). The rational behind supporting addresses is that a) it may be more efficient then forming every possible known common script from a pubkey and b) that pubkeys are somewhat more difficult to export then the pure "used addresses".

@luke-jr

This comment has been minimized.

Member

luke-jr commented Jan 24, 2018

Addresses are opaque identifiers for a given invoice. That they are currently implemented by encoding a scriptPubKey is irrelevant.

@greenaddress

This comment has been minimized.

Contributor

greenaddress commented Feb 26, 2018

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?

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Feb 27, 2018

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.

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Feb 27, 2018

Rebased

@luke-jr

This comment has been minimized.

Member

luke-jr commented Feb 28, 2018

The commit separation here is ugly: CCoinsView::FindScriptPubKey initially checks ShutdownRequested directly, and then this is removed with the RPC changes.

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Feb 28, 2018

Fixed the ugly commit separation in coins.cpp

@jonasschnelli jonasschnelli referenced this pull request May 12, 2018

Open

Motives unclear #5

@laanwj laanwj added this to Blockers in High-priority for review May 17, 2018

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented May 18, 2018

Rebased

@sipa

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.

@@ -19,6 +23,41 @@ bool CCoinsView::HaveCoin(const COutPoint &outpoint) const
return GetCoin(outpoint, coin);
}
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) {

This comment has been minimized.

@sipa

sipa May 18, 2018

Member

Can this be made a function instead of a method? It doesn't seem like it needs access to any of the class's internals.

This comment has been minimized.

@jonasschnelli

jonasschnelli May 25, 2018

Member

Since we access the instance cursor (CCoinsView()::Cursor()), wouldn't it then require to access pcoinsdbview from within coins.cpp which seems not ideal?

src/coins.h Outdated
@@ -157,6 +160,10 @@ class CCoinsView
//! Retrieve the block hash whose state this CCoinsView currently represents
virtual uint256 GetBestBlock() const;
//! Search for a given set of pubkey scripts
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);

This comment has been minimized.

@sipa

sipa May 18, 2018

Member

Nit: style for variable names.

// flush utxo state and start the scan
FlushStateToDisk();
bool res = pcoinsdbview->FindScriptPubKey(g_scan_progress, g_should_abort_scan, count, needles, coins);

This comment has been minimized.

@sipa

sipa May 18, 2018

Member

@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).

}
}
// add all common scripts for the given and derived pubkeys

This comment has been minimized.

@sipa

sipa May 18, 2018

Member

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.

This comment has been minimized.

@MarcoFalke

MarcoFalke May 21, 2018

Member

Agree that just checking for exact matches (scriptPubKey from address) is probably sufficient. If scripts really need to be translated between address types, it can be done in the client (wallet) software or a third party tool by the caller. Thus the implementation is simplified here by removing this whole code block.

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented May 19, 2018

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).

@MarcoFalke

utACK 94c7270

Left inline comments on some of the commits.

} else if (request.params[0].get_str() == "abort") {
CoinsViewScanReserver reserver;
if (reserver.reserve()) {
return false;

This comment has been minimized.

@MarcoFalke

MarcoFalke May 21, 2018

Member

Since this return value is subject to several races [1], I believe you could just return NullUniValue;

[1]:

  • e.g. the scan finishes between the last call and this one
  • e.g. this call returns true, but the last start call was not (yet) actually aborted

This comment has been minimized.

@jonasschnelli

jonasschnelli May 25, 2018

Member

Another way would be to wait at this point (with a conditional variable) until the scan has aborted. Objections?

if (reserver.reserve()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "No scan in progress");
}
result.pushKV("progress", g_scan_progress);

This comment has been minimized.

@MarcoFalke

MarcoFalke May 21, 2018

Member

Could add return result (early return) to clarify that nothing else happens in the hundred lines below for this case.

if (request.params[0].get_str() == "status") {
CoinsViewScanReserver reserver;
if (reserver.reserve()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "No scan in progress");

This comment has been minimized.

@MarcoFalke

MarcoFalke May 21, 2018

Member

Why would this be an exception, considering that the scan could normally finish between the last call and this one? Generally I think we should avoid exceptions for control flow in our code (and especially in third party code that uses the rpc interface)

}
}
// add all common scripts for the given and derived pubkeys

This comment has been minimized.

@MarcoFalke

MarcoFalke May 21, 2018

Member

Agree that just checking for exact matches (scriptPubKey from address) is probably sufficient. If scripts really need to be translated between address types, it can be done in the client (wallet) software or a third party tool by the caller. Thus the implementation is simplified here by removing this whole code block.

throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
}
CScript script = GetScriptForDestination(dest);
if (!script.empty()) {

This comment has been minimized.

@MarcoFalke

MarcoFalke May 21, 2018

Member

I am pretty sure this never happens when the destination is valid (which you checked two lines above.

This comment has been minimized.

@promag

promag May 22, 2018

Member

How about assert(!script.empty())?

class CCoinsViewScanDummySignKeyStore : public CBasicKeyStore
{
public:
bool GetPubKey(const CKeyID &address, CPubKey& vchPubKeyOut) const {

This comment has been minimized.

@MarcoFalke

MarcoFalke May 21, 2018

Member

can be marked override

vchPubKeyOut = pub_key;
return true;
}
bool GetCScript(const CScriptID &hash, CScript& redeemScriptOut) const {

This comment has been minimized.

@MarcoFalke

MarcoFalke May 21, 2018

Member

can be marked override

@@ -1704,6 +1729,9 @@ UniValue scantxoutset(const JSONRPCRequest& request)
" }\n"
" ,...], \n"
" \"total_amount\" : x.xxx, (numeric) The total amount of all found unspent outputs in " + CURRENCY_UNIT + "\n"
" \"rawsweep_tx\" : \"value\", (string) The hex-encoded raw transaction of the optional sweep transaction\n"
" \"rawsweep_vsize\" : \"value\", (numeric) virtual transaction size of the sweep transaction including signatures\n"

This comment has been minimized.

@MarcoFalke

MarcoFalke May 21, 2018

Member

I think the vsizes of the signatures are only estimated?

@@ -1692,6 +1712,11 @@ UniValue scantxoutset(const JSONRPCRequest& request)
" \"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"
" }\n"
" ]\n"
" \"rawsweep\": {\n (object, optional) Optionally creates a raw sweep transaction\n"
" \"address\": \"address\", (string, optional) Address where the funds should be sent to\n"
" \"feerate\": n, (numeric, optional, default not set: makes wallet determine the fee) Set a specific fee rate in " + CURRENCY_UNIT + "/kB\n"

This comment has been minimized.

@MarcoFalke

MarcoFalke May 21, 2018

Member

I believe this doesn't use the wallet, but the conf_target instead?

" \"rawsweep\": {\n (object, optional) Optionally creates a raw sweep transaction\n"
" \"address\": \"address\", (string, optional) Address where the funds should be sent to\n"
" \"feerate\": n, (numeric, optional, default not set: makes wallet determine the fee) Set a specific fee rate in " + CURRENCY_UNIT + "/kB\n"
" \"conf_target\": n, (numeric, optional) Confirmation target (in blocks), has no effect if feerate is provided\n"

This comment has been minimized.

@MarcoFalke

MarcoFalke May 21, 2018

Member

I believe the default for conf_target is 6?

@promag

Some comments.

{
if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
throw std::runtime_error(
"scantxoutset <action> {\"pubkeys\": [\"pubkey\",...], \"xpubs\":[{\"xpub\": \"<xpub>\"}]}\n"

This comment has been minimized.

@promag

promag May 22, 2018

Member

Maybe change this to

    "scantxoutset \"action\" ( option )\n"
" \"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"
" }\n"
" ]\n"
" \"rawsweep\": {\n (object, optional) Optionally creates a raw sweep transaction\n"

This comment has been minimized.

@promag

promag May 22, 2018

Member

Remove \n.

" [\n"
" {\n"
" \"xpub\":\"<xpub>\", (base58check encoded extended public key (xpub)\n"
" \"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"

This comment has been minimized.

@promag

promag May 22, 2018

Member

Needs 2 spaces between argument name and description.

" \"rawsweep_tx\" : \"value\", (string) The hex-encoded raw transaction of the optional sweep transaction\n"
" \"rawsweep_vsize\" : \"value\", (numeric) virtual transaction size of the sweep transaction including signatures\n"
" \"rawsweep_fee\" : \"value\", (numeric) Estimated fee for the sweep transaction in " + CURRENCY_UNIT + "\n"
"]\n"

This comment has been minimized.

@promag

promag May 22, 2018

Member

Replace ] with }.

This comment has been minimized.

@promag

promag Jul 12, 2018

Member

Commit "Commit "Blockchain/RPC: Add scantxoutset method to scan UTXO set"

The response is an object, not an array.

" }\n"
" ,...], \n"
" \"total_amount\" : x.xxx, (numeric) The total amount of all found unspent outputs in " + CURRENCY_UNIT + "\n"
" \"rawsweep_tx\" : \"value\", (string) The hex-encoded raw transaction of the optional sweep transaction\n"

This comment has been minimized.

@promag

promag May 22, 2018

Member

Needs 2 spaces before description.

" ,...], \n"
" \"total_amount\" : x.xxx, (numeric) The total amount of all found unspent outputs in " + CURRENCY_UNIT + "\n"
" \"rawsweep_tx\" : \"value\", (string) The hex-encoded raw transaction of the optional sweep transaction\n"
" \"rawsweep_vsize\" : \"value\", (numeric) virtual transaction size of the sweep transaction including signatures\n"

This comment has been minimized.

@promag

promag May 22, 2018

Member

Needs 1 space before description.

" \"total_amount\" : x.xxx, (numeric) The total amount of all found unspent outputs in " + CURRENCY_UNIT + "\n"
" \"rawsweep_tx\" : \"value\", (string) The hex-encoded raw transaction of the optional sweep transaction\n"
" \"rawsweep_vsize\" : \"value\", (numeric) virtual transaction size of the sweep transaction including signatures\n"
" \"rawsweep_fee\" : \"value\", (numeric) Estimated fee for the sweep transaction in " + CURRENCY_UNIT + "\n"

This comment has been minimized.

@promag

promag May 22, 2018

Member

Needs 1 space before description.

if (request.params[0].get_str() == "status") {
CoinsViewScanReserver reserver;
if (reserver.reserve()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "No scan in progress");

This comment has been minimized.

@promag

promag May 22, 2018

Member

Yes, agree that status should not raise an error when no scan is in progress.

throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
}
CScript script = GetScriptForDestination(dest);
if (!script.empty()) {

This comment has been minimized.

@promag

promag May 22, 2018

Member

How about assert(!script.empty())?

" }\n"
" ,...], \n"
" \"total_amount\" : x.xxx, (numeric) The total amount of all found unspent outputs in " + CURRENCY_UNIT + "\n"
" \"rawsweep_tx\" : \"value\", (string) The hex-encoded raw transaction of the optional sweep transaction\n"

This comment has been minimized.

@promag

promag May 22, 2018

Member

Put these in an object "rawsweep": { }?

needles.insert(script);
temp_keystore.AddWatchOnly(script);
}
// add P2SH-P2WPKH script

This comment has been minimized.

@achow101

achow101 May 27, 2018

Member

I think we should avoid doing this step for uncompressed pubkeys as such outputs would be unspendable.

return true;
}
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) {

This comment has been minimized.

@achow101

achow101 May 27, 2018

Member

nit: searchItems should be search_items to match the .h file.

" {\n"
" \"xpub\":\"<xpub>\", (base58check encoded extended public key (xpub)\n"
" \"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"
" }\n"

This comment has been minimized.

@achow101

achow101 May 27, 2018

Member

nit: should have ,... after this.

" \"amount\" : x.xxx, (numeric) The total amount in " + CURRENCY_UNIT + " received by the address\n"
" \"height\" : n, (numeric) Height of the unspent transaction output\n"
" }\n"
" ,...], \n"

This comment has been minimized.

@achow101

achow101 May 27, 2018

Member

nit: the spacing of this block looks a bit funny. I think the text should be indented more and the braces for the object should be aligned.

@promag

Nit, remove fee handling? The caller could do:

tx = scantxoutset("start", { "sweep_to": address })["sweep_tx"]
fundrawtransaction(tx, { "subtractFeeFromOutputs": [0] })
...

So scantxoutset should just add the unspents and one output.

@Sjors Sjors referenced this pull request May 29, 2018

Open

Use scantxoutset? #27

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Jul 12, 2018

  • Moved the FindScriptPubKey() method to rpc/blockchain.cpp (changes are no longer spread in multiple commits).
  • Removed the sweep transaction creation code (see #12196 (review))

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).

@luke-jr

Maintaining a NACK so long as it confuses addresses (which are unrelated to UTXOs) with keys...

assert(pcursor);
bool res = FindScriptPubKey(g_scan_progress, g_should_abort_scan, count, pcursor.get(), needles, coins);
result.push_back(Pair("success", res ? "yes" : "no"));

This comment has been minimized.

@luke-jr

luke-jr Jul 12, 2018

Member

This should probably be true|false, not a string.

@@ -7,6 +7,7 @@
#include <consensus/consensus.h>
#include <random.h>

This comment has been minimized.

@promag

promag Jul 12, 2018

Member

Commit "Add FindScriptPubKey() to search the UTXO set"

nit, unrelated change.

@@ -1916,6 +1916,35 @@ static UniValue savemempool(const JSONRPCRequest& request)
return NullUniValue;
}
//! Search for a given set of pubkey scripts
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) {

This comment has been minimized.

@promag

promag Jul 12, 2018

Member

Commit "Add FindScriptPubKey() to search the UTXO set"

nit, CCoinsViewCursor* cursor.

explicit CoinsViewScanReserver() : m_could_reserve(false) {}
bool reserve() {
assert (!m_could_reserve);

This comment has been minimized.

@promag

promag Jul 12, 2018

Member

Commit "Blockchain/RPC: Add scantxoutset method to scan UTXO set"

Missing fix :trollface:

case OutputScriptType::P2WPKH: {
if (!key.IsCompressed()) return key.GetID();
CTxDestination witdest = WitnessV0KeyHash(key.GetID());
CScript witprog = GetScriptForDestination(witdest);

This comment has been minimized.

@promag

promag Jul 12, 2018

Member

Commit "Blockchain/RPC: Add scantxoutset method to scan UTXO set"

This could be in the block below:

CTxDestination witdest = WitnessV0KeyHash(key.GetID());
if (type == OutputScriptType::P2SH_P2WPKH) {
    CScript witprog = GetScriptForDestination(witdest);
    return CScriptID(witprog);
} else {
    ...
UniValue scantxoutset(const JSONRPCRequest& request)
{
if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)

This comment has been minimized.

@promag

promag Jul 12, 2018

Member

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)?

{
if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
throw std::runtime_error(
"scantxoutset <action> <scanobjects> ( <options> )\n"

This comment has been minimized.

@promag

promag Jul 12, 2018

Member

Commit "Blockchain/RPC: Add scantxoutset method to scan UTXO set"

nit, add ( ) to <scanobjects> because status action has no more arguments.

" \"rawsweep_tx\" : \"value\", (string) The hex-encoded raw transaction of the optional sweep transaction\n"
" \"rawsweep_vsize\" : \"value\", (numeric) virtual transaction size of the sweep transaction including signatures\n"
" \"rawsweep_fee\" : \"value\", (numeric) Estimated fee for the sweep transaction in " + CURRENCY_UNIT + "\n"
"]\n"

This comment has been minimized.

@promag

promag Jul 12, 2018

Member

Commit "Commit "Blockchain/RPC: Add scantxoutset method to scan UTXO set"

The response is an object, not an array.

int64_t count = 0;
FlushStateToDisk();
std::unique_ptr<CCoinsViewCursor> pcursor(pcoinsdbview->Cursor());

This comment has been minimized.

@sipa

sipa Jul 12, 2018

Member

You should hold cs_main while calling FlushStateToDisk and creating the cursor, or there is no guarantee that the on-disk state is consistent.

This comment has been minimized.

@promag

promag Jul 12, 2018

Member

Looks like some annotations or lock assertions could be added?

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 12, 2018

Member

Agree with @promag, but unrelated to this PR.

This comment has been minimized.

@promag

promag Jul 12, 2018

Member

Oh, I wasn't implying that. I'll follow up.

This comment has been minimized.

@promag

promag Jul 12, 2018

Member

Ops, actually @sipa point is about locking during FlushStateToDisk() and Cursor(), so the lock must be added here, preferably in this PR IMO.

throw JSONRPCError(RPC_INVALID_PARAMETER, "Scan already in progress, use action \"abort\" or \"status\"");
}
std::set<CScript> needles;
CBasicKeyStore temp_keystore;

This comment has been minimized.

@sipa

sipa Jul 12, 2018

Member

This looks unused.

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Jul 12, 2018

Fixed issues:

  • holds now cs_main during coins view cursor creation and FlushToDisk()
  • Removed temp_keystore (relict from raw-sweep-tx).
  • Fixed "yes"/"no" instead of true/false JSON returns
  • Fixed nits
if (count % 256 == 0) {
// update progress reference every 256 item
uint32_t high = 0x100 * *key.hash.begin() + *(key.hash.begin() + 1);
scan_progress = (int)(high * 100.0 / 65536.0 + 0.5);

This comment has been minimized.

@jamesob

jamesob Jul 12, 2018

Member

Nit: code for this seems like it's cropping up in a few different places, might be nice to have an abstraction for it.

 $ git grep -C 1 "/ 65536" | cat

src/index/txindex.cpp-                (static_cast<uint32_t>(*(txid.begin() + 1)) << 0);
src/index/txindex.cpp:            int percentage_done = (int)(high_nibble * 100.0 / 65536.0 + 0.5);
src/index/txindex.cpp-
--
src/txdb.cpp-                uint32_t high = 0x100 * *key.second.begin() + *(key.second.begin() + 1);
src/txdb.cpp:                int percentageDone = (int)(high * 100.0 / 65536.0 + 0.5);
src/txdb.cpp-                uiInterface.ShowProgress(_("Upgrading UTXO database"), percentageDone, true);

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 13, 2018

Member

IMO this should be done outside of this PR

{
if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
throw std::runtime_error(
"scantxoutset <action> ( <scanobjects> ) ( <options> )\n"

This comment has been minimized.

@jamesob

jamesob Jul 12, 2018

Member

If scanobjects is required (per doc below), should it be in parens here?

This comment has been minimized.

@jamesob

jamesob Jul 12, 2018

Member

Also options doesn't seem to exist.

FlushStateToDisk();
std::unique_ptr<CCoinsViewCursor> pcursor(pcoinsdbview->Cursor());
assert(pcursor);
bool res = FindScriptPubKey(g_scan_progress, g_should_abort_scan, count, pcursor.get(), needles, coins);

This comment has been minimized.

@jamesob

jamesob Jul 12, 2018

Member

Can we release cs_main after initializing pcursor? My impression is that CCoinsViewCursor can be used for read-only ops without holding it.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 13, 2018

Member

Oh. Right. Fixed.

@jamesob

This comment has been minimized.

Member

jamesob commented Jul 12, 2018

Concept ACK

Happy to do a more in-depth review. PR description looks in need of an update.

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Jul 13, 2018

Fixed points reported by @jamesob.

assert(pcursor);
}
bool res = FindScriptPubKey(g_scan_progress, g_should_abort_scan, count, pcursor.get(), needles, coins);
result.pushKV("success", res ? true : false);

This comment has been minimized.

@luke-jr

luke-jr Jul 13, 2018

Member

res is already a boolean...

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 13, 2018

Member

Ouch! Fixed.

CScript script;
if (script_type == OutputScriptType::P2PK) {
// support legacy P2PK scripts
script << ToByteVector(pubkey) << OP_CHECKSIG;

This comment has been minimized.

@sipa

sipa Jul 14, 2018

Member

Nit: use GetScriptForRawPubKey.

}
g_should_abort_scan = true;
return true;
} else if (request.params[0].get_str() == "start") {

This comment has been minimized.

@Empact

Empact Jul 15, 2018

Member

nit: Would be nice to store this repeated arg to a local

} else if (!pubkey_uni.isNull() && !pubkey_uni.isObject()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Scanobject \"pubkey\" must contain an object as value");
} else if (!script_uni.isNull() && !script_uni.isStr()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Scanobject \"script\" must contain a single string as value");

This comment has been minimized.

@Empact

Empact Jul 15, 2018

Member

You could remove most of the above with:

RPCTypeCheckObj(scanobject,
    {
        {"address", UniValueType(UniValue::VSTR)},
        {"pubkey", UniValueType(UniValue::VOBJ)},
        {"script", UniValueType(UniValue::VSTR)},
    }, true /* fAllowNull */);

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 15, 2018

Member

I guess your proposed check does not ensure that one of the (either address, pubkey or script must be present, right?

This comment has been minimized.

@Empact

Empact Jul 15, 2018

Member

Right, it just handles the type testing.

UniValue script_uni = find_value(scanobject, "script");
// make sure only one object type is present
if (1 != !address_uni.isNull() + !pubkey_uni.isNull() + !script_uni.isNull()) {

This comment has been minimized.

@Empact

Empact Jul 15, 2018

Member

nit: explicit precedence?

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 15, 2018

Member

I think it helps for the readability? Or can you elaborate what you mean exactly?

This comment has been minimized.

@Empact

Empact Jul 15, 2018

Member

Suggesting to group the sum in parens so that it's more clear that there is only the one top-level condition.

return false;
}
g_should_abort_scan = true;
return true;

This comment has been minimized.

@Empact

Empact Jul 15, 2018

Member

Could you document the "status" and "abort" results above?

nit: I'd somewhat prefer each of these having their own rpc

// loop through the script types and derive the script
for (const UniValue& script_type_uni : script_types_uni.get_array().getValues()) {
OutputScriptType script_type = GetOutputScriptTypeFromString(script_type_uni.get_str());
if (script_type == OutputScriptType::UNKNOWN) throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid script type");

This comment has been minimized.

@Empact

Empact Jul 15, 2018

Member

Could throw above and do away with UNKNOWN

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 15, 2018

Member

I don't understand your comment... can you rephrase or elaborate in detail?

This comment has been minimized.

@Empact

Empact Jul 15, 2018

Member

If you throw on UNKNOWN within GetOutputScriptTypeFromString, then there is no other use of UNKNOWN as an OutputScriptType.

}
};
const char *g_default_scantxoutset_script_types[] = { "P2PKH", "P2SH_P2WPKH", "P2WPKH" };

This comment has been minimized.

@Empact

Empact Jul 15, 2018

Member

nit: maybe make this a unival to avoid translation below?
nit: why not static?

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 15, 2018

Member

Agree. Made static.

throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Public key must be hex encoded");
}
std::vector<unsigned char> data(ParseHexV(pubkeydata_uni, "pubkey"));
CPubKey pubkey(data.begin(), data.end());

This comment has been minimized.

@Empact

Empact Jul 15, 2018

Member

You can construct CPubKey directly from the ParseHexV result.

explicit CPubKey(const std::vector<unsigned char>& _vch)

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 15, 2018

Member

Indeed. Fixed.

// type: script
// check and add the script to the scan containers (needles array)
std::vector<unsigned char> scriptData(ParseHexV(script_uni, "script"));
CScript script(scriptData.begin(), scriptData.end());

This comment has been minimized.

@Empact

Empact Jul 15, 2018

Member

explicit CPubKey(const std::vector<unsigned char>& _vch)

This comment has been minimized.

@jonasschnelli
self.log.info("Test if we have found the non HD unspent outputs.")
assert_equal(self.nodes[0].scantxoutset("start", [ {"pubkey": {"pubkey": pubk1}}, {"pubkey": {"pubkey": pubk2}}, {"pubkey": {"pubkey": pubk3}}])['total_amount'], 6)
assert_equal(self.nodes[0].scantxoutset("start", [ {"address": addr_P2SH_SEGWIT}, {"address": addr_LEGACY}, {"address": addr_BECH32}])['total_amount'], 6)
assert_equal(self.nodes[0].scantxoutset("start", [ {"address": addr_P2SH_SEGWIT}, {"address": addr_LEGACY}, {"pubkey": {"pubkey": pubk3}} ])['total_amount'], 6)

This comment has been minimized.

@Empact

Empact Jul 15, 2018

Member

Tests for "abort" and "status"?

This comment has been minimized.

@jonasschnelli

jonasschnelli Jul 15, 2018

Member

Both would probably require additional code for adding time delays or temporary creation of a large utxo set.

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) {
scan_progress = 0;
count = 0;
while (cursor->Valid()) {

This comment has been minimized.

@Empact

Empact Jul 15, 2018

Member

nit: for would localize the cursor access

jonasschnelli added some commits Jan 16, 2018

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Jul 15, 2018

Fixed relevant points from @Empact

@laanwj laanwj added this to the 0.17.0 milestone Jul 17, 2018

@laanwj laanwj merged commit be98b2d into bitcoin:master Jul 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 17, 2018

Merge #12196: Add scantxoutset RPC method
be98b2d [QA] Add scantxoutset test (Jonas Schnelli)
eec7cf7 scantxoutset: mention that scanning by address will miss P2PK txouts (Jonas Schnelli)
94d73d3 scantxoutset: support legacy P2PK script type (Jonas Schnelli)
892de1d scantxoutset: add support for scripts (Jonas Schnelli)
7830494 Blockchain/RPC: Add scantxoutset method to scan UTXO set (Jonas Schnelli)
9048575 Add FindScriptPubKey() to search the UTXO set (Jonas Schnelli)

Pull request description:

  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.

Tree-SHA512: a2b22a117cf6e27febeb97e5d6fe30184926d50c0c7cbc77bb4121f490fed65560c52f8eac67a9720d7bf8f420efa42459768685c7e7cc03722859f51a5e1e3b
@promag

This comment has been minimized.

Member

promag commented Jul 17, 2018

utACK be98b2d.

@jonasschnelli jonasschnelli removed this from Blockers in High-priority for review Jul 17, 2018

laanwj added a commit that referenced this pull request Aug 1, 2018

Merge #13697: Support output descriptors in scantxoutset
f6b7fc3 Support h instead of ' in hardened descriptor paths (Pieter Wuille)
fddea67 Add experimental warning to scantxoutset (Jonas Schnelli)
6495849 [QA] Extend tests to more combinations (Pieter Wuille)
1af237f [QA] Add xpub range tests in scantxoutset tests (Jonas Schnelli)
151600b Swap in descriptors support into scantxoutset (Pieter Wuille)
0652c32 Descriptor tests (Pieter Wuille)
fe8a7dc Output descriptors module (Pieter Wuille)
e54d760 Add simple FlatSigningProvider (Pieter Wuille)
29943a9 Add more methods to Span class (Pieter Wuille)

Pull request description:

  As promised, here is an implementation of my output descriptor concept (https://gist.github.com/sipa/e3d23d498c430bb601c5bca83523fa82) and integration within the `scantxoutset` RPC that was just added through #12196.

  It changes the RPC to use descriptors for everything; I hope the interface is simple enough to encompass all use cases. It includes support for P2PK, P2PKH, P2WPKH, P2SH, P2WSH, multisig, xpubs, xprvs, and chains of keys - combined in every possible way.

Tree-SHA512: 63b54a96e7a72f5b04a8d645b8517d43ecd6a65a41f9f4e593931ce725a8845ab0baa1e9db6a7243190d8ac841f6e7e2f520d98c539312d78f7fd687d2c7b88f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment