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

Validate consensus rules on root hash and change data modeling. #881

Closed
4 tasks done
hdevalence opened this issue Aug 11, 2020 · 4 comments · Fixed by #1978
Closed
4 tasks done

Validate consensus rules on root hash and change data modeling. #881

hdevalence opened this issue Aug 11, 2020 · 4 comments · Fixed by #1978
Assignees
Labels
A-consensus Area: Consensus rule updates C-bug Category: This is a bug E-help-wanted Call for participation: Help is requested to fix this issue. good first issue NU-1 Sapling Network Upgrade: Sapling specific tasks NU-2 Blossom Network Upgrade: Blossom specific tasks NU-3 Heartwood Network Upgrade: Heartwood specific tasks NU-4 Canopy Network Upgrade: Canopy specific tasks NU-5 Network Upgrade: NU5 specific tasks
Milestone

Comments

@hdevalence
Copy link
Contributor

hdevalence commented Aug 11, 2020

Tasks

There are various consensus rules noted in the comments of the RootHash enum that we never actually check. To check these, I think we should:

  • Change Block::root_hash to return a Result rather than an Option and enforce the all-zeros rules;
  • Change PreSaplingReserved([u8; 32]) to PreSaplingReserved;
  • Change ChainHistoryActivationReserved([u8; 32]) to ChainHistoryActivationReserved;
  • Update the function comments to describe when each variant is verified.

These changes might be impacted by the chain history implementation in #1567.

Variants

Previous PR was #856

@hdevalence hdevalence added A-consensus Area: Consensus rule updates C-bug Category: This is a bug E-easy good first issue labels Aug 11, 2020
@teor2345
Copy link
Contributor

Let's also make sure that we document where each enum variant is validated - while the reserved variants are valid as soon as block::light_client_root_hash returns Ok(), the other variants will need further checks.

(In particular, the Sapling variant needs to be checked against the transactions in the block, and the Heartwood variant needs to be checked against the chain history.)

@vramana
Copy link
Contributor

vramana commented Sep 13, 2020

  • Update the function comments to describe when each variant is verified.

Where are these function comments? I couldn't find them.

@teor2345 teor2345 changed the title Validate consensus rules on light client root hash and change data modeling. Validate consensus rules on root hash and change data modeling. Sep 16, 2020
@teor2345
Copy link
Contributor

  • Update the function comments to describe when each variant is verified.

Where are these function comments? I couldn't find them.

We changed the name of the type to RootHash and the function to root_hash, but forgot to update this ticket:

I've updated the ticket, and I'll update the code comments with a quick PR.

@teor2345 teor2345 added this to the Block Validation milestone Sep 29, 2020
@mpguerra mpguerra removed this from the Block Validation milestone Jan 5, 2021
@mpguerra mpguerra added NU-1 Sapling Network Upgrade: Sapling specific tasks NU-2 Blossom Network Upgrade: Blossom specific tasks NU-3 Heartwood Network Upgrade: Heartwood specific tasks NU-4 Canopy Network Upgrade: Canopy specific tasks NU-5 Network Upgrade: NU5 specific tasks labels Jan 15, 2021
@teor2345 teor2345 mentioned this issue Mar 2, 2021
13 tasks
@mpguerra mpguerra added the E-help-wanted Call for participation: Help is requested to fix this issue. label Mar 16, 2021
@mpguerra mpguerra added this to the 2021 Sprint 6 milestone Mar 17, 2021
@mpguerra mpguerra removed the E-easy label Mar 23, 2021
@teor2345 teor2345 self-assigned this Mar 29, 2021
@teor2345
Copy link
Contributor

Just adding this to an epic

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 C-bug Category: This is a bug E-help-wanted Call for participation: Help is requested to fix this issue. good first issue NU-1 Sapling Network Upgrade: Sapling specific tasks NU-2 Blossom Network Upgrade: Blossom specific tasks NU-3 Heartwood Network Upgrade: Heartwood specific tasks NU-4 Canopy Network Upgrade: Canopy specific tasks NU-5 Network Upgrade: NU5 specific tasks
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants