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

z_gettreestate JSON-RPC method #3156

Closed
4 tasks
Tracked by #3134
teor2345 opened this issue Dec 6, 2021 · 10 comments · Fixed by #3990
Closed
4 tasks
Tracked by #3134

z_gettreestate JSON-RPC method #3156

teor2345 opened this issue Dec 6, 2021 · 10 comments · Fixed by #3990
Assignees
Labels
A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes lightwalletd any work associated with lightwalletd

Comments

@teor2345
Copy link
Contributor

teor2345 commented Dec 6, 2021

Motivation

lightwalletd uses the z_gettreestate JSON-RPC method.

Dependencies

We need to do these tickets first:

Required Fields

The method is documented here: https://zcash.github.io/rpc/z_gettreestate.html

lightwalletd uses either the height or hash arguments, depending on what it has available internally: https://github.com/zcash/lightwalletd/blob/master/frontend/service.go#L182

lightwalletd only uses positive heights above zero, so Zebra doesn't need to support height = -1:
https://github.com/adityapk00/lightwalletd/blob/c1bab818a683e4de69cd952317000f9bb2932274/frontend/service.go#L340

But only these fields are used by lightwalletd: https://github.com/zcash/lightwalletd/blob/master/common/common.go#L104

Field list:

{
  "hash": "hash",         (string) hex block hash
  "height": n,            (numeric) block height
  "time": n,              (numeric) Unix epoch time when the block was mined, from the block header time field
  "sapling": {
    "commitments": {
      "finalState": "hex" (string, optional) final state of block at `height`, if available
    }
  }
  "orchard": {
    "commitments": {
      "finalState": "hex" (string, optional) final state of block at `height`, if available
    }
  }
}

Notes (see https://github.com/zcash/zcash/blob/bd2c35a93fdc93a06d650da2a90378fbb7083711/src/rpc/blockchain.cpp#L1305-L1328):

  • finalRoot is the root of the note commitment tree after the given block
  • finalState is the encoding of the note commitment tree itself for the given block.
    • This requires encoding the tree the same way that zcashd does (not sure of the additional work required, if any)
  • skipHash should never be supplied, we skip identical tree states internally when storing and loading from the database

Tasks

  • Implement the new ReadRequest for ReadState
  • Implement the RPC method
  • Add RPC acceptance tests to CI
  • Test that the RPC method works with lightwalletd

API Reference

We plan to use jsonrpc_core with:

Example Code

Here are examples of:

Resolved Questions

Will we need to add an orchard field to this RPC? Yes.

@ftm1000 ftm1000 added the S-needs-triage Status: A bug report needs triage label Feb 10, 2022
@ftm1000
Copy link

ftm1000 commented Feb 10, 2022

@upbqdn
Copy link
Member

upbqdn commented Mar 23, 2022

The RPC method z_gettreestate is supposed to return note commitment trees serialized as hex strings. Should I implement zcash_serialize for NoteCommitmentTree?

@teor2345
Copy link
Contributor Author

The RPC method z_gettreestate is supposed to return note commitment trees serialized as hex strings. Should I implement zcash_serialize for NoteCommitmentTree?

Good question, and nice catch!

We'll need to serialize to the zcashd RPC byte format, but I'm not sure what that is. Maybe @conradoplg knows?
Otherwise we'll have to check the zcashd code.

Then we can encode as hex.

Here are our different options for serialization:

ZcashSerialize is for consensus-critical serialization, but we want to do RPC-specific serialization here:

/// Consensus-critical serialization for Zcash.
///
/// This trait provides a generic serialization for consensus-critical
/// formats, such as network messages, transactions, blocks, etc. It is intended
/// for use only in consensus-critical contexts; in other contexts, such as
/// internal storage, it would be preferable to use Serde.
pub trait ZcashSerialize: Sized {

IntoDisk is used to store typed in the database, it uses bincode via serde.
It's probably not the same as the RPC format:

impl IntoDisk for sapling::tree::NoteCommitmentTree {
type Bytes = Vec<u8>;
fn as_bytes(&self) -> Self::Bytes {
bincode::DefaultOptions::new()
.serialize(self)
.expect("serialization to vec doesn't fail")
}
}

Here's how we have done some other RPC serializations:

impl Hash {
/// Return the hash bytes in big-endian byte-order suitable for printing out byte by byte.
///
/// Zebra displays transaction and block hashes in big-endian byte-order,
/// following the u256 convention set by Bitcoin and zcashd.
fn bytes_in_display_order(&self) -> [u8; 32] {
let mut reversed_bytes = self.0;
reversed_bytes.reverse();
reversed_bytes
}
}
impl fmt::Display for Hash {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.write_str(&self.encode_hex::<String>())
}
}
impl fmt::Debug for Hash {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.debug_tuple("block::Hash")
.field(&self.encode_hex::<String>())
.finish()
}
}
impl ToHex for &Hash {
fn encode_hex<T: FromIterator<char>>(&self) -> T {
self.bytes_in_display_order().encode_hex()
}
fn encode_hex_upper<T: FromIterator<char>>(&self) -> T {
self.bytes_in_display_order().encode_hex_upper()
}
}

@conradoplg
Copy link
Collaborator

conradoplg commented Mar 24, 2022

The RPC method z_gettreestate is supposed to return note commitment trees serialized as hex strings. Should I implement zcash_serialize for NoteCommitmentTree?

We're using the incrementalmerkletree crate that zcash also uses, but I'm not 100% sure if they use the exact same format or if there are any differences. I think we'll need to check their codebase to be sure (or just ask them)

@teor2345
Copy link
Contributor Author

teor2345 commented Mar 24, 2022

The RPC method z_gettreestate is supposed to return note commitment trees serialized as hex strings. Should I implement zcash_serialize for NoteCommitmentTree?

We're using the incrementalmerkletree crate that zcash also uses, but I'm not 100% sure if they use the exact same format or if there are any differences. I think we'll need to check their codebase to be sure (or just ask them)

That's the option I forgot: a serde::Serialize implementation.

In the code, that looks like:

#[derive(serde::Serialize)]
/// Response to a `getblock` RPC request.
///
/// See the notes for the [`Rpc::get_block` method].
pub struct GetBlock(#[serde(with = "hex")] SerializedBlock);

And it also needs a ToHex implementation.

@teor2345 teor2345 added A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes labels Mar 24, 2022
@upbqdn
Copy link
Member

upbqdn commented Mar 24, 2022

In order to get ToHex, NoteCommitmentTree has to implement AsRef<[u8]>, so I guess a manual implementation of converting NoteCommitmentTree into Vec<u8> is still needed. I'm checking how zcashd does it.

@upbqdn
Copy link
Member

upbqdn commented Apr 26, 2022

I updated the description of this issue so that the RPC response doesn't contain the block time. I noticed the original docs don't mention it.

@teor2345
Copy link
Contributor Author

I updated the description of this issue so that the RPC response doesn't contain the block time. I noticed the original docs don't mention it.

@upbqdn unfortunately, we need to include time, because lightwalletd expects it:
https://github.com/zcash/lightwalletd/blob/68789356fb1a75f62735a529b38389ef08ea7582/common/common.go#L108

@teor2345
Copy link
Contributor Author

(If you'd like to open a zcashd ticket to fix their docs, go for it!)

@upbqdn
Copy link
Member

upbqdn commented Apr 27, 2022

I opened the issue here zcash/zcash#5913.

@mergify mergify bot closed this as completed in #3990 May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes lightwalletd any work associated with lightwalletd
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants