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

Added missing indices over sums #27

Merged
merged 7 commits into from
Jul 4, 2023
Merged

Added missing indices over sums #27

merged 7 commits into from
Jul 4, 2023

Conversation

AntoineRondelet
Copy link

This PR adds missing indices over sums. As a general rule of thumbs, I would suggest to always explicit the indices in the sums (unless absolutely trivial from the context) as it is a source of confusion which can easily be overlooked and end up causing deadly implementation issues.

@netlify
Copy link

netlify bot commented Jun 4, 2023

Deploy Preview for zcash-zips-qedit ready!

Name Link
🔨 Latest commit 782209e
🔍 Latest deploy log https://app.netlify.com/sites/zcash-zips-qedit/deploys/64a3c85e1746d000085939b6
😎 Deploy Preview https://deploy-preview-27--zcash-zips-qedit.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@AntoineRondelet AntoineRondelet marked this pull request as draft June 4, 2023 12:17
zip-0226.rst Outdated
.. math:: \mathsf{bvk = (\sum cv_i^{net})} - \mathsf{ ValueCommit_0^{Orchard}(v^{balanceOrchard})} - \sum_{\mathsf{assetBurn}} \mathsf{ValueCommit_0^{OrchardZSA}(AssetBase, v^{AssetBase}) } = \sum \mathsf{rcv_{i,j}^{net}}\mathcal{R}^{\mathsf{Orchard}}
.. math:: \mathsf{bvk} = \mathsf{(\sum cv_i^{net})} - \mathsf{ValueCommit_0^{Orchard}(v^{balanceOrchard})} - \sum_{\mathsf{assetBurn}} \mathsf{ValueCommit_0^{OrchardZSA}(AssetBase, v^{AssetBase})}

.. math:: \mathsf{bvk} = \sum \mathsf{rcv_{i,j}^{net}}\mathcal{R}^{\mathsf{Orchard}}
Copy link
Author

Choose a reason for hiding this comment

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

AFAICT rcv_{i,j} is not standard notation (couldn't see any use of the double subscript in Zcash protocol specs) and the introduction of the j index is confusing. I suspect this notation is shortcut for sum_{i=1}^{n} rvc_i + \sum_{j=1}^{m} rcv_j or something like that, but I need to have a closer look at the specs and this part of the ZSA to fix this properly and mark this PR as "ready for review."

Copy link
Author

Choose a reason for hiding this comment

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

Ok, so, I bounced on the Zcash specs to see if I was missing a definition or smthg.
AFAIK, there isn't such a thing as rcv_{i,j} so I would strongly suggest to either explicit what this means, or avoid to use this notation all together in order to remove any room for interpretation in the equations.

Looking at Orchard's specs (Section 4.14 Balance and Binding Signature (Orchard) of the Zcash specs):

  • bsk = \ssum^{n}_{i=1} rcv^{net}_i
  • bvk = \csum^{n}_{i=1} cv^{net}_i \cminus ValueCommit_{0}(v^{balance})

where:

  • \ssum is the sum operation of the Pallas scalar field,
  • \csum (resp. \cminus) is the sum (resp. minus) operation of the group (curve)
  • n is the number of Action descriptions
  • cv^net_{i} is the value commitment to the value of the input note minus the value v of the output note for ith Action transfer in the transaction, using rcv_i as random commitment trapdoor.

So, in this ZSA burn section, we build on Orchard's balance and binding signature check, but also allow the sender to include a set of tuples (AssetBase, v^{AssetBase}) to burn v^{AssetBase} of asset AssetBase.

  1. Out of curiosity: Why don't we use assetId instead, since AssetBase is derived from AssetId? Is that just to save on a lookup (or even more: save on the computation of ZSAValueBase) in the circuit? How is it implemented? I guess, the wallet handles the computation of ZSAValueBase to pass the asset's base point as input to the circuit? (If so, do we use caching if ZSAValueBase has already been computed for an assetId? In which case we just need to do a lookup I guess?)

Now looking at the current equation, we have:

\mathsf{bvk} = \mathsf{(\sum cv_i^{net})}  - \mathsf{ValueCommit_0^{Orchard}(v^{balanceOrchard})} - \sum_{\mathsf{assetBurn}} \mathsf{ValueCommit_0^{OrchardZSA}(AssetBase, v^{AssetBase})}

\mathsf{bvk} = \sum \mathsf{rcv_{i,j}^{net}}\mathcal{R}^{\mathsf{Orchard}}

So, I'll unroll the equations, adding explicit summation bounds, assuming we have n Action descriptions, and removing \mathsf for clarity.
Using the definition of ValueCommit_0^{OrchardZSA}() from this ZIP, we get:

bvk = (\sum^{n}_{i=1} cv_i^{net})  - ValueCommit_0^{Orchard}(v^{balanceOrchard}) - (\sum_{assetBurn} ValueCommit_0^{OrchardZSA}(AssetBase, v^{AssetBase}))

=> bvk =  (\sum^{n}_{i=1} cv_i^{net})  - ValueCommit_0^{Orchard}(v^{balanceOrchard}) - (\sum_{assetBurn} [v^{net}_{assetId}]AssetBase^{Orchard}_{assetId} + [0]R^{Orchard})

=> bvk =  (\sum^{n}_{i=1} [v^{net}_i]V^{Orchard} + [rcv_i^{net}]R^{Orchard})  - [v^{balanceOrchard}]V^{Orchard} + [0]R^{Orchard} - (\sum_{assetBurn} [v^{net}_{assetId}] AssetBase^{Orchard}_{assetId} + [0]R^{Orchard})

Now, I'm trying to understand how we lend on the equation:

bvk = \sum \mathsf{rcv_{i,j}^{net}}\mathcal{R}^{\mathsf{Orchard}}

So, let's assume that the cardinality of the assetBurn set is m. Getting back to our last equation, we can explicit the bounds of the sum in the last term to:

bvk =  (\sum^{n}_{i=1} [v^{net}_i]V^{Orchard} + [rcv_i^{net}]R^{Orchard})  - [v^{balanceOrchard}]V^{Orchard} + [0]R^{Orchard} - (\sum^{m}_{j=1} [v^{net}_{j}] AssetBase^{Orchard}_{j} + [0]R^{Orchard})

Note: we replaced \sum_{assetBurn} by \sum^{m}_{j=1}, we replaced v^{net}_{assetId} by v^{net}_{j} and we replaced AssetBase^{Orchard}_{assetId} by AssetBase^{Orchard}_{j} (following the assumption that there is no duplication in the set assetBurn, so there's a bijection between assetBurn and {1..m}).

Now, scalar multiplication by zero gives the point at infinity in the EC group, which is the identity for the group operation. So, [0]R^{Orchard} terms can be removed:

bvk =  (\sum^{n}_{i=1} [v^{net}_i]V^{Orchard} + [rcv_i^{net}]R^{Orchard})  - [v^{balanceOrchard}]V^{Orchard} - (\sum^{m}_{j=1} [v^{net}_{j}] AssetBase^{Orchard}_{j})

Applying distributivity:

bvk = [\sum^{n}_{i=1} v^{net}_i]V^{Orchard} + [\sum^{n}_{i=1} rcv_i^{net}]R^{Orchard} - [v^{balanceOrchard}]V^{Orchard} - [\sum^{m}_{j=1} v^{net}_{j}] AssetBase^{Orchard}_{j}

=> bvk = [(\sum^{n}{i=1} v^{net}_{i}) - v^{balanceOrchard})]V^{Orchard} + [\sum^{n}_{i=1} rcv_i^{net}]R^{Orchard} - [\sum^{m}_{j=1} v^{net}_{j}] AssetBase^{Orchard}_{j}

From that point, and to the risk of missing something obvious, I don't see how we lend to bvk = \sum \mathsf{rcv_{i,j}^{net}}\mathcal{R}^{\mathsf{Orchard}} as currently stated in the ZIP. In fact, we pass 0 as random commitment trapdoor to all instances of ValueCommit^{OrchardZSA}() in this equation, so, unless I miss smthg, there is no such thing as rcv_j.

Copy link
Author

Choose a reason for hiding this comment

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

AFAICT, AssetBase is derived via a Group Hash into Pallas (as per: https://qed-it.github.io/zips/protocol/protocol.pdf#concretegrouphashpallasandvesta), so I don't see any immediate relationship between AssetBase, rcv and R^{Orchard} that could lead to this \sum rcv_{i,j}.

Choose a reason for hiding this comment

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

Out of curiosity: Why don't we use assetId instead, since AssetBase is derived from AssetId? Is that just to save on a lookup (or even more: save on the computation of ZSAValueBase) in the circuit? How is it implemented? I guess, the wallet handles the computation of ZSAValueBase to pass the asset's base point as input to the circuit? (If so, do we use caching if ZSAValueBase has already been computed for an assetId? In which case we just need to do a lookup I guess?)

AssetId is long and can be up to 512B. We prefer to deal with the compact AssetBase. Especially important is the fact that the node need to keep track of all issued and finalized AssetBase/AssetDigest. So the idea is to strive for a small state as much as possible.

Choose a reason for hiding this comment

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

bvk = (\sum^{n}{i=1} cv_i^{net}) - ValueCommit_0^{Orchard}(v^{balanceOrchard}) - (\sum{assetBurn} ValueCommit_0^{OrchardZSA}(AssetBase, v^{AssetBase}))

=> bvk = (\sum^{n}{i=1} cv_i^{net}) - ValueCommit_0^{Orchard}(v^{balanceOrchard}) - (\sum{assetBurn} [v^{net}{assetId}]AssetBase^{Orchard}{assetId} + [0]R^{Orchard})

=> bvk = (\sum^{n}{i=1} [v^{net}i]V^{Orchard} + [rcv_i^{net}]R^{Orchard}) - [v^{balanceOrchard}]V^{Orchard} + [0]R^{Orchard} - (\sum{assetBurn} [v^{net}{assetId}] AssetBase^{Orchard}_{assetId} + [0]R^{Orchard})

This looks correct. However, usually, there is no need to unroll the ValueCommit.

Choose a reason for hiding this comment

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

We might need to introduce indexes in
ValueCommit_0^{OrchardZSA}(AssetBase, v^{AssetBase}))
for AssetBase

Choose a reason for hiding this comment

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

AFAICT, AssetBase is derived via a Group Hash into Pallas (as per: https://qed-it.github.io/zips/protocol/protocol.pdf#concretegrouphashpallasandvesta), so I don't see any immediate relationship between AssetBase, rcv and R^{Orchard} that could lead to this \sum rcv_{i,j}.

To compute bsk we need to combine all rcv that was used by the cv values (both for native zec and other assets). We need to adjust the formula to reflect this fact.

@AntoineRondelet
Copy link
Author

AntoineRondelet commented Jun 18, 2023

Ok, so I've done quite a few changes on the formulas to fix inconsistencies and clarify (for me at least) the intuition about the soundness of the protocol.

  1. Fixed some inconsistencies, e,g. erroneous order in which arguments to ValueCommit were passed. It is defined as ValueCommit(v, AssetBase) an sometimes the AssetBase was passed first, etc.

  2. Clarified what is a check and what isn't. In fact this was a big source of confusion (as per my comments above) as checks weren't always specifically stated. The bvk = \sum rcv_i's in the Burn equation was given on the same line of the rest of the equation and seemed like "the result of simplifying the equation". In fact, that isn't. The equation ONLY equals to \sum rcv_i's IFF the transaction is valid... That's why after playing with expressions I still had terms left and was wondering how we landed on such a simple result.

  3. Unrolled the equations to make it clear why things hold, which also provides intuition around value balancing in transactions. To do this, I split the sum over ZEC and Custom Assets, to relate to the ZCash specs (i.e what terms are the same) and see what are the ZSA specific checks. This clarified for me (and might as well do the same for others), helped me set bounds for all sums and gives intuition to the reader as to what's going on in the check.

  4. Also, I dropped the use of ValueCommit^{Orchard} all together. This was confusing and made the ZIP unnecessarily hard to read IMO. In fact, this ZIP introduces a ValueCommit^{OrchardZSA} function which overloads ValueCommit^{Orchard} by passing an extra argument (i.e. the AssetBase). Since we explicitly mention that ZEC's AssetBase is V^{Orchard}, I've drew a clear connection between ValueCommit^{OrchardZSA} and ValueCommit^{Orchard} and decided to only use ValueCommit^{OrchardZSA} in this ZIP to only have the ValueCommit function defined in this ZIP in the equations.

  5. Finally: Added missing indices and some missing details, e.g. edge cases around sums where the starting index is higher than the upper bound, added notation for the cardinality of the assetBurn set etc.

@AntoineRondelet AntoineRondelet marked this pull request as ready for review June 18, 2023 12:35
Copy link

@PaulLaux PaulLaux left a comment

Choose a reason for hiding this comment

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

Good overall, added some comments for clarity.

In addition, please make sure the changes are compatible with the Value Commitment section - It seems that the expression AssetBase_AssetId^Orchard can be simplified to just AssetBase_k

zip-0226.rst Outdated

The value balance verification is equivalent to:

.. math:: \mathsf{bvk} = \mathsf{(\sum_{i=1}^{l} cv_i^{net}) + (\sum_{j=l+1}^{n} cv_i^{net})} - \mathsf{ValueCommit_0^{OrchardZSA}(v^{balanceOrchard}, \mathcal{V}^{\mathsf{Orchard}})}

Choose a reason for hiding this comment

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

should be (\sum_{i=1}^{l} cv_i^{net}) + (\sum_{j=l+1}^{n} cv_j^{net})} (a small typo in cv_j)
Also, consider a different letter for the upper bound instead of l, maybe k

Copy link
Author

Choose a reason for hiding this comment

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

Also, consider a different letter for the upper bound instead of l, maybe k

Sure. I've switched to m (k is generally used in the indices along with i, j see, e.g. Conventional variable names section)

zip-0226.rst Outdated

.. math:: \mathsf{bvk} = \mathsf{(\sum_{i=1}^{l} cv_i^{net}) + (\sum_{j=l+1}^{n} cv_i^{net})} - \mathsf{ValueCommit_0^{OrchardZSA}(v^{balanceOrchard}, \mathcal{V}^{\mathsf{Orchard}})}

.. math:: \mathsf{bvk} = [\mathsf{(\sum_{i=1}^{l} cv_i^{net})} - \mathsf{ValueCommit_0^{OrchardZSA}(v^{balanceOrchard}, \mathcal{V}^{\mathsf{Orchard}})}] + \mathsf{(\sum_{j=l+1}^{n} cv_i^{net})}

Choose a reason for hiding this comment

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

This rearrangement does not add new information to the reader, the previous form for is fully sufficient, consider removing the entire line.

zip-0226.rst Outdated

.. math:: \mathsf{bvk} = [\mathsf{(\sum_{i=1}^{l} cv_i^{net})} - \mathsf{ValueCommit_0^{OrchardZSA}(v^{balanceOrchard}, \mathcal{V}^{\mathsf{Orchard}})}] + \mathsf{(\sum_{j=l+1}^{n} cv_i^{net})}

The first term of this equation gives the balance check of the Orchard protocol [#protocol-binding]_. For a correctly constructed transaction, we get :math:`\mathsf{(\sum_{i=1}^{l} v_i^{net}) = v^{balanceOrchard}}`, which means that :math:`\mathsf{(\sum_{i=1}^{l} cv_i^{net})} - \mathsf{ValueCommit_0^{OrchardZSA}(v^{balanceOrchard}, \mathcal{V}^{\mathsf{Orchard}})}` MUST be equal to :math:`\mathsf{\sum_{i=1}^{l} rcv_{i}^{net}}\mathcal{R}^{\mathsf{Orchard}}`. With ZSA, transfer Actions for Custom Assets MUST also be balanced across asset bases. As such, for a correctly constructed transaction, we MUST get :math:`\mathsf{(\sum_{j=l+1}^{n} v_i^{net}) = 0}`, and thus :math:`\mathsf{\sum_{j=l+1}^{n} rcv_{j}^{net}}\mathcal{R}^{\mathsf{Orchard}}`, where :math:`\sum_{j=l+1}^{n} x_j` gives the identity element of the group, if :math:`l+1 > n`.

Choose a reason for hiding this comment

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

"For a correctly constructed transaction ..." - there is no point in repeating the spec in such detail. Remove this sentence.

Also, what is x_j ?

And I don't think that l+1 can be larger than n.

Copy link
Author

Choose a reason for hiding this comment

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

And I don't think that l+1 can be larger than n.

If l = n (i.e. all actions move ZEC), then l+1 > n

Also, what is x_j ?

An arbitrary group element. This is introduced to tackle the edge case where l=n and thus l+1 > n, so we want to make it clear that the sum returns the identity element in the group (in code, the "for loop condition" would fail and thus the for loop instruction's would not be executed.). Returning the identity does the similar effect, and makes sure the sum doesn't "interfere" with the rest of the equation when the bounds ordering constraint is violated (i.e. lower bound > upper bound)

zip-0226.rst Outdated

.. math:: \mathsf{bvk = (\sum cv_i^{net})} - \mathsf{ ValueCommit_0^{Orchard}(v^{balanceOrchard})} - \sum_{\mathsf{assetBurn}} \mathsf{ValueCommit_0^{OrchardZSA}(AssetBase, v^{AssetBase}) } = \sum \mathsf{rcv_{i,j}^{net}}\mathcal{R}^{\mathsf{Orchard}}
.. math:: \mathsf{bvk} = \mathsf{(\sum_{i=1}^{n} cv_i^{net})} - \mathsf{ValueCommit_0^{OrchardZSA}}(\mathsf{v^{balanceOrchard}}, \mathcal{V}^{\mathsf{Orchard}}) - \mathsf{\sum_{k=1}^{|assetBurn|} ValueCommit_0^{OrchardZSA}(v_{k}^{AssetBase}, AssetBase_{k})}

Choose a reason for hiding this comment

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

\sum_{k=1}^{|assetBurn|} is quite unusual and looks strange, replace |assetBurn| with a letter here and in We denote :math:|\mathsf{assetBurn}| the cardinality of...

lets remove ^AssetBase from (v_{k}^{AssetBase}. It does not contribute anything.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I can introduce an extra variable for the cardinality (note that |S| is quite standard notation for the cardinality though, e.g. https://en.wikipedia.org/wiki/Cardinality)

Copy link
Author

Choose a reason for hiding this comment

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

lets remove ^AssetBase from (v_{k}^{AssetBase}. It does not contribute anything.

Happy to remove these things, I'm just trying to be consistent with the existing notations (v^AssetBase is used throughout this ZIP)

zip-0226.rst Outdated
Comment on lines 326 to 332
Removing the points at infinity (i.e. identity elements in the group) and unrolling the :math:`cv_i`'s:

.. math:: \mathsf{bvk} = ((\sum_{i=1}^{l} [\mathsf{v_i^{net}}]\mathcal{V}^{\mathsf{Orchard}} + [\mathsf{rcv_i}]\mathcal{R}^{\mathsf{Orchard}}) + (\sum_{j=l+1}^{n} \mathsf{[v_j^{net}]\mathsf{AssetBase}_j} + [\mathsf{rcv_j}]\mathcal{R}^{\mathsf{Orchard}})) - ([\mathsf{v^{balanceOrchard}}]\mathcal{V}^{\mathsf{Orchard}}) - (\sum_{k=1}^{|\mathsf{assetBurn}|} [\mathsf{v_{k}^{AssetBase}}]\mathsf{AssetBase_{k}})

Rearranging

.. math:: \mathsf{bvk} = [(\sum_{i=1}^{l} [\mathsf{v_i^{net}}]\mathcal{V}^{\mathsf{Orchard}} + [\mathsf{rcv_i}]\mathcal{R}^{\mathsf{Orchard}}) - ([\mathsf{v^{balanceOrchard}}]\mathcal{V}^{\mathsf{Orchard}})] + [(\sum_{j=l+1}^{n} \mathsf{[v_j^{net}]\mathsf{AssetBase}_j} + [\mathsf{rcv_j}]\mathcal{R}^{\mathsf{Orchard}}) - (\sum_{k=1}^{|\mathsf{assetBurn}|} [\mathsf{v_{k}^{AssetBase}}]\mathsf{AssetBase_{k}})]

Choose a reason for hiding this comment

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

The unrolling is unnecessary for a reader that understand the original nu5 spec. It tolls the reader and will require a significant amount of maintenance from us in the future. Remove those lines.

zip-0226.rst Outdated

.. math:: \mathsf{bvk} = [(\sum_{i=1}^{l} [\mathsf{v_i^{net}}]\mathcal{V}^{\mathsf{Orchard}} + [\mathsf{rcv_i}]\mathcal{R}^{\mathsf{Orchard}}) - ([\mathsf{v^{balanceOrchard}}]\mathcal{V}^{\mathsf{Orchard}})] + [(\sum_{j=l+1}^{n} \mathsf{[v_j^{net}]\mathsf{AssetBase}_j} + [\mathsf{rcv_j}]\mathcal{R}^{\mathsf{Orchard}}) - (\sum_{k=1}^{|\mathsf{assetBurn}|} [\mathsf{v_{k}^{AssetBase}}]\mathsf{AssetBase_{k}})]

So, we see that the transaction is balanced, only if

Choose a reason for hiding this comment

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

What do you mean by balanced? should briefly mention the connection between bsk and bvk here.

Copy link
Author

Choose a reason for hiding this comment

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

What I mean by balanced is that: the sum of the net values of the Actions equals the sum of v^balanceOrchard and v_k^AssetBase. bvk = [bsk]R^orchard only if this relationship holds for the values in the transaction. Else, residual terms remain and the check fails.

Copy link
Author

Choose a reason for hiding this comment

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

should briefly mention the connection between bsk and bvk here.

This is already mentioned a few lines above

After computing :math:`\mathsf{bvk}`, the verifier MUST use it to verify the `bindingSignature` on the `SIGHASH` message.

But, np, I've just added another mention here as well.

zip-0226.rst Outdated
So, we see that the transaction is balanced, only if

1. :math:`\mathsf{\sum_{i=1}^{l} v_i^{net} = v^{balanceOrchard}}`, in which case the first half of the equation becomes :math:`\mathsf{\sum_{i=1}^{l} rcv_i^{net}}`.
2. :math:`\mathsf{\sum_{j=l+1}^{n} v_j^{net} = \sum_{k=1}^{|\mathsf{assetBurn}|} v_k^{AssetBase}}`, for matching asset bases, i.e. :math:`\sum\limits_{\substack{j=l+1 \\ \mathsf{AssetBase}_j = \mathsf{AssetBase}_k}}^{n} \mathsf{[v_j^{net}]\mathsf{AssetBase}_j} = \sum_{k=1}^{|\mathsf{assetBurn}|} [\mathsf{v_{k}^{AssetBase}}]\mathsf{AssetBase_{k}}`, in which case the second half of the equation becomes :math:`\mathsf{\sum_{j=l+1}^{n} rcv_i^{net}}`.

Choose a reason for hiding this comment

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

Remove " i.e. :math:\sum\limits_{\substack{j=l+1 \\ \mathsf{AssetBase}_j = \mathsf{AssetBase}_k}}^{n} \mathsf{[v_j^{net}]\mathsf{AssetBase}_j} = \sum_{k=1}^{|\mathsf{assetBurn}|} [\mathsf{v_{k}^{AssetBase}}]\mathsf{AssetBase_{k}} ". It does not contribute a lot and it is hard to read. The rest is sufficient.

@AntoineRondelet
Copy link
Author

AntoineRondelet commented Jun 25, 2023

Good overall, added some comments for clarity.

In addition, please make sure the changes are compatible with the Value Commitment section - It seems that the expression AssetBase_AssetId^Orchard can be simplified to just AssetBase_k

@PaulLaux I've implemented most of the changes (see comments above for more details).

On this note though, after doing another pass I'm not sure what you mean here.

  • AssetBase_AssetId^Orchard is defined as such in ZIP227, so I'm trying to stick to the def as much as I can. FYI: I've added some clarification in ZIP227 some time ago to say "when clear from context, we might omit ^Orchard in the notation"
  • In the burn equation I'm using k as index to represent the elements of the set assetBurn which are defined as tuples (AssetBase, v^{AssetBase}) (this is outside of this PR's scope and currently untouched). So, in the equations, we use v_k and AssetBase_k as syntactic sugar for assetBurn_k[1], assetBurn_k[2]. In fact, since assetBurn's elements are tuples, the kth element is a tuple, from which we need to extract specific elements with an operator like [] (or, yet another subscript, like assetBurn_k_1 etc). To keep it simple, I'm using v_k and AssetBase_k. I don't think this notation is necessarily clashing with notations in the Value Commitment section, neither do I think it should be used there. The k index is only relevant to the equation and isn't defined in the value commitment section.

Let me know if I miss something though.

Copy link

@vivek-arte vivek-arte left a comment

Choose a reason for hiding this comment

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

Added some comments.

They are mostly regarding that the spec seems to always use normal math rather than italics for the summation limits. So I suggested some changes to do that.
Other than that, maybe 2-3 other suggestions.

zip-0226.rst Outdated Show resolved Hide resolved
zip-0226.rst Outdated Show resolved Hide resolved

and use it to verify the `bindingSignature` on the `SIGHASH` message.
After computing :math:`\mathsf{bvk}`, the verifier MUST use it to verify the `bindingSignature` on the `SIGHASH` message.

Choose a reason for hiding this comment

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

Just noting we need to remove the single backtick formatting, but that is for a different PR.

Choose a reason for hiding this comment

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

This has been done in PR#30.

zip-0226.rst Outdated

Rationale for Value Balance Verification
````````````````````````````````````````

The main reason why no changes to the Orchard process are needed is that no Custom Assets can be unshielded, so all Custom Assets are contained within the shielded pool. This means that the net balance of the input and output values is zero, with only one Asset of value balance published, that of ZEC, :math:`\mathsf{v^{balanceOrchard}}`. No net amount of any other Asset will be revealed, and the number of Assets in the transaction is also hidden. The only exception to this is in the case that an Asset is *burnt*, as we will see below in the `burn mechanism`_.
We assume :math:`n` Actions in a transfer. Out of these :math:`n` Actions, we further distinguish (for the sake of verbosity and clarity) between Actions related to ZEC and Actions related to Custom Assets. We assume :math:`m` Actions related to ZEC and :math:`n-m` actions related to Custom Assets, where :math:`m \in [0,n]`.

Choose a reason for hiding this comment

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

Is there a need to clarify that for the sake of simplicity we are assuming the ZEC Actions are first and the others are later (in practice they could be in whatever order).

Copy link
Author

Choose a reason for hiding this comment

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

Sure, we can do that. I've just added a line.

zip-0226.rst Outdated Show resolved Hide resolved
zip-0226.rst Outdated Show resolved Hide resolved
.. math:: \mathsf{bvk = (\sum cv_i^{net})} - \mathsf{ ValueCommit_0^{Orchard}(v^{balanceOrchard})} - \sum_{\mathsf{assetBurn}} \mathsf{ValueCommit_0^{OrchardZSA}(AssetBase, v^{AssetBase}) } = \sum \mathsf{rcv_{i,j}^{net}}\mathcal{R}^{\mathsf{Orchard}}
.. math:: \mathsf{bvk} = \mathsf{(\sum_{i=1}^{n} cv_i^{net})} - \mathsf{ValueCommit_0^{OrchardZSA}}(\mathsf{v^{balanceOrchard}}, \mathcal{V}^{\mathsf{Orchard}}) - \mathsf{\sum_{k=1}^{L} ValueCommit_0^{OrchardZSA}(v_{k}, AssetBase_{k})}

After computing :math:`\mathsf{bvk}`, the verifier MUST use it to verify the `bindingSignature` on the `SIGHASH` message.

Choose a reason for hiding this comment

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

Note (again) to remove the single backticks in a separate PR

Choose a reason for hiding this comment

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

This has been done in PR#30.

zip-0226.rst Outdated Show resolved Hide resolved
zip-0226.rst Outdated Show resolved Hide resolved
zip-0226.rst Outdated Show resolved Hide resolved
AntoineRondelet and others added 2 commits July 2, 2023 09:18
Co-authored-by: Vivek Arte <46618816+vivek-arte@users.noreply.github.com>
@vivek-arte
Copy link

Merged in more recent changes to the zsa1 branch so that the tables etc that were updated reflect here too.

Copy link

@vivek-arte vivek-arte left a comment

Choose a reason for hiding this comment

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

Approved for merging.


and use it to verify the `bindingSignature` on the `SIGHASH` message.
After computing :math:`\mathsf{bvk}`, the verifier MUST use it to verify the `bindingSignature` on the `SIGHASH` message.

Choose a reason for hiding this comment

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

This has been done in PR#30.

.. math:: \mathsf{bvk = (\sum cv_i^{net})} - \mathsf{ ValueCommit_0^{Orchard}(v^{balanceOrchard})} - \sum_{\mathsf{assetBurn}} \mathsf{ValueCommit_0^{OrchardZSA}(AssetBase, v^{AssetBase}) } = \sum \mathsf{rcv_{i,j}^{net}}\mathcal{R}^{\mathsf{Orchard}}
.. math:: \mathsf{bvk} = \mathsf{(\sum_{i=1}^{n} cv_i^{net})} - \mathsf{ValueCommit_0^{OrchardZSA}}(\mathsf{v^{balanceOrchard}}, \mathcal{V}^{\mathsf{Orchard}}) - \mathsf{\sum_{k=1}^{L} ValueCommit_0^{OrchardZSA}(v_{k}, AssetBase_{k})}

After computing :math:`\mathsf{bvk}`, the verifier MUST use it to verify the `bindingSignature` on the `SIGHASH` message.

Choose a reason for hiding this comment

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

This has been done in PR#30.

@vivek-arte vivek-arte merged commit 55d9a16 into zsa1 Jul 4, 2023
5 checks passed
PaulLaux pushed a commit that referenced this pull request Oct 4, 2023
This PR adds missing indices over sums. It also fixes and makes improvements to the burn mechanism description.

---------

Co-authored-by: Vivek Arte <46618816+vivek-arte@users.noreply.github.com>
vivek-arte added a commit that referenced this pull request Feb 12, 2024
This PR adds missing indices over sums. It also fixes and makes improvements to the burn mechanism description.

Co-authored-by: Vivek Arte <46618816+vivek-arte@users.noreply.github.com>
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.

None yet

3 participants