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

Off-by-one error in testnet minimum difficulty block gap specification #1276

Closed
teor2345 opened this issue Nov 10, 2020 · 5 comments
Closed
Assignees
Labels
A-docs Area: Documentation 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

Comments

@teor2345
Copy link
Contributor

teor2345 commented Nov 10, 2020

Issue

The minimum difficulty block gap check is ambiguous in ZIP-205 and ZIP-208.

zcashd uses a strictly greater than check.

Details

zcashd implements the minimum difficulty block gap using a strictly greater than test:

            // If the new block's timestamp is more than 6 * block interval minutes
            // then allow mining of a min-difficulty block.
            if (pblock && pblock->GetBlockTime() > pindexLast->GetBlockTime() + params.PoWTargetSpacing(pindexLast->nHeight + 1) * 6)
                return nProofOfWorkLimit;

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

But ZIP-205 specifies greater than or equal to:

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...
https://zips.z.cash/zip-0205#change-to-difficulty-adjustment-on-testnet

And ZIP-208 is ambiguous:

... the difficulty adjustment algorithm allows minimum-difficulty blocks, as described in 8, when the block time exceeds a given threshold. ...

That is, 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...

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

Suggested Resolution

@dconnolly or @daira, can you update ZIP-205 and ZIP-208 to consistently say exceeds or greater than?

@teor2345 teor2345 added 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 teor2345 added this to To Do in 🦓 via automation Nov 10, 2020
@dconnolly dconnolly self-assigned this Nov 10, 2020
daira added a commit to zcash/zips that referenced this issue Nov 10, 2020
…ks matches zcashd.

Fixes ZcashFoundation/zebra#1276 .

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
@dconnolly
Copy link
Contributor

Done 🙏
zcash/zips@806076c

🦓 automation moved this from To Do to Done Nov 10, 2020
@teor2345 teor2345 reopened this Nov 11, 2020
🦓 automation moved this from Done to In progress Nov 11, 2020
@teor2345
Copy link
Contributor Author

We missed a greater than in this sentence in ZIP 208:

That is, if the block time of a block at height *height* ≥ 299188 is at least

zcash/zips@806076c#diff-3f875a100a287c2b47ce389870bdde636f2720429dc19a74d1b42b4e08e31962R187

@daira
Copy link
Contributor

daira commented Nov 12, 2020

Thanks, I'll include this in zcash/zips#417.

daira added a commit to zcash/zips that referenced this issue Nov 12, 2020
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
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 Can we close this one now?

🦓 automation moved this from In progress to Done Nov 24, 2020
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 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
No open projects
🦓
  
Done
Development

No branches or pull requests

4 participants