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

Add Genesis NU, Update Canopy NU Testnet Height #765

Merged
merged 2 commits into from
Jul 27, 2020

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jul 27, 2020

  • Add a Genesis network upgrade, to handle genesis-specific consensus rules
  • Add Testnet activation height for the Canopy network upgrade

These changes modify the same table, so I put them in the same PR.

Removed:

  • Add a TestnetMinimumDifficulty network upgrade, to handle testnet-specific difficulty rule changes
    • Actually, this is a bad design, because it confuses network upgrades, and network-specific consensus rule changes. These changes need their own table and functions.

teor2345 added 2 commits July 27, 2020 17:54
We can use this network upgrade to implement different consensus rules
and chain context handling for genesis blocks.

Part of the chain state design in ZcashFoundation#682.
@teor2345 teor2345 added Poll::Ready A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code labels Jul 27, 2020
@teor2345 teor2345 requested a review from a team July 27, 2020 08:01
@teor2345 teor2345 self-assigned this Jul 27, 2020
oxarbitrage
oxarbitrage previously approved these changes Jul 27, 2020
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Looks good.

@dconnolly dconnolly merged commit 2b1e716 into ZcashFoundation:main Jul 27, 2020
@yaahc yaahc mentioned this pull request Jul 27, 2020
18 tasks
///
/// Zcash genesis blocks use a different set of consensus rules from
/// other BeforeOverwinter blocks, so we treat them like a separate network
/// upgrade.
Copy link
Contributor

@daira daira Jul 29, 2020

Choose a reason for hiding this comment

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

I'm skeptical of the complexity trade-off here. There are only a small number of things that differ for the genesis block:

  • the height-in-coinbase was missing (this was just a bug);
  • there is no Founders' Reward output in the coinbase tx;
  • the hashPrevBlock, difficulty adjustment, and median-time-past rules are special-cased.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, good point. I'm not sure how we'll use the NetworkUpgrades enum yet.

But depending on the outcome of the parallel verification RFC in #763, we might have some additional Zebra special cases:

  • there are no VerificationConstraints, because the genesis block is the first block (except, perhaps, that the genesis block is the first block to be committed to the state)
  • There is no previous verification context
  • the on-disk verification state is empty
  • the verification context and on-disk state need to be initialised with empty values
  • the verification context needs to be updated with the values from the genesis block, based on genesis-specific rules
  • there are no chain tips in memory or on disk - the genesis block automatically becomes the first tip

See https://github.com/ZcashFoundation/zebra/blob/330f9b16154543cf17fd8a785b29a70b26b733da/book/src/designs/0002-parallel-verification.md#chain-context-and-network-upgrades

I think a lot of these special cases are covered by checkpoint verification, but we'll definitely need special handling for the on-disk state.

I'll review the need for the Genesis network upgrade after the RFC.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it still makes sense to have a distinct Genesis variant, because (unfortunately) the rules are actually different. Downstream code for whom this distinction doesn't matter can combine arms of a match statement, but not making the distinction at all means that we can't handle the differences in the rules in a principled way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to make sure you'd thought about the complexity tradeoffs — sounds like you have.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants