-
Notifications
You must be signed in to change notification settings - Fork 106
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
change(rpc): Update getblock
RPC to more closely match zcashd
#9006
base: main
Are you sure you want to change the base?
Conversation
e3b050b
to
618beb3
Compare
getblock
RPC to more closely match zcashd
This is looking good! I left some suggestions in #9008.
The new behaviour seems performant enough for lightwalletd to continue using The performance issue before (fixed in #5307) was that it was reading all of the block's transactions, the database queries added here should be quick, and, if they still have a measurable impact on lightwalletd sync performance, can be optimized easily (like by parallelizing or unifying state requests and maybe using an iterator to get the block hashes at (Though we should make sure to cache block sizes by hash in a new column family when adding that field to avoid reading, deserializing, and then serializing the entire block to get its size.) |
…ardroot field, removes unnecessary state request. (#9008)
de1b3a6
to
1d848d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good, i made a few comments and documentation suggestions.
zebra-rpc/src/methods.rs
Outdated
@@ -154,16 +157,19 @@ pub trait Rpc { | |||
/// # Parameters | |||
/// | |||
/// - `hash_or_height`: (string, required, example="1") The hash or height for the block to be returned. | |||
/// - `verbosity`: (number, optional, default=1, example=1) 0 for hex encoded data, 1 for a json object, and 2 for json object with transaction data. | |||
/// - `verbosity`: (number, optional, default=1, example=1) 0 for hex encoded data, 1 for a json object, and 2 for json object with transaction data, and 3 for a partially filled json object (which is faster and useful for lightwalletd-only usage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused here. I only see the verbosity cases 0, 1 and 2 in the method code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I introduced a new verbosity but Arya pointed out that wasn't needed, and I forgot to update this. Thanks! Fixed in 1013611
zebra-rpc/src/methods.rs
Outdated
solution: Some(solution), | ||
bits: Some(bits), | ||
difficulty: Some(difficulty), | ||
// TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain more what are the todo here and in the next ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I removed the TODOs in 1013611 to make it less confusing (I forgot to clean them up) but they are:
- Supporting returning detailed transaction objects with verbosity=2 (that will be getblock: return transactions details with
verbosity=2
#9024) - Actually computing and returning the size of the block (that will be getblock: return the
size
field #9020)
Co-authored-by: Alfredo Garcia <[email protected]>
Co-authored-by: Arya <[email protected]>
Motivation
Currently zebra has partial support for
getblock()
and it mostly targets lightwalletd usage. For zcashd deprecation we want it to match zcashd more closely.Specifications & References
zcashd RPC docs
Solution
Based on #8967
Since
getblock()
returns basically a superset ofgetblockheader()
, I change ourgetblock()
to callgetblockheader()
for all the shared fields.Work on returning the additional non-header fields will be done in separate PRs.
I also changed
getblockheader()
return codes since whatever error it returns is now returned bygetblock()
, and we have committed to error codes in our snapshots. I also added the -5 error code which can also be returned by zcashd.Since the previous behaviour can be still useful for performance reasons, I moved it to be carried out when verbosity=3 is used. It would be a simple change to
lightwalletd
. I'm not sure about the specific number though, should we use something farther off?This also adds
ZEBRAD_EXTRA_ARGS
tozcash-rpc-diff
because I was stubborn and wanted to make it work with cookie authentication 😆 Let me know if you'd prefer it in a separate PR, but it seems simple enough.Tests
I adjusted the existing
vectors.rs
tests and updated the snapshots. I also tested it manually and withzcash-rpc-diff
; the current diff is thatsize, blockcommitments authdataroot, chainhistoryroot, chainwork anchor, chainSupply and valuePools
are not yet being filled; and verbosity=2 is still the same as 1 (i.e.tx
is not expanded)Follow-up Work
Additional information to put in Changelog
PR Author's Checklist
PR Reviewer's Checklist