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

Sapling validation #3232

Merged
merged 4 commits into from May 10, 2018

Conversation

@ebfull
Contributor

ebfull commented May 1, 2018

Also review zcash/librustzcash#7

This is an attempt to tackle much of #3065

auto ctx = librustzcash_sapling_verification_ctx_init();
BOOST_FOREACH(const SpendDescription &spend, tx.vShieldedSpend) {

This comment has been minimized.

@str4d

str4d May 1, 2018

Contributor

Use for (const SpendDescription &spend : tx.vShieldedSpend) {

}
}
BOOST_FOREACH(const OutputDescription &output, tx.vShieldedOutput) {

This comment has been minimized.

@str4d

str4d May 1, 2018

Contributor

Use for (const OutputDescription &output : tx.vShieldedOutput) {

spend.anchor.begin(),
spend.nullifier.begin(),
spend.rk.begin(),
&spend.zkproof[0],

This comment has been minimized.

@str4d

str4d May 1, 2018

Contributor

spend.zkproof.begin(),

spend.nullifier.begin(),
spend.rk.begin(),
&spend.zkproof[0],
&spend.spendAuthSig[0],

This comment has been minimized.

@str4d

str4d May 1, 2018

Contributor

spend.spendAuthSig.begin(),

output.cv.begin(),
output.cm.begin(),
output.ephemeralKey.begin(),
&output.zkproof[0]

This comment has been minimized.

@str4d

str4d May 1, 2018

Contributor

output.zkproof.begin()

if (!librustzcash_sapling_final_check(
ctx,
tx.valueBalance,
&tx.bindingSig[0]

This comment has been minimized.

@str4d

str4d May 1, 2018

Contributor

tx.bindingSig.begin()

This comment has been minimized.

@str4d

str4d May 1, 2018

Contributor

Missing sighashValue.

@str4d str4d referenced this pull request May 2, 2018

Closed

Implement Sapling consensus rules #3065

25 of 25 tasks complete

@str4d str4d added this to In progress in Consensus Protocol Team May 3, 2018

@str4d str4d added this to the v1.1.1 milestone May 3, 2018

@braddmiller braddmiller moved this from In progress to In Review in Consensus Protocol Team May 8, 2018

@Eirik0

Is it possible to write tests for this?

@ebfull

This comment has been minimized.

Contributor

ebfull commented May 8, 2018

Is it possible to write tests for this?

Right now there's no way for us to do this until we get some wallet PRs written.

@ebfull

This comment has been minimized.

Contributor

ebfull commented May 8, 2018

@zkbot try

zkbot added a commit that referenced this pull request May 8, 2018

Auto merge of #3232 - ebfull:3207-sapling-validation, r=<try>
Sapling validation

**Also review zcash/librustzcash#7

This is an attempt to tackle much of #3065
@zkbot

This comment has been minimized.

Contributor

zkbot commented May 8, 2018

⌛️ Trying commit 1fad2d9 with merge ba661e5...

@ebfull

This comment has been minimized.

Contributor

ebfull commented May 8, 2018

I just made a change that considerably reduces the diff.

Basically, since CheckTransaction does guarantee empty vjoinsplit / vShieldedSpend / vShieldedOutput, we don't need an unnecessary IsCoinbase() check. We can treat the transaction like normal and it's a no-op.

Thus, no need for extra nesting, and it looks cleaner.

@ebfull

This comment has been minimized.

Contributor

ebfull commented May 8, 2018

@zkbot try

@zkbot

This comment has been minimized.

Contributor

zkbot commented May 8, 2018

⌛️ Trying commit b4db32f with merge a07da1e...

zkbot added a commit that referenced this pull request May 8, 2018

Auto merge of #3232 - ebfull:3207-sapling-validation, r=<try>
Sapling validation

**Also review zcash/librustzcash#7

This is an attempt to tackle much of #3065
@zkbot

This comment has been minimized.

Contributor

zkbot commented May 8, 2018

☀️ Test successful - pr-try
State: approved= try=True

@str4d

str4d approved these changes May 8, 2018

utACK. I concur that we can't write tests for this until we have the ability to create Sapling proofs, which this PR does not include. We must write tests in the PR that adds Sapling proving.

@ebfull ebfull referenced this pull request May 9, 2018

Merged

Sprout on Groth16 #3251

@bitcartel

Looks good...

Some questions about error handling when calling librustzcash.

What if rust panics, which could occur here:
https://github.com/zcash/librustzcash/blob/5e220695e5961c8619a1095a3b9022509c6c9b9d/src/rustzcash.rs#L302

What kind of C++ exception gets thrown? Should it be caught?

Basically, is there a situation where librustzcash_sapling_verification_ctx_free(ctx); fails to be invoked if the librustzcash call fails?

@ebfull

This comment has been minimized.

Contributor

ebfull commented May 10, 2018

Assertions in Rust code will abort the process. Nothing in the Rust code should assert things that would otherwise be recoverable.

@bitcartel bitcartel self-requested a review May 10, 2018

@bitcartel

This comment has been minimized.

Contributor

bitcartel commented May 10, 2018

Is there a card up for "We must write tests in the PR that adds Sapling proving." ?

@Eirik0

Eirik0 approved these changes May 10, 2018

@Eirik0

This comment has been minimized.

Contributor

Eirik0 commented May 10, 2018

utACK

@str4d

This comment has been minimized.

Contributor

str4d commented May 10, 2018

@zkbot

This comment has been minimized.

Contributor

zkbot commented May 10, 2018

📌 Commit b4db32f has been approved by str4d

zkbot added a commit that referenced this pull request May 10, 2018

Auto merge of #3232 - ebfull:3207-sapling-validation, r=str4d
Sapling validation

**Also review zcash/librustzcash#7

This is an attempt to tackle much of #3065
@zkbot

This comment has been minimized.

Contributor

zkbot commented May 10, 2018

⌛️ Testing commit b4db32f with merge ae6c258...

@zkbot

This comment has been minimized.

Contributor

zkbot commented May 10, 2018

☀️ Test successful - pr-merge
Approved by: str4d
Pushing ae6c258 to master...

@zkbot zkbot merged commit b4db32f into zcash:master May 10, 2018

1 check passed

homu Test successful
Details

Consensus Protocol Team automation moved this from In Review to Complete PRs (Ignore) May 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment