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

fix: ensure ExecutionStack cannot exceed MAX_STACK_SIZE #65

Merged
merged 1 commit into from
Mar 7, 2022

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Nov 16, 2021

  • Check that ExecutionStack does not exceed MAX_STACK_SIZE (255) when decoding from bytes
  • Add size() to tari script required to limit the maximum number of script elements in transaction and block validation

@sdbondi sdbondi force-pushed the sb-tariscript-add-opcode-len branch from a18cd1f to 451dc49 Compare November 16, 2021 13:57
@sdbondi sdbondi changed the title feat: add opcode_len function feat: add size function to tari script Nov 16, 2021
@sdbondi sdbondi force-pushed the sb-tariscript-add-opcode-len branch from 451dc49 to a7bbd35 Compare November 16, 2021 14:02
@sdbondi sdbondi changed the title feat: add size function to tari script fix: ensure ExecutionStack cannot exceed MAX_STACK_SIZE Nov 16, 2021
@sdbondi sdbondi force-pushed the sb-tariscript-add-opcode-len branch 2 times, most recently from 27ebe1b to c46f768 Compare November 16, 2021 14:32
Copy link
Contributor

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

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

LGTM except for the off-by-one

src/script/stack.rs Outdated Show resolved Hide resolved
@sdbondi
Copy link
Member Author

sdbondi commented Nov 17, 2021

@CjS77 What are your thoughts on limiting the number of ops per script in tari crypto?
Looking at bitcoin, there is a 10000 byte limit on the script byte size, a limit of 520 items on the input stack, 1000 limit on items pushed by a script on the stack (script.h). Perhaps we should introduce similar limits? Alternatively we can add these limits to the base node validators.

@CjS77
Copy link
Contributor

CjS77 commented Nov 17, 2021

We have an implicit limit on ops since we limit the script size, don't we?

Edit: It seems we don't 😱 though I could have sworn I put a limit on the serialised script size in the consensus validation code. Either I forgot, or it's been removed; I don't feel like digging right now.

So we absolutely must put a limit on the script size. I would do it in the consensus code since we may want different rules for different chains. Testnet and Mainnet should match up; but experimental chains like Igor may need a different limit.

I would put the limit on the serialized length rather than straight opcode count because it's the serialized script that takes up precious space in the block pre-pruning.

Another reason not use a naiive opcode count is that opcodes are far from equal. An OP_PUSH_ZERO and a OP_CHECK_MULTI_SIG have vastly different CPU cycle costs.

So: serialized limit mitigates "block stuffing spam attacks", and mitigates expensive opcode attacks to some degree.

I don't know what the script limit should be, but it should be large enough to allow reasonable complicated smart contracts, but small enough to prevent block stuffing.

2048 bytes might be a good starting point and see whether this is too restrictive.

Next thing to look at is, What kind of expensive scripts can I write that fit in this limit? Hashes and sig checks are the most expensive operations, so we could run some benchmarks to test where performance starts to affect consensus.

Multisigs have a self-brake on them because we require the pubkeys to be part of the script, and not just pop them off the stack. So you can't just slap up 2000 multisig opcodes. The maximum would be m of 7 with a 2048 byte limit.

@sdbondi
Copy link
Member Author

sdbondi commented Nov 17, 2021

We currently don't impose a byte limit on the script in consensus, but makes sense to do it in validation rather than a hard coded limit in tari_crypto. 👍

@sdbondi sdbondi force-pushed the sb-tariscript-add-opcode-len branch from c46f768 to 0a2c285 Compare November 17, 2021 11:15
@CjS77
Copy link
Contributor

CjS77 commented Nov 17, 2021

We currently don't impose a byte limit on the script in consensus, but makes sense to do it in validation rather than a hard coded limit in tari_crypto. 👍

Agree. I was editing my last comment when you posted this.

aviator-app bot pushed a commit to tari-project/tari that referenced this pull request Dec 6, 2021
…#3640)

Description
---
- Adds consensus rules for maximum script byte size

Breaking changes:
If an existing script is larger than 2048, that block/output will fail to validate on upgraded nodes. I have not explicitly checked this but the largest scripts used currently on testnet are one-sided payments and they are well within the limit.

Motivation and Context
---
See [@CjS77 comment](tari-project/tari-crypto#65 (comment))

TODO: Ideally, in order to not negatively impact validation performance, we should not have to serialise the script again to gain its byte size.

How Has This Been Tested?
---
New unit test, basic manual test
@CjS77 CjS77 merged commit 1b74d94 into tari-project:main Mar 7, 2022
@sdbondi sdbondi deleted the sb-tariscript-add-opcode-len branch March 8, 2022 03:56
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.

2 participants