Skip to content

psbt: satisfy any ToPublicKey key #137

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

Merged
merged 1 commit into from
Oct 1, 2020

Conversation

darosior
Copy link
Contributor

The satisfier was only implemented for bitcoin::PublicKey and could not be reimplemented for a more generalistic key downstream.

Also, it makes sense after #131 (i have a rebased version for my own needs here https://github.com/darosior/rust-miniscript/commits/satisfy_xpubs). All signatures are looked for by raw public key, and thus should be added to the mapping with to_public_key() (which derives the one pointed by the DescriptorPublicKey in #131).

Could have rebased it on #131 to add a test xpub-descriptor, but seemed overkill as the change is pretty straightforward.

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.

As discussed on IRC, we probably need to implement Satisfier on a structure that also has information for (nSequence, nLocktime) etc. The current implementation on psbt::Input does not have sufficient information to implement the satisfier for after and older.

We require the complete psbt to get the nSequence, nLockTime fields.

@sanket1729
Copy link
Member

sanket1729 commented Sep 30, 2020

I guess this PR is independent of the above issue, we can merge this

@apoelstra
Copy link
Member

I'm actually gonna concept NACK this, because it plays badly with Rust's trait coherency rules. It prevents users from implementing Satisfier themselves on their types if those types also implement MiniscriptKey.

@apoelstra
Copy link
Member

@darosior why do you say it "could not be reimplemented for a more generalistic key downstream"?

@darosior
Copy link
Contributor Author

darosior commented Oct 1, 2020

@darosior why do you say it "could not be reimplemented for a more generalistic key downstream"?

IIRC: As it's implemented for bitcoin::PublickKey which i use and is MiniscriptKey + ToPublicKey, i had an error trying to implement it for a <Pk: Miniscript + ToPublicKey> as it was considered to reimplement a foreign trait.

@apoelstra
Copy link
Member

Right, if you try to implement it for all Pk: Miniscript + ToPublicKey the compiler will prevent it because it may cause breakage with your other dependencies which might have conflicting implementations ...it's legal for us to do it in rust-miniscript but I think it's still likely to be a problem for users.

@sanket1729
Copy link
Member

@apoelstra Why do you think that that downstream user would need another implementation of Satisfier for psbt? As far as I can see, there can only be one way of doing it?

@apoelstra
Copy link
Member

Oh, oops, I misread this PR. I thought we were implementing on Pk itself, not on psbt::Input.

@apoelstra
Copy link
Member

concept ACK then :)

@darosior
Copy link
Contributor Author

darosior commented Oct 1, 2020

concept ACK then :)

:)

More generally, what about we either:

  • implement the default satisfier for every key, if downstream users want an exotic implementation they should use a newtype
  • don't implement the satisfier at all, leaving the implementation design up to the user

As the middle ground seems confusing to me (you likely don't need a specific implem for just one key type but rather for all MiniscriptKey + ToPublicKey keys).

@apoelstra
Copy link
Member

Is the first option what's being proposed in this PR?

@darosior
Copy link
Contributor Author

darosior commented Oct 1, 2020

Conceptually, i think so. Was not the original motivation though.

@apoelstra apoelstra merged commit c1a54ec into rust-bitcoin:master Oct 1, 2020
@sanket1729 sanket1729 mentioned this pull request Oct 1, 2020
@darosior darosior deleted the psbt_satisfy_everyone branch October 3, 2020 00:22
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