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

docs: Transaction consensus rules: Size rules #3461

Merged
merged 6 commits into from
Feb 8, 2022

Conversation

oxarbitrage
Copy link
Contributor

Motivation

Document the transaction consensus rules. This pull request is about a category of them that we called transaction size rules. Will close #3426 if merged.

Solution

Document all the rules in the ticket, remove extra documentation and try to leave where possible just one text for each consensus rule.

Review

Anyone can review.

Reviewer Checklist

  • Consensus rule docs are in a good spot.
  • All the rules in the ticket were added or modified in this PR.
  • No piece of doc comment that i want to keep was deleted.

Follow Up Work

The other 2 sections of the transaction consensus rules which are in the following issues:

@oxarbitrage
Copy link
Contributor Author

I was not able to found this rule :

[Pre-Sapling] The encoded size of the transaction MUST be less than or equal to 100000 bytes.

We probably don't implement it because we checkpoint but i was wondering if this should be quoted anyway somewhere in the code with a note on why it is not enforced.

@teor2345
Copy link
Contributor

teor2345 commented Feb 3, 2022

I was not able to found this rule :

[Pre-Sapling] The encoded size of the transaction MUST be less than or equal to 100000 bytes.

We probably don't implement it because we checkpoint but i was wondering if this should be quoted anyway somewhere in the code with a note on why it is not enforced.

I suggest quoting it where we enforce the block size limit on transactions. (Since transactions must get mined into a block to be useful, we reject transactions that are larger than blocks.)

@codecov
Copy link

codecov bot commented Feb 3, 2022

Codecov Report

Merging #3461 (dcf7bac) into main (499ae89) will increase coverage by 0.15%.
The diff coverage is 77.02%.

❗ Current head dcf7bac differs from pull request most recent head 0ca6751. Consider uploading reports for the commit 0ca6751 to get more accurate results

@@            Coverage Diff             @@
##             main    #3461      +/-   ##
==========================================
+ Coverage   78.34%   78.49%   +0.15%     
==========================================
  Files         267      273       +6     
  Lines       31526    31779     +253     
==========================================
+ Hits        24698    24946     +248     
- Misses       6828     6833       +5     

@oxarbitrage
Copy link
Contributor Author

Thank you @teor2345 , i quoted the missing rule with some notes at 4089e5a

Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Looks good, I've checked all rules and only have a question about the last one

zebra-chain/src/sapling/shielded_data.rs Outdated Show resolved Hide resolved
Co-authored-by: teor <[email protected]>
@teor2345
Copy link
Contributor

teor2345 commented Feb 7, 2022

This PR failed due to an unrelated timing issue in the acceptance tests:

---- zebra_zcash_listener_conflict stdout ----
Error:
0: conflicted node2 was still running, but the test expected a panic

https://github.com/ZcashFoundation/zebra/runs/5099762771?check_suite_focus=true#step:13:1742

I don't think we have a ticket for this failure, I haven't seen it in months.

Edit: this is now #3489

teor2345
teor2345 previously approved these changes Feb 7, 2022
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.

Thanks, looks good to me!

@teor2345 teor2345 requested a review from conradoplg February 8, 2022 00:18
Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Looks good! Nightly test is failing, not sure if it's intermittent, I'll retry it

@mergify mergify bot merged commit 29ad801 into ZcashFoundation:main Feb 8, 2022
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.

Document consensus rules from Zcash spec 7.1.2: Transaction Size
3 participants