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

Fix minimum difficulty bugs #1256

Merged
merged 7 commits into from
Nov 12, 2020

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Nov 5, 2020

Motivation

There are a bunch of bugs in Zebra's difficulty implementation, and the Zcash difficulty specifications also have some bugs.

Solution

New changes:

  • Revise the testnet minimum difficulty code based on zcashd's implementation Testnet min-difficulty rules are incorrectly specified zcash/zips#416
    • ZIPs 205 and 208 are wrong
    • the min-difficulty rules change difficulty adjustment, not the difficulty filter
    • all blocks pass the default difficulty filter
  • Turn expect("minimum difficulty activation is checked earlier in the test") into an Err if the rule is not active (and so the function returns None)

Tests for spec errors:

Reviewed changes:

The code in this pull request has:

  • Documentation Comments
  • Unit Tests and Property Tests

Review

@yaahc has reviewed the Difficulty Contextual Validation RFC, which describes these checks.

Related Issues

Difficulty Contextual Validation RFC PR #1246
Difficulty tracking issue #802

Fixes bugs in:

Follow Up Work

This work is listed in the difficulty tracking issue #802

@teor2345 teor2345 added C-bug Category: This is a bug A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code labels Nov 5, 2020
@teor2345 teor2345 added this to the First Alpha Release milestone Nov 5, 2020
@teor2345 teor2345 self-assigned this Nov 5, 2020
@teor2345 teor2345 added the S-blocked Status: Blocked on other tasks label Nov 6, 2020
@teor2345

This comment has been minimized.

@dconnolly dconnolly requested a review from yaahc November 9, 2020 21:40
@teor2345 teor2345 removed the request for review from yaahc November 10, 2020 10:03
@teor2345 teor2345 removed the S-blocked Status: Blocked on other tasks label Nov 10, 2020
@teor2345
Copy link
Contributor Author

teor2345 commented Nov 10, 2020

Unblocked by Daira, who said that blocks 299188, 299189, and 299190 are minimum-difficulty blocks.
That's enough info to move forward with this PR, and get it out of a draft state.

I also un-assigned yaahc as the reviewer, because this PR is not ready for review yet. I'm not sure if it actually fixes the bug.

@teor2345 teor2345 marked this pull request as ready for review November 10, 2020 15:29
@teor2345 teor2345 requested a review from yaahc November 10, 2020 15:29
@teor2345 teor2345 changed the title WIP: Stop rejecting testnet minimum difficulty blocks Stop rejecting testnet minimum difficulty blocks Nov 10, 2020
@teor2345 teor2345 changed the title Stop rejecting testnet minimum difficulty blocks Fix minimum difficulty bugs Nov 11, 2020
@teor2345
Copy link
Contributor Author

@yaahc after you approve this PR, I'd like to squash and rebase it myself, so we can keep some of the changes to different modules in different commits.

yaahc
yaahc previously approved these changes Nov 11, 2020
yaahc
yaahc previously approved these changes Nov 11, 2020
And add the network to the difficulty filter error.
And add a method for the minimum difficulty time gap threshold.
`zcashd` converts the PoWLimit into a compact representation before
using it to perform difficulty filter checks.

The Zcash specification converts to compact for the default difficulty
filter, but not for testnet minimum difficulty blocks. (ZIP 205 and
ZIP 208 don't specify this conversion either.) See ZcashFoundation#1277.
We need spandoc 0.2.1 to avoid a panic on empty blocks.
@teor2345 teor2345 merged commit 18bd5e5 into ZcashFoundation:main Nov 12, 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-bug Category: This is a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants