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

Add getdeprecationinfo RPC method to return deprecation block height #2839

Merged
merged 2 commits into from Feb 2, 2018

Conversation

arcalinea
Copy link
Contributor

Closes #2828

Returns:

{
  "version": xxxxx,                      (numeric) the server version
  "subversion": "/MagicBean:x.y.z[-v]/",     (string) the server subversion string
  "deprecationheight": xxxxx,            (numeric) the deprecation block height
}

@arcalinea arcalinea added A-rpc-interface Area: RPC interface usability labels Dec 28, 2017
@arcalinea arcalinea added this to the 1.0.15 milestone Dec 28, 2017
@arcalinea arcalinea self-assigned this Dec 28, 2017
@arcalinea arcalinea changed the title Add getdeprecationinfo rpc method to return deprecation block height Add getdeprecationinfo RPC method to return deprecation block height Dec 28, 2017
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General utACK, but see comments.

src/rpcnet.cpp Outdated
"\nResult:\n"
"{\n"
" \"version\": xxxxx, (numeric) the server version\n"
" \"subversion\": \"/MagicBean:x.y.z[-v]/\", (string) the server subversion string\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the naming of this irritating (it's an extended representation of the version), but it matches existing Bitcoin-inherited RPCs and API designs, so leave it as-is.

src/rpcnet.cpp Outdated
"{\n"
" \"version\": xxxxx, (numeric) the server version\n"
" \"subversion\": \"/MagicBean:x.y.z[-v]/\", (string) the server subversion string\n"
" \"deprecationheight\": xxxxx, (numeric) the deprecation block height\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the block height at which this version will deprecate and shut down (unless -disabledeprecation is set)

(modulo line lengths - look at existing RPC help messages for comparison)

@@ -398,6 +399,34 @@ static UniValue GetNetworksInfo()
return networks;
}

UniValue getdeprecationinfo(const UniValue& params, bool fHelp)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... would this make more sense in rpcmisc.cpp? If I had to pick a specific category then I would put it in net, but I think it's closer to upstream's getmemoryinfo, in that it is about the local node. I'm not fussed about bikeshedding this though.

Copy link
Contributor Author

@arcalinea arcalinea Jan 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put it in rpcnet.cpp because it also returns version info, duplicating some of what getnetworkinfo does, so the relevant constants are already imported.

src/rpcnet.cpp Outdated
+ HelpExampleRpc("getdeprecationinfo", "")
);

LOCK(cs_main);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to take the lock, as this function only depends on hard-coded constants.

@str4d str4d added this to 1.0.15 UX in Release planning Jan 2, 2018
@arcalinea
Copy link
Contributor Author

Addressed @str4d's comments

@arcalinea arcalinea requested a review from str4d January 15, 2018 22:55
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

src/rpcnet.cpp Outdated


UniValue obj(UniValue::VOBJ);
obj.push_back(Pair("version", CLIENT_VERSION));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove indentation, since it doesn't align with anything else.

Copy link
Contributor

@bitcartel bitcartel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is good... but when running in testnet mode I noticed it was returning me the mainnet deprecation height. Turns out to be an issue with auto-senescence in general, so I filed #2876. In the meantime, getdeprecationinfo could be made only applicable for mainnet, so consider: (1) updating help message (2) check to see if current network is mainnet or not, and if not, return error message or help message.

@arcalinea
Copy link
Contributor Author

Huh, good to know. Ok, will update help message and check for mainnet before showing.

Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment.

src/rpcnet.cpp Outdated
UniValue getdeprecationinfo(const UniValue& params, bool fHelp)
{
const CChainParams& chainparams = Params();
if (fHelp || params.size() != 0 || chainparams.NetworkIDString() != "mainnet")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Params().NetworkIDString() != "main"

@arcalinea arcalinea requested a review from str4d January 23, 2018 00:57
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. Please merge squash the third commit into the second one.

@arcalinea
Copy link
Contributor Author

Squashed

@bitcartel
Copy link
Contributor

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Feb 2, 2018

📌 Commit df46562 has been approved by bitcartel

@zkbot
Copy link
Contributor

zkbot commented Feb 2, 2018

⌛ Testing commit df46562 with merge a0a010c...

zkbot added a commit that referenced this pull request Feb 2, 2018
Add getdeprecationinfo RPC method to return deprecation block height

Closes #2828

Returns:
```
{
  "version": xxxxx,                      (numeric) the server version
  "subversion": "/MagicBean:x.y.z[-v]/",     (string) the server subversion string
  "deprecationheight": xxxxx,            (numeric) the deprecation block height
}
```
@zkbot
Copy link
Contributor

zkbot commented Feb 2, 2018

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing a0a010c to master...

@zkbot zkbot merged commit df46562 into zcash:master Feb 2, 2018
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 usability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants