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

Track tree depth to avoid stack overflow #704

Merged
merged 3 commits into from
Jul 9, 2024

Conversation

sanket1729
Copy link
Member

We track tree depth at the time of parsing, but that is insufficient because wrappers also count towards the stack limit when creating Miniscript structure.

We could also fix this by fixing the wrappers to be limited to size 10 or something(wrappers are only for transformation of types, they don't provide any additional functionality).

Long term we want to move towards to complete non-recursive parsing, this is a good stop gap solution to address the current issues.

@apoelstra
Copy link
Member

merged #697 -- can you rebase this?

@sanket1729 sanket1729 force-pushed the sanketk/24-07/dos-bug branch from 855835b to dfabe2d Compare July 8, 2024 18:58
@apoelstra
Copy link
Member

Looks like the new test is failing.

@sanket1729 sanket1729 force-pushed the sanketk/24-07/dos-bug branch from dfabe2d to 14f0d7c Compare July 8, 2024 20:26
@sanket1729
Copy link
Member Author

@apoelstra, the new test was not testing the correct thing :) . This is now fixed. 🤞 CI passes this time.

@sanket1729
Copy link
Member Author

@apoelstra ready for review.

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 14f0d7c

@apoelstra apoelstra merged commit 5b0f5e3 into rust-bitcoin:master Jul 9, 2024
8 checks passed
@apoelstra
Copy link
Member

Needs backport.

@apoelstra
Copy link
Member

Opened #705 to release new 12.x version.

apoelstra added a commit that referenced this pull request Jul 14, 2024
fd823e8 release 11.1.0 (Andrew Poelstra)
0d28ebc Add offending test case (Sanket Kanjalkar)
351b192 Track miniscript tree depth explicitly (Sanket Kanjalkar)

Pull request description:

ACKs for top commit:
  apoelstra:
    ACK fd823e8

Tree-SHA512: 643309ed7b0f3c32b52c9fba59b6973c3a3f017b2f5f083c6623205e40f641c707a4aab469f00d48975df100604bc109aa1005abe8ec1d9e4d132b9b0e38c46b
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