Skip to content

Conversation

@odudex
Copy link
Collaborator

@odudex odudex commented Jan 31, 2025

Following guidance from @pythcoiner , we have implemented support for the PSBT_IN_TAP_KEY_SIG field. This addition addresses previously anticipated requirements, as indicated in the related comments, ensuring compatibility with internal key-signed Taproot PSBTs.

# read the taproot key sig
if len(k) != 1:
raise PSBTError("Invalid taproot key signature key")
if self.taproot_key_sig is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

in which case this can happen?
maybe we can avoid error here if self.taproot_key_sig == v in this case anyways?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not exactly sure in what context this function is called, as iirc we neither use it directly in Krux nor indirectly. I simply replicated the verification from final scriptsig and final script witness, believing it is a sanity check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, I guess if we sign twice an input, it shold be with 2 differents nonces? (so we should end up with a different signature)

@odudex
Copy link
Collaborator Author

odudex commented Feb 7, 2025

A question also raised by @jdlcdl: With this PR, Embit now includes the same information in both the PSBT_IN_TAP_KEY_SIG and PSBT_IN_FINAL_SCRIPTWITNESS fields. Should we retain PSBT_IN_FINAL_SCRIPTWITNESS when the signature comes from the taproot internal key? Can a coordinator wallet rely solely on the signature in PSBT_IN_FINAL_SCRIPTWITNESS, making that field still necessary?

@pythcoiner
Copy link
Contributor

pythcoiner commented Feb 7, 2025

I dont think so, iiuc the input finalizer role must generate PSBT_IN_FINAL_SCRIPTWITNESS, and I expect the coordinator to take this role, not any signer.
But I think it's not harmfull to keep it and avoid a breaking change.

@pythcoiner
Copy link
Contributor

utACK ef61cca

@jdlcdl
Copy link

jdlcdl commented Feb 7, 2025

utACK ef61cca

ditto!

Krux: tested and working for pr498,
SeedSigner: doesnt break existing unit-tests or screen-shot-generator, otherwise untested

untested and haven't considered how this might affect other downstream projects.

@jdlcdl
Copy link

jdlcdl commented Feb 10, 2025

With due respect for downstream projects, and especially for past embit contributors, tagging:

Any feedback you may have for this PR, as well as for PR #67, would be greatly appreciated.

@odudex
Copy link
Collaborator Author

odudex commented Feb 21, 2025

We plan to release a new version of Krux soon, incorporating pull requests #67 and #68. @stepansnigirev kindly granted permission to merge these changes on Embit. However, since Stepan may have other priorities, I would like to gather feedback from the other Specter and SeedSigner developers to ensure these commits are safe and beneficial for everyone.

For now, I have pushed the relevant commits to the branch psbt_in_tap_key_sig_and_reproduc_psbt (EDIT)develop. This will allow you to test the changes easily without needing to add a remote. We will continue using this branch until we reach a consensus on PRs #67 and #68, at which point I will merge them into master.

I appreciate your testing and feedback.

@odudex
Copy link
Collaborator Author

odudex commented Mar 26, 2025

The commits from this pull request have now been merged into Embit's develop branch.

@3rdIteration
Copy link

I'll have a look at rolling a build of specter DIY with this and testing it out with Liana over the coming days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants