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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 26 commits into from
Apr 21, 2020
Merged

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

merged 26 commits into from
Apr 21, 2020

Conversation

daira
Copy link
Collaborator

@daira daira commented Mar 10, 2020

closes #328 and #330

@daira daira requested review from str4d, defuse and gtank March 10, 2020 21:53
str4d
str4d reviewed Mar 10, 2020
View reviewed changes
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
gtank
gtank reviewed Mar 17, 2020
View reviewed changes
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.

str4d
str4d reviewed Mar 19, 2020
View reviewed changes
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 •
edited
Loading

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
View reviewed changes
zip-0207.rst
\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 •
edited
Loading

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
@daira
ZIP 214: Update version in Protocol Specification reference.
0fdd5de 
Signed-off-by: Daira Hopwood <[email protected]>
@daira
ZIP 251: remove trailing whitespace.
45833a1 
Signed-off-by: Daira Hopwood <[email protected]>
@daira
ZIP 207: s/founders reward/Founders' Reward/
83abb5f 
Signed-off-by: Daira Hopwood <[email protected]>
@daira
ZIP 214 is applicable only to Zcash.
e73e517 
Signed-off-by: Daira Hopwood <[email protected]>
@daira
ZIP 207: add definitions of "branch" and "network upgrade".
624ce6d 
Signed-off-by: Daira Hopwood <[email protected]>
@daira
ZIP 214: state explicitly that start height is inclusive and end heig…
0a26121 
…ht is exclusive.

Signed-off-by: Daira Hopwood <[email protected]>
@daira
ZIP 214: change wording to reflect the fact that ZEC != TAZ.
4fd8f06 
Signed-off-by: Daira Hopwood <[email protected]>
@daira
ZIP 214: add rationale section.
0bd8548 
Signed-off-by: Daira Hopwood <[email protected]>
@daira
"branch" -> "consensus branch"
1f77e08 
Signed-off-by: Daira Hopwood <[email protected]>
@daira
Scrunch pre and math blocks a bit more.
3cca1cc 
Signed-off-by: Daira Hopwood <[email protected]>
@daira
ZIP 207: simplify wording about using a proportion of block subsidy.
c9ae7af 
Signed-off-by: Daira Hopwood <[email protected]>
@daira
ZIP 207: use math markup.
b70ba34 
Signed-off-by: Daira Hopwood <[email protected]>
@daira
ZIP 207: in the example implementation, ensure that multiplication by…
1ec5ac7 
… the numerator won't overflow.

Signed-off-by: Daira Hopwood <[email protected]>
@daira
ZIP 207: be more explicit about which consensus rule is no longer act…
9ce78b0 
…ive after NU4 activation.

Signed-off-by: Daira Hopwood <[email protected]>
@daira
ZIP 207: reference fix and HTML.
9ac5984 
Signed-off-by: Daira Hopwood <[email protected]>
@daira
Protocol spec: clarify note about hashFinalSaplingRoot/hashLightClien…
edc4390 
…tRoot.

Signed-off-by: Daira Hopwood <[email protected]>
@daira
Protocol spec: add references for the NU4 upgrade.
5fa56e8 
Signed-off-by: Daira Hopwood <[email protected]>
@daira @str4d
ZIP 214: blockchains -> block chains.
d3784f6 
Co-Authored-By: str4d <[email protected]>
@daira
ZIP 214: allow addresses within a funding stream to be repeated.
27accd4 
Remove definitions of unused RFC 2119 keywords.

Signed-off-by: Daira Hopwood <[email protected]>
@daira
ZIP 207: improvements to the pseudocode.
5182e21 
Signed-off-by: Daira Hopwood <[email protected]>
@daira
ZIPs 208, 250 and 251: keep `NETWORK_UPGRADE_PEER_PREFERENCE_BLOCK_PE…
ec7b435 
…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
@daira daira mentioned this pull request Apr 22, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@str4d str4d str4d left review comments

@ebfull ebfull ebfull approved these changes

@dconnolly dconnolly dconnolly approved these changes

@defuse defuse defuse left review comments

@gtank gtank Awaiting requested review from gtank

@acityinohio acityinohio Awaiting requested review from acityinohio

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ZIP 214] Consensus rules for a Zcash Development Fund
8 participants
@daira @acityinohio @dconnolly @gtank @ebfull @defuse @str4d @r3ld3v