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

[ZIP 213] Shielded Coinbase #217

Merged
merged 14 commits into from
Jan 30, 2020
Merged

Conversation

str4d
Copy link
Collaborator

@str4d str4d commented Mar 30, 2019

@daira daira changed the title Draft ZIP: Shielded Coinbase [WIP] [ZIP 213] Shielded Coinbase Apr 1, 2019
Copy link
Collaborator

@daira daira left a comment

Choose a reason for hiding this comment

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

ACK modulo comments.

drafts/zip-str4d-shielded-coinbase.rst Outdated Show resolved Hide resolved
drafts/zip-str4d-shielded-coinbase.rst Outdated Show resolved Hide resolved
drafts/zip-str4d-shielded-coinbase.rst Outdated Show resolved Hide resolved
drafts/zip-str4d-shielded-coinbase.rst Outdated Show resolved Hide resolved
@daira
Copy link
Collaborator

daira commented Apr 5, 2019

Issue that I raised (off-topicly!) in the Blossom audit meeting: how do you detect which outputs are to funding streams in coinbase transactions, if they can be shielded?

@str4d
Copy link
Collaborator Author

str4d commented Apr 30, 2019

@daira there are several consensus rules specified in the ZIP that collectively enforce that shielded funding streams can be detected.

Relatedly, now that the funding streams ZIP has been removed from Blossom, I need to modify this ZIP to make those sections conditional on ZIP 207, and cover the situation where that ZIP is never deployed but we separately decide to migrate the existing FR transparent addresses to shielded ones.

str4d added 3 commits May 2, 2019 11:49
ZIP 207 was withdrawn from the Blossom network upgrade, and this ZIP can
no longer assume that it will already be deployed.
@str4d
Copy link
Collaborator Author

str4d commented May 2, 2019

Addressed @daira's comments, and made the funding stream consensus rules conditional on ZIP 207 deployment.

@ravibhojwani
Copy link

Congrats @daira on #199

@mms710
Copy link

mms710 commented May 17, 2019

@zmanian Would you be willing to review and provide comments on this ZIP next week?

@geffenz
Copy link

geffenz commented May 20, 2019

[UX Review]: This should have no impact on an end-user's UX.

I am re-evaluating this with more research on how this effects miners and any security/privacy risks associated with this ZIP. I'll post a new UX Review below soon.

@mms710
Copy link

mms710 commented May 22, 2019

Hi @zmanian, just checking back in. Would you be willing to review and provide comments on this ZIP this week?

ebfull
ebfull previously requested changes May 23, 2019
zip-0213.rst Outdated Show resolved Hide resolved
zip-0213.rst Outdated Show resolved Hide resolved
zip-0213.rst Outdated Show resolved Hide resolved
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.

Great stuff, let's do it! 👍

drafts/zip-str4d-shielded-coinbase.rst Outdated Show resolved Hide resolved
contain Sapling outputs.

- The consensus rules applied to ``valueBalance``, ``vShieldedOutput``, and ``bindingSig``
in non-coinbase transactions MUST also be applied to coinbase transactions.

Choose a reason for hiding this comment

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

I don't know if these are the only ones.

drafts/zip-str4d-shielded-coinbase.rst Outdated Show resolved Hide resolved
outputs.

- All Sapling outputs in coinbase transactions MUST have valid note commitments when
recovered using a 32-byte array of zeroes as the outgoing viewing key.

Choose a reason for hiding this comment

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

I don't understand how we would implement this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the consensus rules, for each Sapling coinbase output, we trial-recover it with the all-zeroes OVK, and if that succeeds we then check the commitment matches.

drafts/zip-str4d-shielded-coinbase.rst Outdated Show resolved Hide resolved
enforce this on all outputs. Additionally, this maintains the ability for network
observers to track miners and mining pools. Meanwhile, the miners and mining pools could
put useful or identifying text in the memo fields of the outputs, instead of storing it
ad-hoc elsewhere in the coinbase transaction.

Choose a reason for hiding this comment

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

Is this behavior obvious to everyone? I think it is. Would be a nasty surprise if not.

zip-0213.rst Outdated
- In the coinbase transaction for block height ``height``, for every active funding stream
that targets a Sapling address at ``height``, the transaction MUST include at least one
Sapling output that pays exactly the funding stream's value to the target Sapling
address.

Choose a reason for hiding this comment

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

Do you mean "exactly one Sapling output that pays exactly the funding stream's value to the target Sapling address." I think I understand what you meant - the transaction must have at least one output, but I think you might have literally said that it's okay to pay out the block reward multiple times in multiple outputs.

Copy link

Choose a reason for hiding this comment

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

What I'm saying is that it might be worth restating that the outputs of the coinbase transaction still have to balance.

zip-0213.rst Show resolved Hide resolved
zip-0213.rst Outdated Show resolved Hide resolved
zip-0213.rst Outdated Show resolved Hide resolved
@mms710
Copy link

mms710 commented May 24, 2019

Hi @zmanian! Just a reminder that we'd like your review of this ZIP today if possible. Thanks!

@zmanian
Copy link

zmanian commented May 26, 2019

I would support prioritizing this because it moves Zcash closer to deprecating transparent addresses and does not seem technically complex.

Copy link

@jcb82 jcb82 left a comment

Choose a reason for hiding this comment

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

This mostly looks good to me, though I couldn't completely follow the logic about maturity rules.

zip-0213.rst Show resolved Hide resolved
to create.

The Sapling network upgrade [#zip-0205]_ deployed architectural changes and performance
improvements that make shielding funds directly in the coinbase transaction feasible. In
Copy link

Choose a reason for hiding this comment

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

This may be irrelevant but this explanation doesn't quite make sense to me. Why was it banned if the only problem was feasibility? It seems it always could have been optional if miners wanted to do it. Perhaps an explanation would help about why miners are very sensitive to latency in creating a coinbase transaction

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem wasn't only efficiency: "This was partly in order to reduce the complexity of the original Zcash modifications to the Bitcoin Core codebase [...]".

zip-0213.rst Outdated
in non-coinbase transactions MUST also be applied to coinbase transactions.

- The existing consensus rule requiring transactions that spend coinbase outputs to have
an empty ``vout``, is amended to only apply to transactions that spend transparent
Copy link

Choose a reason for hiding this comment

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

I don't quite get the rational here. Is this really saying shielded coinbase outputs can be sent directly to transparent addresses?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. What this line is doing is narrowing an existing consensus rule, because after this ZIP is deployed, the existing consensus rule is unenforceable as written. This consensus rule cannot be enforced on transactions that "spend coinbase outputs" (now that this includes shielded outputs), because transactions spending shielded coinbase outputs cannot be distinguished from transactions spending other shielded outputs. Thus the rule must be narrowed to only cover transactions that "spend transparent coinbase outputs".

coinbase note commitments in the Sapling commitment tree. The standard recommendation when
spending a note is to select an anchor 10 blocks back from the current chain tip, which
acts as a de-facto 10-block maturity on all notes, coinbase included. This might be
proposed as a consensus rule in future.
Copy link

Choose a reason for hiding this comment

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

Wouldn't this break a lot of functionality as a consensus rule?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not that I can see. It simply forces users to wait an extra ten blocks before spending newly-received ZEC, which from a usability perspective is equivalent to the transaction being mined 10 blocks "late" (something apps already have to account for in their UX, because this can happen naturally on the network today).

zip-0213.rst Outdated
coinbase output had been appended to. That is, all economic activity would be rolled back
in addition to the shielded coinbase output disappearing, so there is no reason to make
shielded coinbase a special case when the same behaviour occurs in regular shielded notes
already. In the transparent coinbase case, only direct child transactions of the
Copy link

Choose a reason for hiding this comment

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

I got a bit lost here. It would help to explain why coinbase maturity rules exist in the first place, and why the above reasoning that they are unnecessary does not apply to transparent coinbase outputs. I can't see why that's the case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I'm unsure what side-effects there might be to making it possible to spend shielded coinbases right away. A couple things come to mind:

  • Change in economic incentives: miners can sell their earnings ~4 hours sooner, potentially enabling them to profit from attacks (e.g. a 51% attack) they believed would not have been profitable before because they expected the attack to be detected and the price to crash before their mining rewards matured.
  • A malicious miner has has a new resource: the rewards from blocks they mine become available immediately. That might increase the strength of already-known attacks. If there's any situation where a malicious miner needs to send bribes while they are carrying out an attack, being able to use the block rewards earned as part of the attack to bribe would reduce the up-front cost of the attack. For example maybe they're renting GPUs to mine a competing chain and as part of that attack they immediately send their block rewards to miners' of other chains to incentivize them to start mining on the shorter chain.

Is there anything else I'm missing?

zip-0213.rst Outdated
coinbase output had been appended to. That is, all economic activity would be rolled back
in addition to the shielded coinbase output disappearing, so there is no reason to make
shielded coinbase a special case when the same behaviour occurs in regular shielded notes
already. In the transparent coinbase case, only direct child transactions of the
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I'm unsure what side-effects there might be to making it possible to spend shielded coinbases right away. A couple things come to mind:

  • Change in economic incentives: miners can sell their earnings ~4 hours sooner, potentially enabling them to profit from attacks (e.g. a 51% attack) they believed would not have been profitable before because they expected the attack to be detected and the price to crash before their mining rewards matured.
  • A malicious miner has has a new resource: the rewards from blocks they mine become available immediately. That might increase the strength of already-known attacks. If there's any situation where a malicious miner needs to send bribes while they are carrying out an attack, being able to use the block rewards earned as part of the attack to bribe would reduce the up-front cost of the attack. For example maybe they're renting GPUs to mine a competing chain and as part of that attack they immediately send their block rewards to miners' of other chains to incentivize them to start mining on the shorter chain.

Is there anything else I'm missing?

zip-0213.rst Show resolved Hide resolved
@zookozcash
Copy link

For the record, I've read this ZIP and all of the comments up to here.

zip-0213.rst Outdated Show resolved Hide resolved
@daira daira force-pushed the master branch 3 times, most recently from 9a2be35 to e5c7413 Compare August 6, 2019 00:33
@str4d str4d added the S-committed Status: Planned work in a sprint label Jan 22, 2020
zkbot added a commit to zcash/zcash that referenced this pull request Jan 23, 2020
[NU3 Heartwood] Shielded Coinbase

Implements [ZIP 213](zcash/zips#217).
@str4d str4d dismissed ebfull’s stale review January 27, 2020 20:47

Addressed comments

@str4d str4d changed the title [WIP] [ZIP 213] Shielded Coinbase [ZIP 213] Shielded Coinbase Jan 27, 2020
zip-0213.rst Outdated
=============

Prior to activation of the Heartwood network upgrade, the existing consensus rule
requiring coinbase transactions to have zero Sapling outputs is enforced.
Copy link
Collaborator

@daira daira Jan 27, 2020

Choose a reason for hiding this comment

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

The rule is actually:

A coinbase transaction MUST NOT have any JoinSplit descriptions, Spend descriptions, or Output descrip-
tions.

This becomes:

[Pre-Heartwood] A coinbase transaction MUST NOT have any JoinSplit descriptions, Spend descriptions, or Output descriptions.
[Heartwood onward] A coinbase transaction MUST NOT have any JoinSplit descriptions or Spend descriptions.

or alternatively

A coinbase transaction MUST NOT have any JoinSplit descriptions or Spend descriptions.
[Pre-Heartwood] A coinbase transaction also MUST NOT have any Output descriptions.

zip-0213.rst Outdated

- The existing consensus rule requiring coinbase outputs to have 100 confirmations
before they may be spent (coinbase maturity), is amended to only apply to transparent
coinbase outputs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Specifically it becomes:

A transaction MUST NOT spend a transparent output of a coinbase transaction from a block less than 100 blocks prior to the spend. Note that outputs of coinbase transactions include Founders’ Reward outputs [and potentially funding stream outputs].

[#zip-0205]_.

The terms "Founders' Reward" and "funding stream" in this document are to be interpreted
as described in ZIP 207 [#zip-0207]_.
Copy link
Collaborator

@daira daira Jan 27, 2020

Choose a reason for hiding this comment

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

Since ZIP 207 is definitely not included in Heartwood and there will be a new ZIP (or ZIP 1014 will be amended) to include the dev fund in NU4 [subject to the conclusion of the dev funding process], I think we should just simplify this ZIP by removing references to ZIP 207.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I considered that, but it's still relevant to the rationale. If we weren't going to have funding stream outputs ever be shielded, then we wouldn't technically need to require that all outputs be decryptable.

of the transparent coinbase would become invalid, and thus it would be possible to end up
in a situation where a logical child transaction (for example, a mining pool paying out
miners) persists in the block chain after its logical parent (the mining pool receiving a
block) disappears.
Copy link
Collaborator

Choose a reason for hiding this comment

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

How can such a child transaction "persist in the block chain"? It becomes invalid, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A "logical" child isn't necessarily a cryptographic child. For example, if a mining pool receives coinbase as UTXO Q, and then makes a payout for the block that Q was mined in, using UTXO M from before Q was mined, then it is possible for a reorg to cause Q to disappear but still leave M spent.

Copy link
Collaborator

@daira daira left a comment

Choose a reason for hiding this comment

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

ACK with minor comments.

Copy link
Collaborator

@daira daira left a comment

Choose a reason for hiding this comment

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

ACK

@daira daira merged commit 124476b into zcash:master Jan 30, 2020
zkbot added a commit to zcash/zcash that referenced this pull request Mar 6, 2020
[NU3 Heartwood] Shielded Coinbase

Implements [ZIP 213](zcash/zips#217).
@str4d str4d deleted the zip-str4d-shielded-coinbase branch April 14, 2020 21:35
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.