Push key xonly

[mplsgrant]

Issue

I can not use XOnlyPublicKey in my Scripts which prevents me from working with Taproot.

Cause

The current version of script::Builder does not accept XOnlyPublicKeys.

Solution

So, I created a function push_xkey(self, key: &XOnlyPublicKey) based on the existing push_key function. I also augmented an existing test in an attempt to reach testing parity with existing code.

After toying around with push_xkey, it seems to work on my end.

[dr-orlovsky]

Concept NACK [changed my mind: see rationale next comment]: XonlyPubkeys should not be used outside of TapScript context and the crate should prevent it. This once again brings the question of separating different script types. I do agree with @mplsgrant that having this functionality is indeed needed.

@mplsgrant the only current solution (before we refactor script types here) is to compose Taproot scripts using miniscript crate via Tr descriptor. I already have a wallet basing on it and it perfectly works for key- and script-path spendings where script path spendings involve multisigs.

[dr-orlovsky]

Having a second thought, I think there is no reasonably fast way forward with this issue: introduction of a proper script type system (which will be discussed here #921) will take many months (if not year+), and preventing library users from being able to construct tapscripts without the need to use miniscript is bad.

On the other hand, adding push_xonly to the script builder will just add to already existing harm, since we anyway can compose incorrect/invalid scripts with the existing Builder structure (like pushing uncompressed key in witness scripts).

[dr-orlovsky]

ACK 2ac73aa

[sanket1729]

concept ACK. Left a few comments

[apoelstra]

concept ACK but I'd also liek the method to be renamed.

[dr-orlovsky]

ACK 03b1305

[sanket1729]

Sorry for nit-picking. Can you please squash so that there are only 2 commits. One adding x_only_public_key. One adding test.

[sanket1729]

I can not use XOnlyPublicKey in my Scripts which prevents me from working with Taproot.

FWIW, you can call

bulder.push_slice(&x_only_key.serialize())

This is what I am doing for rust-miniscript. But this is a cleaner solution.

[mplsgrant]

Sorry for nit-picking. Can you please squash so that there are only 2 commits. One adding x_only_public_key. One adding test.

@sanket1729 The squash ended up a little messier that I would have liked. Would you like me to revert the commits and create two fresh ones?

[sanket1729]

Sure. As long as there are two clean commits, It doesn't matter how you did it :)

[mplsgrant]

I believe we now have 2 clean commits.

[tcharding]

Thanks for the contribution @mplsgrant! This comment is just an FYI, no changes to the PR needed it LGTM. I'm explaining here for both of our benefits :)

This PR should technically be single commit because the unit test tests new functionality added in the PR.

Can you please squash so that there are only 2 commits. One adding x_only_public_key. One adding test.

This is typically how we do bug fixes, the reason that the test is in a separate patch is so that reviewers can apply the test patch to master and verify that it really fails and therefore the fix really fixes something.

[sanket1729]

This PR should technically be a single commit because the unit test tests new functionality added in the PR.

Indeed, the initial PR had two commits. This is why I suggested two commits, but you are right. One commit is also good.

I would like to have similar consistency for naming x_only vs xonly as discussed in rust-secp convenience methods PR. But we can do that later. There is no need to hold this PR hostage till that resolves.

If we get it this before release, it's good. If not, it's not a biggie, as there is a simple work-around.

I can not use XOnlyPublicKey in my Scripts which prevents me from working with Taproot.

FWIW, you can call

bulder.push_slice(&x_only_key.serialize())

This is what I am doing for rust-miniscript. But this is a cleaner solution.

[sanket1729]

ACK e8c7c14. I would have preferred if you only tested the x_only_key in the test instead of copying all the unnecessary code from the previous test.

If you plan to squash these into one commit as @tcharding suggested, I suggest also fixing so that we only test the required function and don't double-test things with a bunch of unnecessary code.

[sanket1729]

note to reviewers, the already existing function is called push_key (not push_public_key). Therefore, it makes sense to call this push_x_only_key.

[sanket1729]

modulo I really dislike having multiple function calls separated with ; in a single line, even in test

This was already existing in the previous test code. We should fix that together.

[sanket1729]

ACK 8a042dd

[dr-orlovsky]

Revoking my previous ACK. I think the test case should be improved (see the comment) to be less confusing.

[sanket1729]

You might need to squash again :( @mplsgrant

[sanket1729]

utACK 83aaa0f

[dr-orlovsky]

Needs fix due to a failed CI

[dr-orlovsky]

@mplsgrant and squash commits again - we must have each commit passing CI, this is the policy of the repo (https://github.com/rust-bitcoin/rust-bitcoin/blob/master/CONTRIBUTING.md#preparing-prs says "each commit within a PR must compile and pass unit tests with no errors")

[dr-orlovsky]

@mplsgrant and the fix is invalid: when I suggest code changes I give a hint, but I have no way of running my code. Please try to compile it locally and make it working.

[mplsgrant]

Thanks for code hint @dr-orlovsky.
The as_ref() angle required type inference that I could not quickly resolve. Instead, I went with a different approach that I imagine will make the compilers happy.

[dr-orlovsky]

ACK f27c4a5

Thank you very much for going through all these iterations

[mplsgrant]

You're welcome! Thank you for all the feedback and guidance. I appreciate it.

[sanket1729]

utACK f27c4a5. Thanks a lot for keeping up the iterations with prompt responses

[mplsgrant]

You're welcome. I appreciate what you folks are doing here.