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.

Sign up for GitHub

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

Jump to bottom

Chain: Test oversized scripts #387

Merged
merged 2 commits into from
Mar 6, 2025
Merged

Chain: Test oversized scripts #387

merged 2 commits into from
Mar 6, 2025
+192 −36

Conversation

Davidson-Souza
Copy link
Collaborator

What is the purpose of this pull request?

  • Bug fix
  • Documentation update
  • New feature
  • Test
  • Other:

Which crates are being modified?

  • floresta-chain
  • floresta-cli
  • floresta-common
  • floresta-compact-filters
  • floresta-electrum
  • floresta-watch-only
  • floresta-wire
  • floresta
  • florestad
  • Other: .

Description

This PR adds test cases with scripts manually crafted to be oversized and break on some boundary check. Some are meant to be barely valid, so we can sanity check our code.

To achieve this, a first step was to refactor verify_block_transactions, moving the tx validation to a new function. This will also be useful for checking mempool transactions in the future.

Notes to the reviewers

6d10b82 is move-only and should not change any behavior.

Checklist

  • I've signed all my commits
  • I ran just lint
  • I ran cargo test
  • I've checked the integration tests
  • I've followed the contribution guidelines
  • I'm linking the issue being fixed by this PR (if any)

@Davidson-Souza Davidson-Souza changed the title Chain: Test overside scripts Chain: Test oversized scripts Feb 27, 2025
JoseSK999
JoseSK999 suggested changes Feb 28, 2025
View reviewed changes
Copy link
Contributor

@JoseSK999 JoseSK999 left a comment

Choose a reason for hiding this comment

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

I love the change, minor suggestions

@Davidson-Souza Davidson-Souza mentioned this pull request Mar 5, 2025
Merged
21 tasks
@Davidson-Souza
Copy link
Collaborator Author

Updated with @JoseSK999 comments and added a test for #396

@Davidson-Souza
consensus: refactor verify_block_transactions
8d5de37 
This commit moves transaction-specific validation to a dedicated
function. This will be used to test our transaction validation logic,
and to verify mempool transactions in the future.

A new function, `verify_transaction` was created. It takes the UTXOs
map, a transaction, some flags and returns the input/output amounts or
an error, if any. It'll check the following:

  - The transaction doesn't spend more coins than it claims in the inputs
  - The transaction doesn't create more coins than allowed
  - The transaction has valid scripts
  - The transaction doesn't have duplicate inputs (TODO)
@Davidson-Souza
Copy link
Collaborator Author

Rebased with master

@Davidson-Souza
consensus: test overside scripts
cf8d988 
This commit adds some tests with huge scripts to see our validation is
working as expected. Those test cases were tested against core and we
check if floresta outputs the same thing as the former.
@Davidson-Souza
Copy link
Collaborator Author

Fixed some warnings too

@Davidson-Souza Davidson-Souza merged commit 005189e into vinteumorg:master Mar 6, 2025
8 checks passed
@JoseSK999
Copy link
Contributor

Nice!

Something to note: Technically if the UTXO that we spend has a >10KB script we won't have it in our accumulator (as dictated by proof_util::is_unspendable), so it should fail already on the utreexo side.

This now checks it as well on the verify_transaction side, but anyways it is inexpensive.

@Davidson-Souza
Copy link
Collaborator Author

Nice!

Something to note: Technically if the UTXO that we spend has a >10KB script we won't have it in our accumulator (as dictated by proof_util::is_unspendable), so it should fail already on the utreexo side.

This now checks it as well on the verify_transaction side, but anyways it is inexpensive.

Right. This utxo will not be inside the accumulator and therefore it's impossible to make a proof for it. But if it gets created on the same block, the utreexo side won't check it. So I think the current check is also useful.

@JoseSK999
Copy link
Contributor

Good proint!

jaoleal pushed a commit to jaoleal/Floresta that referenced this pull request Mar 7, 2025
@Davidson-Souza @jaoleal
Chain: Test oversized scripts (vinteumorg#387)
6aa726b 
* consensus: refactor verify_block_transactions

This commit moves transaction-specific validation to a dedicated
function. This will be used to test our transaction validation logic,
and to verify mempool transactions in the future.

A new function, `verify_transaction` was created. It takes the UTXOs
map, a transaction, some flags and returns the input/output amounts or
an error, if any. It'll check the following:

  - The transaction doesn't spend more coins than it claims in the inputs
  - The transaction doesn't create more coins than allowed
  - The transaction has valid scripts
  - The transaction doesn't have duplicate inputs

* consensus: test over sized scripts

This commit adds some tests with huge scripts to see our validation is
working as expected. Those test cases were tested against core and we
check if floresta outputs the same thing as the former.
jaoleal pushed a commit to jaoleal/Floresta that referenced this pull request Mar 7, 2025
@Davidson-Souza @jaoleal
Chain: Test oversized scripts (vinteumorg#387)
8d91d03 
* consensus: refactor verify_block_transactions

This commit moves transaction-specific validation to a dedicated
function. This will be used to test our transaction validation logic,
and to verify mempool transactions in the future.

A new function, `verify_transaction` was created. It takes the UTXOs
map, a transaction, some flags and returns the input/output amounts or
an error, if any. It'll check the following:

  - The transaction doesn't spend more coins than it claims in the inputs
  - The transaction doesn't create more coins than allowed
  - The transaction has valid scripts
  - The transaction doesn't have duplicate inputs

* consensus: test over sized scripts

This commit adds some tests with huge scripts to see our validation is
working as expected. Those test cases were tested against core and we
check if floresta outputs the same thing as the former.
jaoleal pushed a commit to jaoleal/Floresta that referenced this pull request Mar 7, 2025
@Davidson-Souza @jaoleal
Chain: Test oversized scripts (vinteumorg#387)
cbbcd22 
* consensus: refactor verify_block_transactions

This commit moves transaction-specific validation to a dedicated
function. This will be used to test our transaction validation logic,
and to verify mempool transactions in the future.

A new function, `verify_transaction` was created. It takes the UTXOs
map, a transaction, some flags and returns the input/output amounts or
an error, if any. It'll check the following:

  - The transaction doesn't spend more coins than it claims in the inputs
  - The transaction doesn't create more coins than allowed
  - The transaction has valid scripts
  - The transaction doesn't have duplicate inputs

* consensus: test over sized scripts

This commit adds some tests with huge scripts to see our validation is
working as expected. Those test cases were tested against core and we
check if floresta outputs the same thing as the former.
jaoleal pushed a commit to jaoleal/Floresta that referenced this pull request Mar 7, 2025
@Davidson-Souza @jaoleal
Chain: Test oversized scripts (vinteumorg#387)
4286473 
* consensus: refactor verify_block_transactions

This commit moves transaction-specific validation to a dedicated
function. This will be used to test our transaction validation logic,
and to verify mempool transactions in the future.

A new function, `verify_transaction` was created. It takes the UTXOs
map, a transaction, some flags and returns the input/output amounts or
an error, if any. It'll check the following:

  - The transaction doesn't spend more coins than it claims in the inputs
  - The transaction doesn't create more coins than allowed
  - The transaction has valid scripts
  - The transaction doesn't have duplicate inputs

* consensus: test over sized scripts

This commit adds some tests with huge scripts to see our validation is
working as expected. Those test cases were tested against core and we
check if floresta outputs the same thing as the former.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoseSK999 JoseSK999 JoseSK999 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Davidson-Souza @JoseSK999