From a31ba7a0cb855e4419470a7175d2a325c86bc06a Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 31 Jan 2017 17:19:11 +0100 Subject: [PATCH 1/4] Usability improvements for z_importkey - Add height parameter to z_importkey to reduce rescan range - Change semantics of rescan parameter, so users can explicitly force a rescan for existing keys. Closes #2032 --- qa/rpc-tests/wallet_1941.py | 20 ++++++++++++-- src/rpcclient.cpp | 2 +- src/test/rpc_wallet_tests.cpp | 2 +- src/wallet/rpcdump.cpp | 50 ++++++++++++++++++++++++++--------- 4 files changed, 57 insertions(+), 17 deletions(-) diff --git a/qa/rpc-tests/wallet_1941.py b/qa/rpc-tests/wallet_1941.py index 4958768f9..999adda94 100755 --- a/qa/rpc-tests/wallet_1941.py +++ b/qa/rpc-tests/wallet_1941.py @@ -31,6 +31,14 @@ class Wallet1941RegressionTest (BitcoinTestFramework): connect_nodes_bi(self.nodes,0,1) self.sync_all() + def restart_second_node(self, extra_args=[]): + self.nodes[1].stop() + bitcoind_processes[1].wait() + self.nodes[1] = start_node(1, self.options.tmpdir, extra_args=['-regtestprotectcoinbase','-debug=zrpc'] + extra_args) + self.nodes[1].setmocktime(starttime + 9000) + connect_nodes_bi(self.nodes, 0, 1) + self.sync_all() + def wait_and_assert_operationid_status(self, myopid, in_status='success', in_errormsg=None): print('waiting for async operation {}'.format(myopid)) opids = [] @@ -93,8 +101,16 @@ class Wallet1941RegressionTest (BitcoinTestFramework): self.nodes[1].generate(101) self.sync_all() - # Import the key on node 1. - self.nodes[1].z_importkey(key) + # Import the key on node 1, only scanning the last few blocks. + self.nodes[1].z_importkey(key, 'true', self.nodes[1].getblockchaininfo()['blocks'] - 100) + + # Confirm that the balance on node 1 is zero, as we have not + # rescanned over the older transactions + resp = self.nodes[1].z_getbalance(myzaddr) + assert_equal(Decimal(resp), 0) + + # Re-import the key on node 1, scanning from before the transaction. + self.nodes[1].z_importkey(key, 'true', self.nodes[1].getblockchaininfo()['blocks'] - 110) # Confirm that the balance on node 1 is valid now (node 1 must # have rescanned) diff --git a/src/rpcclient.cpp b/src/rpcclient.cpp index f173afe2c..07adf65fc 100644 --- a/src/rpcclient.cpp +++ b/src/rpcclient.cpp @@ -111,7 +111,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "z_sendmany", 3}, { "z_getoperationstatus", 0}, { "z_getoperationresult", 0}, - { "z_importkey", 1 } + { "z_importkey", 2 }, }; class CRPCConvertTable diff --git a/src/test/rpc_wallet_tests.cpp b/src/test/rpc_wallet_tests.cpp index ea74d52f3..5529b864b 100644 --- a/src/test/rpc_wallet_tests.cpp +++ b/src/test/rpc_wallet_tests.cpp @@ -500,7 +500,7 @@ BOOST_AUTO_TEST_CASE(rpc_wallet_z_importexport) BOOST_CHECK_THROW(CallRPC("z_exportkey"), runtime_error); // error if too many args - BOOST_CHECK_THROW(CallRPC("z_importkey too many args"), runtime_error); + BOOST_CHECK_THROW(CallRPC("z_importkey way too many args"), runtime_error); BOOST_CHECK_THROW(CallRPC("z_exportkey toomany args"), runtime_error); // wallet should currently be empty diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 7ae68b994..f437f80bc 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -552,19 +552,24 @@ UniValue z_importkey(const UniValue& params, bool fHelp) if (!EnsureWalletIsAvailable(fHelp)) return NullUniValue; - if (fHelp || params.size() < 1 || params.size() > 2) + if (fHelp || params.size() < 1 || params.size() > 3) throw runtime_error( - "z_importkey \"zkey\" ( rescan )\n" + "z_importkey \"zkey\" ( rescan startHeight )\n" "\nAdds a zkey (as returned by z_exportkey) to your wallet.\n" "\nArguments:\n" "1. \"zkey\" (string, required) The zkey (see z_exportkey)\n" - "2. rescan (boolean, optional, default=true) Rescan the wallet for transactions\n" + "2. rescan (boolean or \"whenkeyisnew\", optional, default=\"whenkeyisnew\") Rescan the wallet for transactions\n" + "3. startHeight (numeric, optional, default=0) Block height to start rescan from\n" "\nNote: This call can take minutes to complete if rescan is true.\n" "\nExamples:\n" "\nExport a zkey\n" + HelpExampleCli("z_exportkey", "\"myaddress\"") + "\nImport the zkey with rescan\n" + HelpExampleCli("z_importkey", "\"mykey\"") + + "\nImport the zkey with partial rescan\n" + + HelpExampleCli("z_importkey", "\"mykey\", \"whenkeyisnew\", 30000") + + "\nRe-import the zkey with longer partial rescan\n" + + HelpExampleCli("z_importkey", "\"mykey\", true, 20000") + "\nAs a JSON-RPC call\n" + HelpExampleRpc("z_importkey", "\"mykey\", false") ); @@ -575,8 +580,24 @@ UniValue z_importkey(const UniValue& params, bool fHelp) // Whether to perform rescan after import bool fRescan = true; - if (params.size() > 1) - fRescan = params[1].get_bool(); + bool fIgnoreExistingKey = true; + if (params.size() > 1) { + auto rescan = params[1].get_str(); + if (rescan.compare("whenkeyisnew") != 0) { + fIgnoreExistingKey = false; + UniValue jVal; + if (!jVal.read(std::string("[")+rescan+std::string("]")) || + !jVal.isArray() || jVal.size()!=1 || !jVal[0].isBool()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "rescan must be bool or \"whenkeyisnew\""); + } + fRescan = jVal[0].getBool(); + } + } + + // Height to rescan from + int nRescanHeight = 0; + if (params.size() > 2) + nRescanHeight = params[2].get_int(); string strSecret = params[0].get_str(); CZCSpendingKey spendingkey(strSecret); @@ -585,22 +606,25 @@ UniValue z_importkey(const UniValue& params, bool fHelp) { // Don't throw error in case a key is already there - if (pwalletMain->HaveSpendingKey(addr)) - return NullUniValue; + if (pwalletMain->HaveSpendingKey(addr)) { + if (fIgnoreExistingKey) { + return NullUniValue; + } + } else { + pwalletMain->MarkDirty(); - pwalletMain->MarkDirty(); + if (!pwalletMain-> AddZKey(key)) + throw JSONRPCError(RPC_WALLET_ERROR, "Error adding spending key to wallet"); - if (!pwalletMain-> AddZKey(key)) - throw JSONRPCError(RPC_WALLET_ERROR, "Error adding spending key to wallet"); - - pwalletMain->mapZKeyMetadata[addr].nCreateTime = 1; + pwalletMain->mapZKeyMetadata[addr].nCreateTime = 1; + } // whenever a key is imported, we need to scan the whole chain pwalletMain->nTimeFirstKey = 1; // 0 would be considered 'no value' // We want to scan for transactions and notes if (fRescan) { - pwalletMain->ScanForWalletTransactions(chainActive.Genesis(), true); + pwalletMain->ScanForWalletTransactions(chainActive[nRescanHeight], true); } } From b7e5b7d5eefabfbb24672d67005e2f4fb21449f2 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 23 Mar 2017 15:57:11 +1300 Subject: [PATCH 2/4] Simplify z_importkey by making rescan a string Transparently handles older boolean values as well. --- qa/rpc-tests/wallet_1941.py | 4 ++-- src/wallet/rpcdump.cpp | 27 ++++++++++++++++++--------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/qa/rpc-tests/wallet_1941.py b/qa/rpc-tests/wallet_1941.py index 999adda94..2e6772460 100755 --- a/qa/rpc-tests/wallet_1941.py +++ b/qa/rpc-tests/wallet_1941.py @@ -102,7 +102,7 @@ class Wallet1941RegressionTest (BitcoinTestFramework): self.sync_all() # Import the key on node 1, only scanning the last few blocks. - self.nodes[1].z_importkey(key, 'true', self.nodes[1].getblockchaininfo()['blocks'] - 100) + self.nodes[1].z_importkey(key, 'yes', self.nodes[1].getblockchaininfo()['blocks'] - 100) # Confirm that the balance on node 1 is zero, as we have not # rescanned over the older transactions @@ -110,7 +110,7 @@ class Wallet1941RegressionTest (BitcoinTestFramework): assert_equal(Decimal(resp), 0) # Re-import the key on node 1, scanning from before the transaction. - self.nodes[1].z_importkey(key, 'true', self.nodes[1].getblockchaininfo()['blocks'] - 110) + self.nodes[1].z_importkey(key, 'yes', self.nodes[1].getblockchaininfo()['blocks'] - 110) # Confirm that the balance on node 1 is valid now (node 1 must # have rescanned) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index f437f80bc..c9b3504ed 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -558,7 +558,7 @@ UniValue z_importkey(const UniValue& params, bool fHelp) "\nAdds a zkey (as returned by z_exportkey) to your wallet.\n" "\nArguments:\n" "1. \"zkey\" (string, required) The zkey (see z_exportkey)\n" - "2. rescan (boolean or \"whenkeyisnew\", optional, default=\"whenkeyisnew\") Rescan the wallet for transactions\n" + "2. rescan (string, optional, default=\"whenkeyisnew\") Rescan the wallet for transactions - can be \"yes\", \"no\" or \"whenkeyisnew\"\n" "3. startHeight (numeric, optional, default=0) Block height to start rescan from\n" "\nNote: This call can take minutes to complete if rescan is true.\n" "\nExamples:\n" @@ -567,11 +567,11 @@ UniValue z_importkey(const UniValue& params, bool fHelp) "\nImport the zkey with rescan\n" + HelpExampleCli("z_importkey", "\"mykey\"") + "\nImport the zkey with partial rescan\n" - + HelpExampleCli("z_importkey", "\"mykey\", \"whenkeyisnew\", 30000") + + + HelpExampleCli("z_importkey", "\"mykey\" whenkeyisnew 30000") + "\nRe-import the zkey with longer partial rescan\n" - + HelpExampleCli("z_importkey", "\"mykey\", true, 20000") + + + HelpExampleCli("z_importkey", "\"mykey\" yes 20000") + "\nAs a JSON-RPC call\n" - + HelpExampleRpc("z_importkey", "\"mykey\", false") + + HelpExampleRpc("z_importkey", "\"mykey\", \"no\"") ); LOCK2(cs_main, pwalletMain->cs_wallet); @@ -585,12 +585,21 @@ UniValue z_importkey(const UniValue& params, bool fHelp) auto rescan = params[1].get_str(); if (rescan.compare("whenkeyisnew") != 0) { fIgnoreExistingKey = false; - UniValue jVal; - if (!jVal.read(std::string("[")+rescan+std::string("]")) || - !jVal.isArray() || jVal.size()!=1 || !jVal[0].isBool()) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "rescan must be bool or \"whenkeyisnew\""); + if (rescan.compare("yes") == 0) { + fRescan = true; + } else if (rescan.compare("no") == 0) { + fRescan = false; + } else { + // Handle older API + UniValue jVal; + if (!jVal.read(std::string("[")+rescan+std::string("]")) || + !jVal.isArray() || jVal.size()!=1 || !jVal[0].isBool()) { + throw JSONRPCError( + RPC_INVALID_PARAMETER, + "rescan must be \"yes\", \"no\" or \"whenkeyisnew\""); + } + fRescan = jVal[0].getBool(); } - fRescan = jVal[0].getBool(); } } From 53c96bfa6ce96d7815774ee40d94024002b6a1e2 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 24 Mar 2017 09:23:11 +1300 Subject: [PATCH 3/4] Test boolean fallback in z_importkey --- qa/rpc-tests/wallet_1941.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/qa/rpc-tests/wallet_1941.py b/qa/rpc-tests/wallet_1941.py index 2e6772460..5f2fe3f72 100755 --- a/qa/rpc-tests/wallet_1941.py +++ b/qa/rpc-tests/wallet_1941.py @@ -102,7 +102,8 @@ class Wallet1941RegressionTest (BitcoinTestFramework): self.sync_all() # Import the key on node 1, only scanning the last few blocks. - self.nodes[1].z_importkey(key, 'yes', self.nodes[1].getblockchaininfo()['blocks'] - 100) + # (uses 'true' to test boolean fallback) + self.nodes[1].z_importkey(key, 'true', self.nodes[1].getblockchaininfo()['blocks'] - 100) # Confirm that the balance on node 1 is zero, as we have not # rescanned over the older transactions From 33589401ba7df5495a2e57c6c4edd113dff3bd8b Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 24 Mar 2017 16:05:32 +1300 Subject: [PATCH 4/4] Require that z_importkey height parameter be in valid range --- src/test/rpc_wallet_tests.cpp | 8 ++++++++ src/wallet/rpcdump.cpp | 3 +++ 2 files changed, 11 insertions(+) diff --git a/src/test/rpc_wallet_tests.cpp b/src/test/rpc_wallet_tests.cpp index 5529b864b..b25e0e234 100644 --- a/src/test/rpc_wallet_tests.cpp +++ b/src/test/rpc_wallet_tests.cpp @@ -503,6 +503,14 @@ BOOST_AUTO_TEST_CASE(rpc_wallet_z_importexport) BOOST_CHECK_THROW(CallRPC("z_importkey way too many args"), runtime_error); BOOST_CHECK_THROW(CallRPC("z_exportkey toomany args"), runtime_error); + // error if invalid args + auto sk = libzcash::SpendingKey::random(); + std::string prefix = std::string("z_importkey ") + CZCSpendingKey(sk).ToString() + " yes "; + BOOST_CHECK_THROW(CallRPC(prefix + "-1"), runtime_error); + BOOST_CHECK_THROW(CallRPC(prefix + "2147483647"), runtime_error); // allowed, but > height of active chain tip + BOOST_CHECK_THROW(CallRPC(prefix + "2147483648"), runtime_error); // not allowed, > int32 used for nHeight + BOOST_CHECK_THROW(CallRPC(prefix + "100badchars"), runtime_error); + // wallet should currently be empty std::set addrs; pwalletMain->GetPaymentAddresses(addrs); diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index c9b3504ed..936532837 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -607,6 +607,9 @@ UniValue z_importkey(const UniValue& params, bool fHelp) int nRescanHeight = 0; if (params.size() > 2) nRescanHeight = params[2].get_int(); + if (nRescanHeight < 0 || nRescanHeight > chainActive.Height()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Block height out of range"); + } string strSecret = params[0].get_str(); CZCSpendingKey spendingkey(strSecret);