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

add tests for script deserialization #63

Closed

Conversation

joemphilips
Copy link
Contributor

While I was trying to port this library into C# , I found a bug in script deserialization.

I was trying to fix it by myself, but I'm afraid that the spec will change again. So I will not do it for now (unless we are sure that the spec won't change drastically anymore.)

Here is the failing case, tell me if I'm doing something wrong.

@apoelstra
Copy link
Member

Thanks for this report - sorry for letting it drop. I will try to investigate today.

@apoelstra
Copy link
Member

offending script:
OP_IF OP_SIZE OP_PUSHBYTES_1 20 OP_EQUALVERIFY OP_HASH256 OP_PUSHBYTES_32 131772552c01444cd81360818376a040b7c3b2b7b0a53550ee3edde216cec61b OP_EQUAL OP_ELSE OP_0 OP_ENDIF OP_VERIFY OP_SIZE OP_PUSHBYTES_1 20 OP_EQUALVERIFY OP_SHA256 OP_PUSHBYTES_32 ec4916dd28fc4c10d78e287ca5d9cc51ee1ae73cbfde08c6b37324cbfaac8bc5 OP_EQUALVERIFY OP_PUSHNUM_1

or 6382012088aa20131772552c01444cd81360818376a040b7c3b2b7b0a53550ee3edde216cec61b876700686982012088a820ec4916dd28fc4c10d78e287ca5d9cc51ee1ae73cbfde08c6b37324cbfaac8bc58851

@apoelstra
Copy link
Member

Yep, I see ... the problem is that when we are parsing backward after seeing VERIFY, we parse OP_EQUAL and think we're in a pubkeyhash construction, but we're actually in a hashlock construction. We need to split up Terminal::PkH into a NonTerminal::PkHOrHashLock so that we can differentiate these cases.

Great find!

@kiminuo kiminuo mentioned this pull request Feb 24, 2020
apoelstra pushed a commit that referenced this pull request Jul 6, 2020
@sanket1729 sanket1729 mentioned this pull request Jul 6, 2020
@apoelstra
Copy link
Member

This PR needs an update to rename c:pk to pk, thresh_m to multi, and to use Segwitv0Script rather than Miniscript::<bitcoin::PublicKey>.

It still does not pass by the way, even with these fixes.

@sanket1729
Copy link
Member

sanket1729 commented Aug 28, 2020

@apoelstra This test has been merged by #78 / #106 and this can be closed.

joemphilips pushed a commit to joemphilips/rust-miniscript that referenced this pull request Dec 9, 2022
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