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

Better way to handle multisig change validation? #42

Open
mflaxman opened this issue Apr 17, 2021 · 0 comments
Open

Better way to handle multisig change validation? #42

mflaxman opened this issue Apr 17, 2021 · 0 comments

Comments

@mflaxman
Copy link
Collaborator

mflaxman commented Apr 17, 2021

In #41, I added a method called describe_basic_multisig_tx() that abstracts away a lot of what's happening under the hood for wallet developers.

One remaining issue is that Signers need a copy of all the xpubs, which requires state to be stored somewhere. It seems like describe_basic_multisig_tx() is duplicating a lot of code from the various parse() and validate methods in psbt.py. Another complicating factor is that my current implementation requires a dict of the form {xfp_hex: HDPublicKey}, but it would be possible to drop the xfp_hex requirement (and just iterate through a list of HDPublicKeys here. Performance would be worse, but developer UX would be better (no need to know root_fingerprint_hex for each HDPublicKey).

Some ideas:

  1. Would it be a good idea to move some of the logic from from describe_basic_multisig_tx() to the PSBT class (either parse() and/or validate() methods? The simplest answer may be to add the xpubs as NamedHDPublicKey to the PSBT, and do that in a way so that validating business logic (validate each input matches those xpubs, and that change has the same xpubs m-of-n, etc) can easily be done in describe_basic_multisig_tx().
  2. Is there a way to pass state in via the PSBT? This may not be possible as we want to be compatible with what Specter-Desktop is using, so that's problematic.

There's also a potential security vulnerability pointed out by @jimmysong. I don't see the vulnerability, so would like to understand what I'm missing so I can fix it!

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

No branches or pull requests

1 participant