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 ZIP 208 (Shorter Block Target Spacing) #237

Merged
merged 12 commits into from May 23, 2019
Merged

Conversation

daira
Copy link
Collaborator

@daira daira commented May 20, 2019

Co-authored-by: Simon Liu simon@z.cash
Co-authored-by: Daira Hopwood daira@z.cash

daira and others added 12 commits May 17, 2019 18:19
Co-authored-by: Simon Liu <simon@z.cash>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
…nces.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Co-authored-by: Jack Grigg <jack@z.cash>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
… 2019.0.0.

Signed-off-by: Daira Hopwood <daira@jacaranda.org>
Signed-off-by: Daira Hopwood <daira@jacaranda.org>
…ght for consistency.

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

@zebambam zebambam left a comment

Choose a reason for hiding this comment

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

I was not able to check the correctness of the FR changes. Presumably it will be obvious to anyone setting the EOS halt on new releases that they should take this change into account, given that we set it manually on each release. We should look up the default expiry delta and set it correctly.

increases more slowly than the decrease in block time, and so, up to a point,
decreasing the block target spacing can provide a better trade-off between
latency and security. This argument assumes that the verification and
propagation time for a block remain small compared to the block target spacing.

Choose a reason for hiding this comment

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

great description

Throughout the specification, rename HalvingInterval to PreBlossomHalvingInterval,
and rename PoWTargetSpacing to PreBlossomTargetSpacing. These constants retain
their values from [#preblossom-protocol]_ of 840000 (blocks) and 150 (seconds)
respectively.

Choose a reason for hiding this comment

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

I think this makes it clear. It seems like there could be a generic way to do this but I have no helpful suggestions about how.


In section 5.3 (Constants), define PostBlossomPoWTargetSpacing := 75 seconds.

TODO: check that PostBlossomPoWTargetSpacing is reasonable.

Choose a reason for hiding this comment

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

I think this was part of the security audit review. Is there more internal review to be done? If so is there already a ticket for it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it was in scope of the audits. Coinspect said:

ZIP-208 - Consider Worst Case Block Verification Time
The time between blocks must not be too close to the worst case of block verification time. The proposed block target spacing is 75 seconds and we believe it could be too close to the worst case verification time.

I think we need to make an estimate of worst-case verification time with parallelization (and with batching if we intend to rely on implementing that for all transactions in a block).

NCC did not say anything about the advisability of choosing 75s, although they went into much more detail than Coinspect about difficulty adjustment and clock skew issues.

The effect on the recommended number of confirmations is covered by #215. The verification time issue is discussed on zcash/zcash#3690 .

Define IsBlossomActivated(height) to return true if height ≥ BlossomActivationHeight,
otherwise false.

This specification assumes that BlossomActivationHeight ≥ SlowStartInterval.

Choose a reason for hiding this comment

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

Good to document. Does it also assume that BlossomActivationHeight > overwinter and sapling heights?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Strictly speaking no (a Zcash fork could activate changes together or in a different order, since Overwinter and Sapling did not alter anything related to block spacing).

- PoWTargetSpacing(height) :=

- PreBlossomPoWTargetSpacing, if not IsBlossomActivated(height)
- PostBlossomPoWTargetSpacing, otherwise.

Choose a reason for hiding this comment

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

The problem with this naming scheme is that it gets clumsy when and if we change it again. But I suppose let's cross that bridge when we come to it.

TODO: ideally, BlossomActivationHeight, PostBlossomHalvingInterval, and PostBlossomTargetSpacing should be chosen so that:

- (BlossomActivationHeight - SlowStartShift) / PreBlossomHalvingInterval + (height - BlossomActivationHeight) / PostBlossomHalvingInterval)
is exactly 1 for some integer height.

Choose a reason for hiding this comment

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

why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's just cleaner to calculate future halving block heights by hand if this condition holds.

spacing to adjust to the new target, at the normal rate for a difficulty
adjustment. The results of simulations are consistent with this expected
behaviour.

Choose a reason for hiding this comment

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

Seems correct.

block height that corresponds approximately to a given time after release. This
interval SHOULD be adjusted in releases where the End-of-Service halt time will
follow Blossom activation.

Choose a reason for hiding this comment

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

Pretty important we get this right! :D



TODO: check for any other number-of-block constants.

Choose a reason for hiding this comment

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

Has anyone done this?

@zebambam
Copy link

I also did not check the tex changes.

@daira daira merged commit acff8a6 into zcash:master May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants