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

Implicit conversion of the minimum difficulty threshold to the compact representation #1277

Closed
teor2345 opened this issue Nov 10, 2020 · 4 comments
Assignees
Labels
A-docs Area: Documentation C-bug Category: This is a bug NU-1 Sapling Network Upgrade: Sapling specific tasks NU-2 Blossom Network Upgrade: Blossom specific tasks S-needs-spec-update Status: Not in the Zcash spec, but it should be

Comments

@teor2345
Copy link
Contributor

Issue

zcashd converts the PoWLimit to the compact representation before using it to perform the alternate difficulty check for testnet minimum difficulty blocks.

But this conversion is not explicitly specified in the Zcash Specification, ZIP-205, or ZIP-208.

This means that a strict reading of the specification might allow some block hashes that zcashd rejects.

Details

zcashd implements the proof of work limit as a compact value, truncating some of its least significant bits:

unsigned int GetNextWorkRequired(const CBlockIndex* pindexLast, const CBlockHeader *pblock, const Consensus::Params& params)
{
    unsigned int nProofOfWorkLimit = UintToArith256(params.powLimit).GetCompact();

https://github.com/zcash/zcash/blob/514d86817990a1bf9578ee5fa2d01c34c6ca6035/src/pow.cpp#L18

But the Zcash specification says:

PoWLimit :=
    2^243 − 1 for Mainnet
    2^251 − 1 for Testnet

https://zips.z.cash/protocol/protocol.pdf#constants

And ZIP-205 says:

If the block time of a block from this height onward is at least 15 minutes after that of the preceding block, then the block is a minimum-difficulty block, and its target threshold is set to the value of PoWLimit for testnet ...
https://zips.z.cash/zip-0205#change-to-difficulty-adjustment-on-testnet

And ZIP-208 says:

... if the block time of a block at height height ≥ 299188 is at least 6 · PoWTargetSpacing(height) seconds after that of the preceding block, then the block is a minimum-difficulty block, and its target threshold is set to the value of PoWLimit for testnet ...

https://zips.z.cash/zip-0208#minimum-difficulty-blocks-on-the-test-network

Suggested Resolution

@dconnolly or @daira, I'm not sure which of the following updates you'll want to make:

  • update the Zcash Specification so the PoWLimit constants are always converted to the compact representation
  • update the Zcash Specification so the difficulty filter specifies the precise comparison for testnet minimum difficulty blocks
  • update ZIP 205 and ZIP 208 to convert the PoWLimit constant to the compact representation before comparison
@teor2345 teor2345 added C-bug Category: This is a bug A-docs Area: Documentation NU-1 Sapling Network Upgrade: Sapling specific tasks S-needs-spec-update Status: Not in the Zcash spec, but it should be NU-2 Blossom Network Upgrade: Blossom specific tasks labels Nov 10, 2020
@teor2345 teor2345 added this to the Block Validation milestone Nov 10, 2020
teor2345 added a commit to teor2345/zebra that referenced this issue Nov 10, 2020
`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.
@dconnolly dconnolly self-assigned this Nov 11, 2020
@daira
Copy link
Contributor

daira commented Nov 11, 2020

Thanks for reporting this. Please review zcash/zips#417 :-)

teor2345 added a commit to teor2345/zebra that referenced this issue Nov 12, 2020
`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.
teor2345 added a commit that referenced this issue Nov 12, 2020
`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 #1277.
teor2345 added a commit to teor2345/zebra that referenced this issue Nov 12, 2020
Update the design based on the spec bugs in ZcashFoundation#1276, ZcashFoundation#1277, and
zcash/zips#416.

These changes make the difficulty filter into a context-free check,
so we remove it from this contextual validation RFC.
@daira
Copy link
Contributor

daira commented Nov 13, 2020

Fixed in zcash/zips#417.

@mpguerra
Copy link
Contributor

@teor2345 anything else to be done here?

@teor2345
Copy link
Contributor Author

It's fixed in the ZIPs.

teor2345 added a commit that referenced this issue Nov 26, 2020
* Difficulty Contextual RFC: Introduction
Add a header, summary, and motivation

* Difficulty RFC: Add draft definitions
And update the state RFC definitions to match.

* Difficulty RFC: Add relevant chain
* Difficulty RFC: draft guide-level explanation
Outline the core calculations and checks.

* Difficulty RFC: Revised based on spec fixes
Update the design based on the spec bugs in #1276, #1277, and
zcash/zips#416.

These changes make the difficulty filter into a context-free check,
so we remove it from this contextual validation RFC.

* Difficulty RFC: Explain how Zebra's calculations can match the spec
* Difficulty RFC: write most of the reference section
Includes most of the implementation, modules for each function, and
draft notes for some of the remaining parts of the RFC.

* Difficulty RFC: Add an AdjustedDifficulty struct
* Difficulty RFC: Summarise module structure in the one place
* Difficulty RFC: Create implementation notes subsections
* Difficulty RFC: add consensus critical order of operations
* Difficulty RFC: Use the ValidateContextError type
* Difficulty RFC: make the median_time arg mut owned

We have to clone the data to pass a fixed-length array to a function,
so we might as well sort that array to find the median, and avoid a
copy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation C-bug Category: This is a bug NU-1 Sapling Network Upgrade: Sapling specific tasks NU-2 Blossom Network Upgrade: Blossom specific tasks S-needs-spec-update Status: Not in the Zcash spec, but it should be
Projects
None yet
Development

No branches or pull requests

4 participants