Skip to content

Conversation

@apoelstra
Copy link
Member

This PR has three changes which are mostly unrelated except that they were all found when fuzzing my "rewrite expression parser to be nonrecursive" branch against 12.x.

…= 12.x

See rust-bitcoin#734 for
discussion of this. Meanwhile, add a unit test so we can determine when
the behavior changes.
When fuzzing my new non-recursive tree parsing logic, I noticed that we
were deviating from the released 12.0 in the way that we display l:0.
This is an ambiguous fragment which can be displayed either as l:0 or
u:0. In our released code we use u:0 so stick with that.

This was unintentially changed as part of rust-bitcoin#722. Change it back.

While we are at it add a regression test for rust-bitcoin#735
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 1259375

Copy link
Member Author

@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.

Successfully ran local tests on 1259375.

@apoelstra apoelstra merged commit b11cdc2 into rust-bitcoin:master Sep 3, 2024
@apoelstra apoelstra deleted the 2024-09--compat branch September 3, 2024 17:25
heap-coder added a commit to heap-coder/rust-miniscript that referenced this pull request Sep 27, 2025
…things

1259375d7b7c91053e09d1cbe3db983612fe301c miniscript: make display prefer 'u' over 'l' in the fragment l:0 (Andrew Poelstra)
67fdc506dfdfd7c3c35108fbd30010fda9a4a2eb descriptor: reject strings of the form "tr(<key>,)" (Andrew Poelstra)
00cac40ef68a0c74e40e9a252000f21dfb82c58a descriptor: add unit test demonstrating sanity-checking behavior in <= 12.x (Andrew Poelstra)

Pull request description:

  This PR has three changes which are mostly unrelated except that they were all found when fuzzing my "rewrite expression parser to be nonrecursive" branch against 12.x.

  * First is a unit test demonstrating #734. It doesn't fix anything, just tests the current behavior.
  * Second is a fix for #736 (backported in #735).
  * Third tweaks the new `Display` code from #722 to change how the ambiguous `l:0`/`u:0` is serialized, to match 12.x.

ACKs for top commit:
  sanket1729:
    ACK 1259375d7b7c91053e09d1cbe3db983612fe301c

Tree-SHA512: 921d65a1efd49bda0f9db488a2854b004e14518f584d832497a9dbc13a845ceec99544375385570c6ac42d4985277e8dcbb3aa8654de93235cf9067ba601f91d
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