Add CheckScriptPushSize to validate script push data size

srvinii commented at 0:09 am on April 28, 2024: none

This pull request adds a new function CheckScriptPushSize to the transaction validation logic to ensure that the push data within script elements does not exceed the maximum allowed size, as defined by MAX_SCRIPT_ELEMENT_SIZE.

Motivation and Context

Scripts within transaction outputs are not currently checked for the size of their push data elements. This oversight could potentially allow for scripts with oversized data elements, which might introduce security risks and non-standard transactions into the blockchain. By enforcing this check, we ensure that all scripts adhere to the existing size constraints, improving the robustness of transaction validation.

Changes

Added CheckScriptPushSize function to tx_check.cpp.
Integrated this function into the CheckTransaction routine to validate each output’s scriptPubKey.

Testing

Unit tests have been updated to include tests for CheckScriptPushSize:

check_script_push_size_tests ensures that scripts with data elements within and beyond the allowed size are handled correctly.

Impact

This change does not affect the existing blockchain data but will apply to all new transactions, preventing the inclusion of any transactions that violate script size rules in future blocks.

Please review the changes and provide feedback.

Add CheckScriptPushSize to validate script push data size 292d052472

DrahtBot commented at 0:09 am on April 28, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Fix indentation 36ecdc5412

Fix test section 806b98dc77

sipa commented at 0:31 am on April 28, 2024: member

This is a consensus change, which means it need discussion and agreement from the community before it can be considered in this repository.

Further, as implemented, it applies to all scriptPubKeys, including historical ones. It’s entirely possible that that means it will reject the existing main chain.

That aside, I don’t understand the motivation here. UTXOs with oversized pushes in their scriptPubKeys are both non-standard to create and unspendable anyway already.

srvinii commented at 0:36 am on April 28, 2024: none

This is a consensus change, which means it need discussion and agreement from the community before it can be considered in this repository.

That aside, I don’t understand the motivation here. UTXOs with oversized pushes in their scriptPubKeys are unspendable anyway.

I understand your concern regarding the consensus nature of the change and the implications it could have without broad community agreement. The motivation behind introducing a check for oversized data pushes in scriptPubKeys was primarily to prevent the creation of unspendable UTXOs that can clutter the UTXO set, potentially leading to performance degradation over time.

sipa commented at 0:38 am on April 28, 2024: member

We could simply remove them from the UTXO set if such outputs started to make a meaningful impact. Since they’re already unspendable, that would not be a consensus change (see CScript::IsUnspendable).

srvinii commented at 0:42 am on April 28, 2024: none

We could simply remove them from the UTXO set if such outputs started to make a meaningful impact. Since they’re already unspendable, that would not be a consensus change.

Considering this alternative, I agree that pursuing a consensus change may not be necessary if the impact of these unspendable outputs can be managed through more straightforward means. I appreciate your insight into this matter and the practical solution you’ve proposed.

Would you recommend any specific strategies or best practices for identifying and managing such unspendable outputs within the UTXO set? I’d like to explore further options for addressing this issue effectively while minimizing the need for consensus adjustments.

DrahtBot added the label CI failed on Apr 28, 2024

DrahtBot commented at 1:04 am on April 28, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/24337665726