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: sync max multi_a pubkey with core #746

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

ChrisCho-H
Copy link
Contributor

Although this is checked indirectly by checking MAX_STACK_SIZE, it'd be better to check in threshold explicitly as core does.

@apoelstra
Copy link
Member

o.O I had no idea this limit existed.

Great find!

@apoelstra
Copy link
Member

apoelstra commented Sep 9, 2024

A few extra thoughts:

  • The "max stack size" limit is enforced during sanity checking (and can therefore be disabled and eventually adjusted) while the threshold limits are enforced during parsing and in the Rust type system (cannot be adjusted)
  • The threshold limits also provide a hard barrier on how much memory an individual Miniscript node can take and I'd like them to be as small as possible. (Though this is a bit silly since we have no limit on the thresh combinator...maybe we should, on the basis that and ADD each take at least one byte and each child takes at least one byte, it could be limited to 2MB? Not sure that a 2-meg limit is much more useful than no limit at all.)

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 96d8662 successfully ran local tests

@apoelstra
Copy link
Member

Will let @sanket1729 take a quick look before merging.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK 96d8662

@apoelstra apoelstra merged commit 4d913ee into rust-bitcoin:master Sep 10, 2024
30 checks passed
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.

3 participants