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

Map out block validation data flow #560

Merged
merged 4 commits into from
Aug 5, 2020
Merged

Map out block validation data flow #560

merged 4 commits into from
Aug 5, 2020

Conversation

dconnolly
Copy link
Contributor

@dconnolly dconnolly commented Jun 30, 2020

Part of #529

@dconnolly
Copy link
Contributor Author

#529 (comment)

@dconnolly
Copy link
Contributor Author

That build failure is very weird:

image

@dconnolly
Copy link
Contributor Author

That build failure is very weird:

image

I wonder if crates.io is having a hard time with new Rust 1.45.0 release?

@dconnolly
Copy link
Contributor Author

What the hell

image

@dconnolly dconnolly marked this pull request as ready for review July 21, 2020 21:22
@dconnolly dconnolly linked an issue Jul 21, 2020 that may be closed by this pull request
@dconnolly dconnolly changed the title Data flow Map out block validation data flow Jul 21, 2020
@dconnolly dconnolly requested a review from a team July 21, 2020 21:24
@dconnolly dconnolly added A-consensus Area: Consensus rule updates C-design Category: Software design work A-docs Area: Documentation Poll::Ready labels Jul 21, 2020
@ZcashFoundation ZcashFoundation deleted a comment from codecov bot Jul 21, 2020
- https://github.com/zcash/zcash/blob/ab2b7c0969391d8a57d90d008665da02f3f618e7/src/pow.cpp#L96
- requires no information except `n`, `k` params
- checks that the proof of work parameters are valid
- requires the current proof of work amount `params.powLimit`
Copy link
Contributor

Choose a reason for hiding this comment

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

Added to the consensus parameters tracking issue #565

- Calls `ContextualCheckBlockHeader`, defined at: https://github.com/zcash/zcash/blob/ab2b7c0969391d8a57d90d008665da02f3f618e7/src/main.cpp#L3900
- Does checks given a pointer to the previous block
- Check Equihash solution is valid
- In our code we compute the equihash solution on the block alone, we will need to also do a step to check that its block height is the appopriate N+1 re: the previous block
Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in PR #748

- Does checks given a pointer to the previous block
- Check Equihash solution is valid
- In our code we compute the equihash solution on the block alone, we will need to also do a step to check that its block height is the appopriate N+1 re: the previous block
- Check proof of work
Copy link
Contributor

Choose a reason for hiding this comment

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

See #572 and PR #799

- https://github.com/zcash/zcash/blob/ab2b7c0969391d8a57d90d008665da02f3f618e7/src/main.cpp#L4040
- Reject block.nVersion < 4 blocks
- Don't accept any forks from the main chain prior to last checkpoint
- We will probably get this 'more naturally' and don't need to explicitly check it like this
Copy link
Contributor

Choose a reason for hiding this comment

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

CheckpointVerifier effectively does this, and when we don't have hard-coded checkpoints, the parallel verification RFC should handle it #763

- Calls `ContextualCheckBlock`, defined at https://github.com/zcash/zcash/blob/ab2b7c0969391d8a57d90d008665da02f3f618e7/src/main.cpp#L4065
- For each transaction:
- Calls `ContextualCheckTransaction`, defined at https://github.com/zcash/zcash/blob/ab2b7c0969391d8a57d90d008665da02f3f618e7/src/main.cpp#L760
- For Zebra, we should only be doing Transaction v4 (Sapling+) checks, because checkpointing
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to keep context from v1-v3 transactions, so we can verify v4 transactions?

For example, UTXOs and revealed nullifiers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, we might have to compute the treestate including nullifier set that is correct at the final checkpoint?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To answer your question, "I'm not sure"

image

- Calls `IsFinalTx`, defined at https://github.com/zcash/zcash/blob/ab2b7c0969391d8a57d90d008665da02f3f618e7/src/main.cpp#L675
- Calls tx::IsFinal(): https://github.com/zcash/zcash/blob/6d9573c66e33b5b742c49ab35e816d74d4ab55b1/src/primitives/transaction.h#L401
- Enforce BIP 34: https://github.com/zcash/zcash/blob/ab2b7c0969391d8a57d90d008665da02f3f618e7/src/main.cpp#L4090
- If blockheight < the block just before the first subsidy halving block, dispense Founder's Reward: https://github.com/zcash/zcash/blob/ab2b7c0969391d8a57d90d008665da02f3f618e7/src/main.cpp#L4104
Copy link
Contributor

Choose a reason for hiding this comment

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

The coinbase, founders reward, and dev fund don't need any verification context, so I've opened ticket #801 for them. I also added the relevant consensus parameters to #565.

- If blockheight < the block just before the first subsidy halving block, dispense Founder's Reward: https://github.com/zcash/zcash/blob/ab2b7c0969391d8a57d90d008665da02f3f618e7/src/main.cpp#L4104
- Everything else is state/index management after good verification
- Calls `CheckBlockIndex`, defined at https://github.com/zcash/zcash/blob/ab2b7c0969391d8a57d90d008665da02f3f618e7/src/main.cpp#L5125
- Calls `ActivateBestChain`, defined at https://github.com/zcash/zcash/blob/ab2b7c0969391d8a57d90d008665da02f3f618e7/src/main.cpp#L3513
Copy link
Contributor

Choose a reason for hiding this comment

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

See the parallel verification RFC #763

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for doing this work!

I had one question about using v1-v3 transaction state to verify v4 transactions.

@yaahc
Copy link
Contributor

yaahc commented Aug 4, 2020

Is this good to merge?

@hdevalence hdevalence merged commit 1f57b3a into main Aug 5, 2020
@hdevalence hdevalence deleted the data-flow branch August 5, 2020 05:44
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 C-design Category: Software design work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map out dataflow dependencies for block verification.
4 participants