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

Jump to bottom

Closes #2186. RPC getblock now accepts height or hash. #2187

Merged
merged 1 commit into from Mar 24, 2017
Merged

Closes #2186. RPC getblock now accepts height or hash. #2187

merged 1 commit into from Mar 24, 2017

Conversation

bitcartel
Copy link
Contributor

No description provided.

@bitcartel bitcartel added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 16, 2017
@bitcartel bitcartel added this to the 1.0.8 milestone Mar 16, 2017
@bitcartel bitcartel force-pushed the 1.0.7_getblock_by_height branch 2 times, most recently from 27ea5f0 to 83f2540 Compare March 16, 2017 18:35
@bitcartel bitcartel added the A-rpc-interface Area: RPC interface label Mar 16, 2017
daira
daira approved these changes Mar 19, 2017
View reviewed changes
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

Spacing nit, and a suggested extension (could go either in this or another PR).

src/rpcblockchain.cpp Outdated
}
catch (const std::exception &e) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid block height parameter");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Inconsistent indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indentation fixed via force push

Copy link
Contributor

Choose a reason for hiding this comment

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

I still see an indentation problem - lines 416-419 start with a tab instead of eight spaces.

src/rpcblockchain.cpp
"\nArguments:\n"
"1. \"hash\" (string, required) The block hash\n"
"1. \"hash|height\" (string, required) The block hash or 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.

It might be useful to also allow a height relative to the chain tip. Maybe tip-<num>, if there isn't already a syntax for this in another RPC call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #2197

@str4d
Copy link
Contributor

str4d commented Mar 21, 2017

Note that this is a specific deviation from Bitcoin Core's RPC policy, which is to have each RPC call perform some unique atomic process (hence createrawtransaction, fundrawtransaction, signrawtransaction etc.). The functionality in this PR can be equally handled by nesting existing commands: zcash-cli getblock $(zcash-cli getblockhash 12345)

I'm not suggesting this PR is a bad idea; I'm just pointing out that we should be aware of the direction we are taking the RPC by doing so. Multiplexing in this way also means that the height must be represented as a JSON string rather than a JSON integer, which may affect some use cases (see e.g. #2052 where the boolean field is now a string, and in Python must be handled as such).

@bitcartel
Copy link
Contributor Author

Well, the RPC is still atomic i.e. retrieving a block from storage. The only thing that has changed here is the key format being used to find a block. Btw, I'm not sure sure if upstream has a strict policy about this, otherwise sendmany wouldn't exist... maybe only enforced for new API calls?

I agree that having to convert from string to int requires a bit of extra legwork. The benefits though are improved usability for end users and reduction of network traffic and load on the server - one rpc call instead of two.

@bitcartel bitcartel requested a review from str4d March 21, 2017 21:27
@bitcartel bitcartel mentioned this pull request Mar 22, 2017
Closed
str4d
str4d requested changes Mar 23, 2017
View reviewed changes
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 pending resolution of comments.

src/rpcblockchain.cpp Outdated
}
catch (const std::exception &e) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid block height parameter");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I still see an indentation problem - lines 416-419 start with a tab instead of eight spaces.

src/rpcblockchain.cpp Outdated
}

if (nHeight < 0 || nHeight > chainActive.Height()) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Block height out of range");
Copy link
Contributor

Choose a reason for hiding this comment

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

Tab instead of spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed vim gremlins.

src/rpcblockchain.cpp Outdated
);

LOCK(cs_main);

std::string strHash = params[0].get_str();

// If height is supplied, find the hash
if (strHash.size() < 64) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking: is there a constant somewhere we can reference here instead of a hard-coded length? Or is there a precedent in the codebase for assuming txid hashes are 64 chars long? It looks like uint256S() reads up to 64 characters from the end of the given string, so I guess it's fair enough to specify 64 here given we directly reference uint256 shortly after.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, now references uint256.

@bitcartel
Copy link
Contributor Author

@str4d Comments addressed. Please update to green tick.

@str4d str4d mentioned this pull request Mar 24, 2017
Merged
@bitcartel
Copy link
Contributor Author

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Mar 24, 2017

📌 Commit 7d3b152 has been approved by bitcartel

@zkbot
Copy link
Contributor

zkbot commented Mar 24, 2017

⌛ Testing commit 7d3b152 with merge 23f792c...

zkbot added a commit that referenced this pull request Mar 24, 2017
@zkbot
Auto merge of #2187 - bitcartel:1.0.7_getblock_by_height, r=bitcartel
23f792c 
Closes #2186. RPC getblock now accepts height or hash.
@zkbot
Copy link
Contributor

zkbot commented Mar 24, 2017

☀️ Test successful - zcash

@zkbot zkbot merged commit 7d3b152 into zcash:master Mar 24, 2017
@nathan-at-least nathan-at-least added this to In Progress in Continuous Improvement Apr 20, 2017
@str4d str4d moved this from In Progress to Awaiting Review in Continuous Improvement Apr 25, 2017
@str4d str4d moved this from Awaiting Review to Complete in Continuous Improvement Apr 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daira daira daira approved these changes

@str4d str4d str4d approved these changes

Assignees
No one assigned
Labels
A-rpc-interface Area: RPC interface S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. usability
Projects
No open projects
Continuous Improvement
  
Complete
Milestone
1.0.8
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bitcartel @str4d @zkbot @daira