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

Implement transaction::Verifier rejection tests for Sapling #2426

Closed
8 tasks
jvff opened this issue Jul 1, 2021 · 2 comments
Closed
8 tasks

Implement transaction::Verifier rejection tests for Sapling #2426

jvff opened this issue Jul 1, 2021 · 2 comments
Labels
C-enhancement Category: This is an improvement

Comments

@jvff
Copy link
Contributor

jvff commented Jul 1, 2021

Motivation

PR #2419 added some transaction::Verifier tests that check if a transaction with Sapling spends and outputs is accepted. However, no tests that check if the verifier is rejecting transactions was implemented. This prevents detection of consensus regression bugs that cause invalid transactions to be erroneously accepted.

Specifications

Sapling spend consensus rules:

• Elements of a Spend description MUST be valid encodings of the types given above.
cv and rk MUST NOT be of small order, i.e.[ℎ_J] cv MUST NOT be 𝒪_J and [ℎ_J]rk MUST NOT be 𝒪_J.
• The proof 𝜋_ZKSpend MUST be valid given a primary input formed from the other fields except spendAuthSig — i.e. ZKSpend.Verify(︀(cv, rt^Sapling, nf, rk), 𝜋_ZKSpend)︀ = 1.
• Let SigHash be the SIGHASH transaction hash of this transaction, not associated with an input, as defined in § 4.10 ‘SIGHASH Transaction Hashing’ on p. 47 using SIGHASH_ALL.
The spend authorization signature MUST be a valid SpendAuthSig^Sapling signature over SigHash using rk as the validating key — i.e. SpendAuthSig^Sapling.Validate_rk(SigHash,spendAuthSig) = 1.
[NU5 onward] As specified in§ 5.4.7 ‘RedDSA, RedJubjub, and RedPallas’ on p. 88, the validation of the 𝑅 component of the signature changes to prohibit non-canonical encodings.

Sapling output consensus rules:

• Elements of an Output description MUST be valid encodings of the types given above.
cv and epk MUST NOT be of small order, i.e. [ℎ_J] cv MUST NOT be 𝒪_J and [ℎ_J] epk MUST NOT be 𝒪_J.
• The proof 𝜋_ZKOutput MUST be valid given a primary input formed from the other fields except C^enc and C^out — i.e. ZKOutput.Verify(︀(cv, cm_𝑢, epk), 𝜋_ZKOutput)︀= 1.

Designs

An initial idea for testing would be to add the following test cases:

  • Small order spend cv
  • Small order spend rk
  • Invalid PI_ZKSpend
  • Invalid spend authorization signature
  • Small order output cv
  • Small order output wpk (epk?)
  • Invalid PI_ZKOutput
  • Invalid binding signature

One important thing to consider is if signing or generating ZK proofs for the test cases take too long to run. If that is the case, it might be best to write the code to generate the transactions to be rejected and store them serialized in the repository, and just load and deserialize them during testing. However, it will probably be good to keep the code to generate the serialized transactions some place where it can be periodically executed to regenerate the transactions and guarantee that the generation code doesn't break as well.

@jvff jvff added C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage labels Jul 1, 2021
@teor2345 teor2345 added this to the 2021 Sprint 15 milestone Jul 1, 2021
@teor2345
Copy link
Contributor

teor2345 commented Jul 1, 2021

I'm going to put this in sprint 15, so we remember to write tests.

We should also get one of our cryptographers to check that there are no existing tests for these failure cases. (It might be good practice to pass the same test vectors to the transaction verifier anyway.)

@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Jul 5, 2021
@teor2345
Copy link
Contributor

I'm going to move this out of our current sprints, because extra tests aren't on our critical path.

@teor2345 teor2345 removed this from the 2021 Sprint 15 milestone Jul 22, 2021
@jvff jvff closed this as completed Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: This is an improvement
Projects
None yet
Development

No branches or pull requests

3 participants