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

Use fixed genesis coinbase data in generated genesis blocks #2568

Merged
merged 2 commits into from
Aug 4, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Aug 4, 2021

Motivation

Zebra serializes genesis blocks with arbitrary coinbase data, but our deserialization checks for fixed mainnet/testnet coinbase bytes.

This causes failures in some proptests.

This is unscheduled work due to a Zebra bug / spec bug.

Specifications

The current Zcash specification for the genesis coinbase height encoding is incorrect. (zcash/zips#540)

Solution

  • Return an error when trying to serialize a genesis height with non-genesis coinbase data
  • Use fixed genesis coinbase data in generated genesis blocks
  • Document input, coinbase, and height encoding and decoding functions
  • Delete some documentation that is incorrect after recent spec changes

Review

This fix blocks PR #2519, so @jvff can review it.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Follow Up Work

We should get the spec fixed, then implement whatever it says.


This change is Reviewable

This error prevents cryptic errors during genesis coinbase deserialization.

And fix and improve documentation.
This change is required, because genesis transactions do not have a
coinbase height in their coinbase data.
@teor2345 teor2345 added C-bug Category: This is a bug A-docs Area: Documentation A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code NU Genesis Network Upgrade: Genesis block specific tasks P-Medium labels Aug 4, 2021
@teor2345 teor2345 added this to the 2021 Sprint 15 milestone Aug 4, 2021
@teor2345 teor2345 requested a review from jvff August 4, 2021 09:46
@teor2345 teor2345 self-assigned this Aug 4, 2021
Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

:lgtm: Thanks for this! 😄

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @teor2345)

@jvff jvff merged commit 7ad5f1c into main Aug 4, 2021
@jvff jvff deleted the fix-generated-genesis-coinbase-data branch August 4, 2021 11:43
@daira
Copy link
Contributor

daira commented Aug 4, 2021

I don't think there is a spec bug here. The genesis block (at height 0 of any chain) is explicitly excluded from coinbase height checks in section 7.1.2 of the protocol spec:

image

and in section 7.12:

image

@teor2345
Copy link
Contributor Author

teor2345 commented Aug 4, 2021

I don't think there is a spec bug here. The genesis block (at height 0 of any chain) is explicitly excluded from coinbase height checks in section 7.1.2 of the protocol spec

I'm still not sure how implementations are meant to identify genesis blocks, and distinguish them from blocks at height 520,617,983. But I think it's ok if we don't want to specify a consistent algorithm here.

See my detailed comment:
zcash/zips#540 (comment)

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-docs Area: Documentation A-rust Area: Updates to Rust code C-bug Category: This is a bug NU Genesis Network Upgrade: Genesis block specific tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants