Skip to content

Conversation

@random-zebra
Copy link

@random-zebra random-zebra commented Nov 6, 2020

The size limits for a sapling transaction are currently checked in 3 places:
1- CheckTransaction (validation.cpp)
2- CheckTransactionWithoutProofVerification (called by SaplingValidation::CheckTransaction)
3- SaplingValidation::ContextualCheckTransaction

While in (2) and (3) the size is correctly checked against MAX_TX_SIZE_AFTER_SAPLING, in (1) it is checked against the old MAX_ZEROCOIN_TX_SIZE.
So a shielded transaction with size greater than MAX_ZEROCOIN_TX_SIZE would be wrongly rejected in CheckTransaction.

Fix the check in (1), selecting the correct max size for shielded/non-shielded transactions.
Move the size-check before calling SaplingValidation::CheckTransaction (as it's faster).
Remove the checks in (2) and (3) that are just redundant.

Based on top of:

@random-zebra
Copy link
Author

Rebased.

furszy
furszy previously approved these changes Nov 10, 2020
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

looking good 👍 , ACK c004841

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

ACK 3f80e84

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK 3f80e84

@Fuzzbawls Fuzzbawls merged commit c76192f into PIVX-Project:master Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants