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

(Some) semantic transaction verification checks #1174

Merged
merged 7 commits into from
Oct 29, 2020
Merged

Conversation

hdevalence
Copy link
Contributor

Follow on to #1173, supersedes #1100. See those PRs for details.

@hdevalence hdevalence requested a review from dconnolly October 16, 2020 23:08
@hdevalence hdevalence force-pushed the transaction-verifier-structure branch from 88503c0 to 84d2abd Compare October 16, 2020 23:10
@dconnolly dconnolly force-pushed the transaction-verifier-structure branch from 50182ad to dcd814d Compare October 20, 2020 05:36
Base automatically changed from transaction-verifier-structure to main October 20, 2020 15:16
@hdevalence
Copy link
Contributor Author

There were some comments on these changes as part of that PR: #1100 (comment)

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

It seems like we're missing the following semantic verification consensus rules:

Sorry for the mangled text, my PDF viewer doesn't seem to handle spaces in the Zcash Spec PDF.

•[Pre-Heartwood] Acoinbase transactionalsoMUST NOThave anyOutput descriptions.
•[Overwinteronward]nExpiryHeightMUSTbe less than or equal to 499999999.
•[Overwinteronward] If atransactionis not acoinbase transactionand itsnExpiryHeight�eld is nonzero,then itMUST NOTbe mined at ablock heightgreater than itsnExpiryHeight.
•[Saplingonward]valueBalanceMUSTbe in the range{−MAX_MONEY..MAX_MONEY}.
]
•[Heartwoodonward] AllSaplingoutputs incoinbase transactionsMUSTdecrypt to anote plaintext, i.e. theprocedure in§ 4.17.3‘Decryption using a Full Viewing Key (Sapling)’on p. 49 does not return⊥, using asequence of32zero bytes as theoutgoing viewing key.
•[Canopyonward] AnySaplingoutput of acoinbase transactiondecrypted to anote plaintextaccording tothe preceding ruleMUSThavenote plaintext lead byteequal to0x02. (This applies even during the “graceperiod” speci�ed in [ZIP-212].)

Are they on a tracking issue somewhere?

@dconnolly
Copy link
Contributor

It seems like we're missing the following semantic verification consensus rules:

Sorry for the mangled text, my PDF viewer doesn't seem to handle spaces in the Zcash Spec PDF.

•[Pre-Heartwood] Acoinbase transactionalsoMUST NOThave anyOutput descriptions.
•[Overwinteronward]nExpiryHeightMUSTbe less than or equal to 499999999.
•[Overwinteronward] If atransactionis not acoinbase transactionand itsnExpiryHeight�eld is nonzero,then itMUST NOTbe mined at ablock heightgreater than itsnExpiryHeight.
•[Saplingonward]valueBalanceMUSTbe in the range{−MAX_MONEY..MAX_MONEY}.
]
•[Heartwoodonward] AllSaplingoutputs incoinbase transactionsMUSTdecrypt to anote plaintext, i.e. theprocedure in§ 4.17.3‘Decryption using a Full Viewing Key (Sapling)’on p. 49 does not return⊥, using asequence of32zero bytes as theoutgoing viewing key.
•[Canopyonward] AnySaplingoutput of acoinbase transactiondecrypted to anote plaintextaccording tothe preceding ruleMUSThavenote plaintext lead byteequal to0x02. (This applies even during the “graceperiod” speci�ed in [ZIP-212].)

Are they on a tracking issue somewhere?

This is an incomplete set of checks, I think we want to merge the ones we have and then keep adding more one at a time to see if they break anything.

@dconnolly dconnolly changed the title Transaction checks (Some) semantic transaction verification checks Oct 22, 2020
@dconnolly
Copy link
Contributor

valueBalance MUST be in the range{−MAX_MONEY..MAX_MONEY}.

I think this is handled by Amount

@dconnolly dconnolly self-assigned this Oct 22, 2020
@teor2345 teor2345 mentioned this pull request Oct 22, 2020
4 tasks
@dconnolly dconnolly mentioned this pull request Oct 24, 2020
4 tasks
@dconnolly dconnolly added this to the Transaction Validation milestone Oct 27, 2020
@dconnolly dconnolly added A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code labels Oct 27, 2020
@dconnolly dconnolly requested a review from yaahc October 27, 2020 06:46
@dconnolly
Copy link
Contributor

(This is currently expected to fail the build until some of the error casting/impls are worked out)

@dconnolly dconnolly merged commit 0cb8010 into main Oct 29, 2020
@dconnolly dconnolly deleted the transaction-checks branch October 29, 2020 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants