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 compilation of pk()-only policies into tr() descriptors #677

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

shesek
Copy link
Contributor

@shesek shesek commented Apr 30, 2024

Before this fix, the added test failed with: Unexpected("Empty Miniscript compilation")

@tcharding
Copy link
Member

Thanks for the contribution!

Can you put the unit test as a separate patch after the fix please so that during review I can swap them around and verify the failure.

@shesek shesek force-pushed the 202404-tr-pk-only branch from 956f482 to bb6347f Compare April 30, 2024 21:02
@shesek
Copy link
Contributor Author

shesek commented Apr 30, 2024

@tcharding Yes for sure, I updated my branch.

@tcharding
Copy link
Member

Bother, the CI fail is unrelated to this PR. I hit the same thing elsewhere yesterday, I'll push a PR to fix it (it requires dependency pinning).

@tcharding
Copy link
Member

I verified the unit test does indeed fail and the fix does fix the test. I have no view on the sanity of the change, requires @apoelstra or @sanket1729 who actually know this crate. I'm just the code monkey :)

Thanks for the contribution.

@sanket1729
Copy link
Member

It's been a while since I looked at MSRV things. What is the current rust-bitcoin MSRV?

The CI is failing at

error: package cc v1.0.96 cannot be built because it requires rustc 1.63 or newer, while the currently active rustc version is 1.56.1

@tcharding
Copy link
Member

Will need rebasing once #678 goes in.

Prior to this fix, the test failed with: `Unexpected("Empty Miniscript compilation")`
@shesek
Copy link
Contributor Author

shesek commented Sep 17, 2024

Rebased.

Note that #732 (bedb6d7) turned this from an Err to a panic.

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 ea3c523 successfully ran local tests; good find!

@apoelstra
Copy link
Member

I'm gonna go ahead and merge this. My local tests pass and I've been recently reading this code (hence my breaking it :)) so I'm fairly confident I understand it.

@apoelstra apoelstra merged commit 98104b9 into rust-bitcoin:master Sep 17, 2024
29 of 30 checks passed
shesek added a commit to shesek/minsc that referenced this pull request Oct 22, 2024
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.

4 participants