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

Return hashes from getlatestblock #361

Merged
merged 2 commits into from
Jul 27, 2021

Conversation

adityapk00
Copy link
Contributor

Return the Hash (along with the Height) for the getLatestBlock RPC

@adityapk00
Copy link
Contributor Author

cc: @LarryRuane

@LarryRuane
Copy link
Collaborator

LarryRuane commented Jul 27, 2021

Concept ACK, let me do a little testing, and then I would like to merge this, thanks.

@LarryRuane
Copy link
Collaborator

Added a unit test. (Also there was something missing from .gitignore, fixed even though not related to this PR.)

@LarryRuane LarryRuane merged commit 3f669c3 into zcash:master Jul 27, 2021
@LarryRuane
Copy link
Collaborator

After this PR merged, @gmale had some concern that increasing the byte length of the reply, as this PR does, would significantly increase the overall bandwidth (since this gRPC is executed every few seconds, I think). It may have been better to add a new gRPC that does what thisGetLatestBlock does now with this PR, and leave GetLatestBlock unchanged (so that it returns only the height). But the current thinking is that the way GetLatestBlock is now (with this PR) is probably okay, so we'll leave it as is, and we can always revisit later.

One idea I proposed is to return a shortened hash (low-order 4 bytes only) because all the hash is used for is to detect a reorg by comparing it to the previous hash we received. This would mean there's a one in 4 billion chance a reorg wouldn't be detected, but even in this case, it would eventually (when the next block arrives).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants