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

Adding substitution of pkh for taproot #589

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

Harshil-Jani
Copy link
Contributor

Recently , We have introduced the method substitute_raw_pkh to deal with the change of pkh to expr_raw_pkh. This is the implementation of that method for the taproot miniscript.
Reference PR : #557

@apoelstra
Copy link
Member

concept ACK. Though it took me quite a while to figure out what's going on. I think we should add a comment explaining that we're replacing RawPkH with PkH, which doesn't change the script in any way, but does attach the actual public key to the pkh fragment.

I was also confused that pkh was still supported in Taproot, but I was just confused, I don't think we need to add any comments to clarify that.

Finally, I don't know what the key origins table is that you're taking keys from -- I can't find it in BIP 174 or 370, at least not with the name "origin". So I'm unsure whether this is too magical...it may be that this causes certain workflows to work, but in such a way it'll stop working as soon as the set of PkHs and the key origin table stop being in sync, and this is such a nonobvious connection that it'd be a nightmare to debug. Unsure.

@Harshil-Jani Harshil-Jani force-pushed the substitute-taproot branch 2 times, most recently from 54ad691 to f7eb8cb Compare August 8, 2023 20:11
Signed-off-by: Harshil Jani <harshiljani2002@gmail.com>
@Harshil-Jani
Copy link
Contributor Author

Harshil-Jani commented Aug 8, 2023

concept ACK. Though it took me quite a while to figure out what's going on. I think we should add a comment explaining that we're replacing RawPkH with PkH, which doesn't change the script in any way, but does attach the actual public key to the pkh fragment.

I was also confused that pkh was still supported in Taproot, but I was just confused, I don't think we need to add any comments to clarify that.

Finally, I don't know what the key origins table is that you're taking keys from -- I can't find it in BIP 174 or 370, at least not with the name "origin". So I'm unsure whether this is too magical...it may be that this causes certain workflows to work, but in such a way it'll stop working as soon as the set of PkHs and the key origin table stop being in sync, and this is such a nonobvious connection that it'd be a nightmare to debug. Unsure.

We need to satisfy or dissatisfy the key and thus we need all the given key from a descriptor . tap_key_origin is the only field which consist of all the keys added on a descriptor and thus we try to get keys from it. I am also not sure about why origin has all keys and not some other field.

edit : I have added more clear comment on this.

@sanket1729
Copy link
Member

@apoelstra, in update_psbt_input, we use the key_origin_feilds (PSBT_IN_TAP_BIP32_DERIVATION) to inform the signer which keys we want it to sign for.

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.

utACK c6bb142

@apoelstra
Copy link
Member

@apoelstra, in update_psbt_input, we use the key_origin_feilds (PSBT_IN_TAP_BIP32_DERIVATION) to inform the signer which keys we want it to sign for.

Ah, that's right! Ok, with that in mind, I think it make sense to also use this set of keys to obtain public keys, since those are morally pretty similar to bip32 origins.

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 c6bb142

@apoelstra apoelstra merged commit d1c61e4 into rust-bitcoin:master Aug 8, 2023
16 checks passed
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.

None yet

3 participants