Skip to content
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

Bugfix: z_importviewingkey RPC call missing startHeight #2937

Closed
Tomas-M opened this issue Feb 7, 2018 · 6 comments
Closed

Bugfix: z_importviewingkey RPC call missing startHeight #2937

Tomas-M opened this issue Feb 7, 2018 · 6 comments
Assignees
Labels
A-rpc-interface Area: RPC interface C-bug Category: This is a bug
Milestone

Comments

@Tomas-M
Copy link
Contributor

Tomas-M commented Feb 7, 2018

If I call RPC command z_importviewingkey with one or two arguments, it works OK.
But if I call RPC with three arguments, adding startHeight, RPC returns with error (usage text).

So it seems to me RPC is not configured properly, it does not understand the third argument.

Using zcash 1.0.14

@Tomas-M
Copy link
Contributor Author

Tomas-M commented Feb 7, 2018

Also the returned help text which prints correct usage example with zcash-cli does not have the third parameter in the example usage for JSON-RPC call with curl.

@Tomas-M
Copy link
Contributor Author

Tomas-M commented Feb 8, 2018

I believe something like this would probably fix it

diff --git a/src/rpcclient.cpp b/src/rpcclient.cpp
index def3250..59dbe95 100644
--- a/src/rpcclient.cpp
+++ b/src/rpcclient.cpp
@@ -117,7 +117,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "z_getoperationstatus", 0},
     { "z_getoperationresult", 0},
     { "z_importkey", 2 },
     { "z_importviewingkey", 2 },
+    { "z_importviewingkey", 3 },
     { "z_getpaymentdisclosure", 1},
     { "z_getpaymentdisclosure", 2}
 };

@Tomas-M Tomas-M changed the title Bug: z_importviewingkey RPC call missing startHeight? Bugfix: z_importviewingkey RPC call missing startHeight Feb 8, 2018
@Tomas-M
Copy link
Contributor Author

Tomas-M commented Feb 8, 2018

The previously mentioned patch is not sufficient. This is probably also needed:

diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
index 1177cfc..ca4193d 100644
--- a/src/wallet/rpcdump.cpp
+++ b/src/wallet/rpcdump.cpp
@@ -653,7 +653,7 @@ UniValue z_importviewingkey(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_importviewingkey \"vkey\" ( rescan startHeight )\n"
             "\nAdds a viewing key (as returned by z_exportviewingkey) to your wallet.\n"

@ioptio ioptio added this to Blocked in User Support Feb 12, 2018
@daira daira self-assigned this Feb 14, 2018
@daira daira added C-bug Category: This is a bug A-rpc-interface Area: RPC interface labels Feb 14, 2018
@Tomas-M
Copy link
Contributor Author

Tomas-M commented Feb 15, 2018

By the way, if my identification of the problem was correct, there may be several more bugs regarding the number of accepted parameters in he structure defined in rpcclient.cpp.

For example z_listreceivedbyaddress accepts 2 parameters (address and minconf), but rpcclient.cpp has
{ "z_listreceivedbyaddress", 1}. So you may consider reviewing all of the numbers to see if all newly added parameters are accounted for.

@str4d
Copy link
Contributor

str4d commented Mar 20, 2018

The entries in rpcclient.cpp (which are zero-indexed) define whether a parameter is parsed as a JSON value, or left as a string. In the case of z_listreceivedbyaddress, the first parameter is a string, while the second is an integer, so there is only the line { "z_listreceivedbyaddress", 1}.

Your later diagnosis here looks like the actual bug. Thanks for catching this!

@Tomas-M
Copy link
Contributor Author

Tomas-M commented Mar 21, 2018

Thanks for clarification. I still do not fully understand, why there are things like

{ "listtransactions", 1 },
{ "listtransactions", 2 },
{ "listtransactions", 3 },

then. But no need to explain this to me, just make sure you know it :)

@str4d str4d added this to the v1.1.0 milestone Mar 30, 2018
str4d added a commit to str4d/zcash that referenced this issue Apr 3, 2018
zkbot added a commit that referenced this issue Apr 3, 2018
Fix z_importviewingkey startHeight parameter

Closes #2937.

Co-authored-by: Tomas M <tomas@slax.org>
@zkbot zkbot added this to Complete in Continuous Improvement via automation Apr 3, 2018
User Support automation moved this from Blocked to Complete Apr 3, 2018
Asherda pushed a commit to Asherda/VerusCoin that referenced this issue Oct 12, 2022
Closes zcash#2937.

# Conflicts:
#	src/wallet/rpcdump.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc-interface Area: RPC interface C-bug Category: This is a bug
Projects
User Support
  
Complete
Development

Successfully merging a pull request may close this issue.

3 participants