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

[ZIPs 207, 214, 251] Consensus ZIPs for Zcash Development Fund #332

Merged
merged 26 commits into from
Apr 21, 2020

Conversation

daira
Copy link
Collaborator

@daira daira commented Mar 10, 2020

closes #328 and #330

zip-0214.rst Show resolved Hide resolved
zip-0214.rst Show resolved Hide resolved
zip-0214.rst Outdated Show resolved Hide resolved
zip-0251.rst Outdated Show resolved Hide resolved
@daira daira added this to the Core Sprint 2020-11 milestone Mar 12, 2020
@daira
Copy link
Collaborator Author

daira commented Mar 13, 2020

@str4d all your comments should have been addressed, please re-review.

We also need a reviewer from ZF.

@daira daira force-pushed the zip-0251 branch 3 times, most recently from 4f94882 to ab5e7ef Compare March 17, 2020 15:18
Copy link
Contributor

@gtank gtank left a comment

Choose a reason for hiding this comment

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

This is an artifact of ECC's key management policy, not consensus necessity. It's likely that ZF and notional direct grant recipients may have different address handling. We need to think about this.

zip-0214.rst Outdated Show resolved Hide resolved
zip-0214.rst Outdated Show resolved Hide resolved
@daira
Copy link
Collaborator Author

daira commented Mar 19, 2020

Waiting on @acityinohio to decide whether ZF objects to the SHALL requirement for independent addresses.

@acityinohio
Copy link
Contributor

Waiting on @acityinohio to decide whether ZF objects to the SHALL requirement for independent addresses.

Thanks for the mention @daira; I agree with @gtank and based on the discussion here I'd definitely prefer SHOULD instead of SHALL; it would give us more leeway given the Foundation's current custody arrangements, which lend themselves to using the same address. I'm perhaps not the best person to talk about the trade-offs here but I can say that we feel comfortable using the same address for the time being, given the environment in which it was generated, the insurance against it, and how it's stored.

@daira daira force-pushed the zip-0251 branch 2 times, most recently from be53e79 to 8b143d7 Compare March 24, 2020 23:08
defuse
defuse previously requested changes Mar 25, 2020
\mathsf{AddressChangeInterval} &=& \mathsf{PostBlossomHalvingInterval} / 48 \\
\mathsf{AddressPeriod}(\mathsf{height}) &=&
\mathsf{floor}\left(
{\small\frac{\mathsf{height} + \mathsf{PostBlossomHalvingInterval} - \mathsf{HeightForHalving}(1)}{\mathsf{AddressChangeInterval}}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is PostBlossomHalvingInterval added in the numerator, doesn't that just add 48 to the result?

Copy link
Collaborator Author

@daira daira Mar 26, 2020

Choose a reason for hiding this comment

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

Yeah I don't see why that's necessary. It doesn't change the specified behaviour, because only differences between two outputs of AddressPeriod are used in the computation of FundingStream[Fund].AddressIndex(height). I guess a C++ implementation would have to be more careful to implement floor correctly, since without the addition of PostBlossomHalvingInterval, the AddressPeriod could be negative before the first halving. (This cannot happen on mainnet with the planned activation height, but can happen on testnet.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@str4d, do you think we should change this, or not?

zip-0207.rst Show resolved Hide resolved
zip-0207.rst Show resolved Hide resolved
zip-0251.rst Outdated Show resolved Hide resolved
zip-0207.rst Show resolved Hide resolved
zip-0207.rst Show resolved Hide resolved
@daira daira force-pushed the zip-0251 branch 2 times, most recently from c4c4037 to 90ab3de Compare March 27, 2020 00:16
@daira daira requested review from defuse, gtank and str4d March 27, 2020 00:39
@daira daira dismissed defuse’s stale review March 27, 2020 00:41

Addressed blocking comments.

@daira
Copy link
Collaborator Author

daira commented Mar 30, 2020

This still needs an ACK from a ZF reviewer.

daira and others added 21 commits April 2, 2020 14:49
Signed-off-by: Daira Hopwood <[email protected]>
Signed-off-by: Daira Hopwood <[email protected]>
Signed-off-by: Daira Hopwood <[email protected]>
… the numerator won't overflow.

Signed-off-by: Daira Hopwood <[email protected]>
…ive after NU4 activation.

Signed-off-by: Daira Hopwood <[email protected]>
Remove definitions of unused RFC 2119 keywords.

Signed-off-by: Daira Hopwood <[email protected]>
…RIOD` the same

in terms of blocks, but it represents 1.5 days for the Heartwood upgrade onward.

Signed-off-by: Daira Hopwood <[email protected]>
@daira daira requested review from acityinohio and removed request for defuse and str4d April 13, 2020 08:18
@r3ld3v r3ld3v removed this from the Core Sprint 2020-13 milestone Apr 13, 2020
@daira daira merged commit df126fb into zcash:master Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ZIP 214] Consensus rules for a Zcash Development Fund
8 participants