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

Light Client Root Hash for Block #856

Merged
merged 4 commits into from
Aug 10, 2020

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Aug 7, 2020

Parse the LightClientRootHash using block.light_client_root_hash(network).

@teor2345 teor2345 added C-design Category: Software design work E-med A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement labels Aug 7, 2020
@teor2345 teor2345 requested a review from hdevalence August 7, 2020 10:02
@teor2345 teor2345 self-assigned this Aug 7, 2020
@teor2345 teor2345 mentioned this pull request Aug 7, 2020
8 tasks
@teor2345
Copy link
Contributor Author

teor2345 commented Aug 7, 2020

See #768 for a previous attempt at this change, and the associated discussion.

@teor2345 teor2345 added the S-blocked Status: Blocked on other tasks label Aug 7, 2020
@teor2345
Copy link
Contributor Author

teor2345 commented Aug 7, 2020

Blocked by data dependency problems, which need design changes or refactoring.

@teor2345 teor2345 mentioned this pull request Aug 7, 2020
16 tasks
@hdevalence
Copy link
Contributor

@teor2345 pointed out that we need not just the block height but also the network in order to interpret the field, because the block height at which the interpretation changes depends on the network, and this doesn't fit with the ZcashDeserialize trait. So I guess we cannot fully parse the field upfront. The next best option seems like a variant of @str4d's suggestion on Discord to have a method that takes a Network and returns the enum representation (I don't think it's necessary to pass the block height, since the block height is included in the block already). Since the block height is optional I guess it will have to return an option of an enum.

* use the right variant in LightClientRootHash::from_bytes()
* make block.header.light_client_root_hash pub(super)
* add tests for LightClientRootHash and block.light_client_root_hash
@teor2345 teor2345 added NU-3 Heartwood Network Upgrade: Heartwood specific tasks NU-1 Sapling Network Upgrade: Sapling specific tasks Poll::Ready and removed S-blocked Status: Blocked on other tasks labels Aug 10, 2020
@teor2345 teor2345 marked this pull request as ready for review August 10, 2020 00:47
@teor2345 teor2345 requested a review from a team August 10, 2020 00:48
@teor2345 teor2345 changed the title Help Needed: Light Client Root Hash Light Client Root Hash for Block Aug 10, 2020
@teor2345
Copy link
Contributor Author

The next best option seems like a variant of @str4d's suggestion on Discord to have a method that takes a Network and returns the enum representation (I don't think it's necessary to pass the block height, since the block height is included in the block already). Since the block height is optional I guess it will have to return an option of an enum.

I implemented this design as block.light_client_root_hash(network).

Copy link
Contributor

@hdevalence hdevalence left a comment

Choose a reason for hiding this comment

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

LGTM!

@teor2345 teor2345 merged commit 7afd76f into ZcashFoundation:main Aug 10, 2020
@teor2345 teor2345 mentioned this pull request Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-design Category: Software design work C-enhancement Category: This is an improvement NU-1 Sapling Network Upgrade: Sapling specific tasks NU-3 Heartwood Network Upgrade: Heartwood specific tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants