-
Notifications
You must be signed in to change notification settings - Fork 159
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
[ZIP 233] Zcash Sustainability Fund #703
Conversation
This was likely a copy-paste error with the section above it, which is very similar but presents the human-readable part of *incoming* viewing keys.
The `LEBS2OS` function does not exist and isn't meant to. This reference is understood to have meant `LEBS2OSP`. See discussion at: https://forum.zcashcommunity.com/t/what-is-the-lebs2os-function-in-the-zip-32-spec/44886
The `dk` value is 256 bits long. It's the *diversifier* that is only 88 bits long. The incoming viewing key requires the diversifier key -- not the diversifier. This change also reflects the de facto standard in implementations up to this point, including YWallet and the [zcash_address crate](https://docs.rs/zcash_address/latest/src/zcash_address/kind/unified/ivk.rs.html).
|
||
- `ZSF_DEPOSIT : u64 [zatoshi]` | ||
|
||
The `ZSF_BALANCE[H]` for a block at height `H` can be calculated given a value of `ZSF_BALANCE[H-1]` and the set of transactions contained in that block. First, the `ZSF_DEPOSIT[H]` is calculated based solely on `ZSF_BALANCE[H-1]`. This is subtracted from the previous block's balance to be distributed as part of the block reward. Second, the sum of all the `ZSF_DEPOSIT` fields of all transactions in the block is added to the balance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO for the next ZIP editors meeting: suggestions for partially deleting and rewriting this paragraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will suggest how to rewrite this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daira any suggestion here?
Signed-off-by: Daira Emma Hopwood <[email protected]>
Fix errors in protocol and ZIPs
Bumps [actions/checkout](https://github.com/actions/checkout) from 3.5.3 to 3.6.0. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v3.5.3...v3.6.0) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
|
||
Consensus nodes are then required to track new per-block state: | ||
|
||
- `ZSF_BALANCE[H] : u64 [zatoshi]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Format questions for ZIP Editors:
- should this be
ZSF_BALANCE_AFTER[h]
for consistency with the functionZsfBalanceAfter(h)
? - are the similar names too confusing?
- is the missing
_AFTER
potentially ambiguous?
This change would need to apply to the whole document. (I have already suggested switching to ZsfBalanceAfter
in the other ZIP, so there's no need to update it.)
- `ZSF_BALANCE_AFTER[H] : u64 [zatoshi]`
draft-zsf.md
Outdated
@@ -0,0 +1,167 @@ | |||
``` | |||
ZIP: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assigned ZIP 233.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 5c19898
|
||
The state is a single 64 bit integer (representing units of `zatoshi`) at any given block height, ``H``, representing the Sustainability Fund balance at that height, ``H``. The `ZSF_BALANCE` can be calculated using the following formula: | ||
|
||
`ZsfBalanceAfter(height) = MAX_MONEY + sum_{h = 0}^{height} (ZsfDeposit(height) + Unclaimed(height) - BlockSubsidy(height))` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The sum is ignoring the sum variable and is instead repeatedly counting the last block in the range.
`ZsfBalanceAfter(height) = MAX_MONEY + sum_{h = 0}^{height} (ZsfDeposit(height) + Unclaimed(height) - BlockSubsidy(height))` | |
`ZsfBalanceAfter(height) = MAX_MONEY + sum_{h = 0}^{height} (ZsfDeposit(h) + Unclaimed(h) - BlockSubsidy(h))` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ZIP Editors had a not-insubstantial discussion about off-by-one errors, and decided that it would be useful to define ZsfBalanceDelta(height)
as the change in the ZSF balance as a result of applying the block at height
to the chain consensus state. Then other values can be derived from this in a way that more closely matches how full nodes will actually implement it:
ZsfBalanceBefore(height) = MAX_MONEY if height == 0 else ZsfBalanceAfter(height - 1)
ZsfBalanceAfter(height) = ZsfBalanceBefore(height) + ZsfBalanceDelta(height)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ZIP still requires work before it could transition out of Draft, but we have quorum from @str4d, @conradoplg, and @arya2 that the ZIP draft can be merged in its current state and then iterated on in subsequent PRs.
(Note for clarity: approval of this PR does not constitute approval of the ZIP for it to transition from Draft to Proposed status; that happens via the ZIP 0 status workflow. Also note that per a comment below, this ZIP is a Consensus ZIP for the purpose of ZIP 0.)
draft-zsf.md
Outdated
|
||
The changes in this ZIP are ultimately minimal, only requiring for the node to track state in the form of a `ZSF_BALANCE`, and for a new transaction field to be added, called `ZSF_DEPOSIT`. While wallet developer would be encouraged to add the `ZSF_DEPOSIT` field to their UIs, no changes or new behavior are absolutely required for developers or ZEC holders. | ||
|
||
This ZIP does not change the current ZEC issuance schedule. Any additional amounts paid into the sustainability fund are reserved for use in future ZIPs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence needs clarification, because @daira and I had different interpretations of "issuance schedule". As of this PR, moving funds "back into" the ZSF doesn't result in any possibility of reissuance under current consensus rules, and thus this PR is isomorphic to "burning", which changes the circulating supply.
Instead, this should say "current ZEC block subsidy issuance schedule" for more precision and to avoid confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in adb46fb
|
||
#### Consensus Rule Changes | ||
|
||
- Transparent inputs to a transaction insert value into a transparent transaction value pool associated with the transaction. Transparent outputs **and sustainability fund deposits** remove value from this pool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to reference the relevant rules in the protocol spec that are being modified, and give their modifications.
|
||
### Coinbase Transactions | ||
|
||
Any excess miner fee/reward is sent to the ZSF. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be made precise, and given in terms of the consensus rule in Section 3.4 of the protocol spec about the balance of the "transparent transaction value pool" (it is sufficient to do so for coinbase transactions; the non-coinbase transactions in a block have this balance of theirs entirely treated as input to the coinbase transaction).
In new blocks, this deposit is tracked via the `ZSF_DEPOSIT` field in coinbase transactions. | ||
|
||
If the `ZSF_DEPOSIT` field is not present in a coinbase transaction with an older transaction version, it is defined as the value of any unspent miner fee and miner reward. This re-defines these previously unspendable funds as ZSF deposits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rewrite these as formal consensus rules applied to particular transaction versions.
|
||
### Non-Coinbase Transactions | ||
|
||
If the `ZSF_DEPOSIT` field is not present in an older transaction version, it is defined to be zero for non-coinbase transactions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be rewritten as a formal consensus rule. It may be easier to depend on ZIP 230 which adds an explicit fee field fee
; after that, this change would be "include the zsf_deposit
field in the transaction value balance calculation".
### `zsfDeposit` fields in transactions | ||
|
||
Each transaction can dedicate some of its excess funds to the ZSF, and the remainder becomes the miner fee, with any excess miner fee/reward going to the ZSF | ||
|
||
This is achieved by adding a new field to the common transaction fields [#zip-0225-transaction-format]: | ||
|
||
- `zsfDeposit : u64 [zatoshi]` | ||
|
||
The `ZSF_BALANCE[H]` for a block at height `H` can be calculated given a value of `ZSF_BALANCE[H-1]` and the set of transactions contained in that block. First, the `ZSF_DEPOSIT[H]` is calculated based solely on `ZSF_BALANCE[H-1]`. This is subtracted from the previous block's balance to be distributed as part of the block reward. Second, the sum of all the `ZSF_DEPOSIT` fields of all transactions in the block is added to the balance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the ZIP Sync meeting today, we decided that (on the presumption that this will be in NU6) this section should be moved to ZIP 230 (and rewritten therein), and ZIP 233 will instead depend on ZIP 230 and constrain how that new field is used.
|
||
## Deposits into the Sustainability Fund | ||
|
||
Sustainability fund deposits can be made via the new `zsfDeposit` field. This can be done by: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the naming and formatting of this field consistent.
### `ZSF_DEPOSIT` Requirements | ||
|
||
- ZIP 230 [3] must be updated to include `ZSF_DEPOSIT`. | ||
- ZIP 244 [4] must be updated as well to include `ZSF_DEPOSIT`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might go in ZIP 244, or it might go into a separate ZIP; the decision depends on the overall size of the delta between ZIPs 225 and 230. If e.g. ZSAs remained in ZIP 230 then this would definitely be a new ZIP.
Owners: Jason McGee <[email protected]> | ||
Mark Henderson <[email protected]> | ||
Tomek Piotrowski <[email protected]> | ||
Mariusz Pilarek <[email protected]> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ZIP editors would like to reconfirm whether these are still the active owners of the ZIP:
- Jason McGee
- @aphelionz
- @tomekpiotrowski
- Mariusz Pilarek
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to stay as owner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest are also active owners.
Co-authored-by: str4d <[email protected]>
I made the edits that I could, very minor things. The rest I'm not confident that I have the mathematical background to do correctly. Might somebody be able to assist from here? |
No description provided.