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.

Sign up for GitHub

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

Jump to bottom

descriptors: define script_code() for legacy outputs #127

Merged
merged 1 commit into from
Oct 1, 2020
Merged

descriptors: define script_code() for legacy outputs #127

merged 1 commit into from
Oct 1, 2020

Conversation

darosior
Copy link
Contributor

@darosior darosior commented Aug 28, 2020 •
edited
Loading

This fixes my confusion in #123 that @apoelstra pointed out.

@darosior
Copy link
Contributor Author

I still need to push some tests for legacy but it's based on https://github.com/bitcoin/bitcoin/blob/1cf73fb8eba3b89a30d5e943deaec5052a8b21b7/src/script/interpreter.cpp#L1586: what is called scriptCode in EvalScript is the previous txo's scriptPubKey.

For witness programs it's special cased in VerifyWitnessProgram as the witness script for P2SWSH and as a P2PKH-like scriptPubKey for P2WPKH.

@darosior darosior force-pushed the legacy_do_have_scriptcode branch from 236bcf5 to 5718bea Compare August 28, 2020 20:35
@sanket1729
Copy link
Member

sanket1729 commented Aug 28, 2020 •
edited
Loading

Now, this function is same as witness_script() . So, I guess it is simpler to define this to alias this to witness_script call?

@darosior
Copy link
Contributor Author

No it's not for wpkh, but now you pointed it out i wonder why is witness_script() defined for legacy and wpkh outputs since i assumed witnessScript meant "redeemScript but for P2WSH".

@sanket1729
Copy link
Member

sanket1729 commented Aug 28, 2020 •
edited
Loading

Ah yes, you are right! I think the API(and the doc-comment) for witness_script is indeed incorrect. Waiting to @apoelstra to confirm

@apoelstra
Copy link
Member

Yep, the doccomment for witness_script is wrong

apoelstra
apoelstra reviewed Sep 10, 2020
View reviewed changes
apoelstra
apoelstra reviewed Sep 10, 2020
View reviewed changes
src/descriptor/mod.rs
// The item 5:
// - For P2WPKH witness program, the scriptCode is `0x1976a914{20-byte-pubkey-hash}88ac`.
Descriptor::Wpkh(ref pk) | Descriptor::ShWpkh(ref pk) => {
let addr = bitcoin::Address::p2pkh(&pk.to_public_key(), bitcoin::Network::Bitcoin);
Ok(addr.script_pubkey())
addr.script_pubkey()
Copy link
Member

Choose a reason for hiding this comment

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

When we update rust-bitcoin to get rust-bitcoin/rust-bitcoin#387 we won't need to do this silly round-trip-through-Address business.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can it make it into rust-bitcoin before the upcoming version bump ?

Copy link
Member

Choose a reason for hiding this comment

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

Before which version bump?

Copy link
Contributor Author

@darosior darosior Sep 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

rust-bitcoin ? Nevermind got my answer rust-bitcoin/rust-bitcoin#475 :)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah so let's get rust-bitcoin bumped, then we'll update rust-miniscript and then update this PR :)

Copy link
Member

Choose a reason for hiding this comment

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

I think actually we shuold merge this now then do a general fixup where we remove all the uses of bitcoin::Address in this crate (there are a few of them) when we were just trying to compute scriptpubkeys.

@darosior
descriptors: define script_code() for legacy outputs
8582c17 
This fixes the incorrect assumption made in
283676e that the `scriptCode` was
defined in bip-0143.

Reported-by: Andrew Poelstra <apoelstra@wpsoftware.net>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
@darosior darosior force-pushed the legacy_do_have_scriptcode branch from 5718bea to 8582c17 Compare September 10, 2020 17:33
@darosior
Copy link
Contributor Author

FWIW this semantic divergence wrt witness_script seems to be about to be resolved as bitcoind is likely to call it exec_script soon :-)

apoelstra
apoelstra approved these changes Oct 1, 2020
View reviewed changes
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 8582c17

@apoelstra apoelstra merged commit efd971b into rust-bitcoin:master Oct 1, 2020
@sanket1729 sanket1729 mentioned this pull request Oct 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@darosior @sanket1729 @apoelstra