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

Implement multisig address explorer #25

Open
wants to merge 2 commits into
base: master
from

Conversation

@hodlwave
Copy link
Contributor

hodlwave commented Oct 24, 2019

This change required several steps:

  • Add methods to create a redeem script and multisig address
  • Refactor Address Explorer to use injected get_address_at_idx method
  • Implement multisig address explorer menu
  • Test for multisig address explorer

The test also required

  • adding a fixture in conftest.py to select the blockchain on the device
  • refactoring the make_multisig fixture to use multiple chains / addr_fmts

N.B. In order to generalize the make_multisig fixture, I set the last signer to be the simulator's actual extended private key as opposed to the one in the constants file. I don't believe this should have any backwards compatibility issues, but I wanted to make a note of it in case tests start breaking.

@hodlwave

This comment has been minimized.

Copy link
Contributor Author

hodlwave commented Oct 24, 2019

@peter-conalgo @nvk

Also I held off on adding the multisig wallets to the regular 'Address Explorer' menu for now because I thought the PR was pretty large as is.

shared/multisig.py Outdated Show resolved Hide resolved
shared/multisig.py Outdated Show resolved Hide resolved
shared/multisig.py Outdated Show resolved Hide resolved
@peter-conalgo

This comment has been minimized.

Copy link
Contributor

peter-conalgo commented Oct 28, 2019

Sorry I pressed "start review" by mistake, and no clear way to un-do it... meant to just comment on some line numbers.

hodlwave added 2 commits Oct 20, 2019
This change required several steps:
- Add methods to create a redeem script and multisig address
- Refactor Address Explorer to use injected get_address_at_idx method
- Implement multisig address explorer menu
- Test for multisig address explorer

The test also required
- adding a fixture in conftest.py to select the blockchain on the device
- refactoring the make_multisig fixture to use multiple chains / addr_fmts
Implemented fixes based on @peter-conalgo PR review. These include:
- Refactor get_address_at_idx to get_addresses for performance
- Avoid passing coin as argument to methods
- Avoid calculating extra data that is never used
@hodlwave hodlwave force-pushed the hodlwave:multisig-address-explorer branch from c3dfa72 to 7d19381 Oct 31, 2019
@hodlwave

This comment has been minimized.

Copy link
Contributor Author

hodlwave commented Oct 31, 2019

New updates

Implemented fixes based on @peter-conalgo PR review. These include:

  • Refactor get_address_at_idx to get_addresses for performance
  • Avoid passing coin as argument to methods
  • Avoid calculating extra data that is never used
@peter-conalgo

This comment has been minimized.

Copy link
Contributor

peter-conalgo commented Oct 31, 2019

Great changes, thanks.

I've looked more closely now, and here are my concerns:

  1. I don't like ms_path_mapper() because it presumes "purpose 48" being used, and we don't assume that elsewhere. We don't need to reconstruct paths because we know the shared common prefix path based on the wallet (common_prefix). .. we can just take that and add {change}/{idx}. I suggest deleting ms_path_mapper() .. sorry.
  2. make_ms_address() should be a member function on MultisigWallet, and should probably take a "suffix path" to be appended onto the existing common prefix path (self.common_prefix). For this explorer application, we'd provide string: '0/0' thru '0/10'... The keys argument should stay as an optimization.
  3. IIRC some multisig schemes use: ...common prefix.../{index of co-signer}/{change}/{idx}, so that there is no danger of collision between the co-signers when they construct addresses. We should support that. I don't see how we can though.

I am very scared of providing an address the UTXO-watching wallet isn't expecting. With multisig, we might even make an address that we cannot recover funds from, because we need additional signatures from another system that is not as flexible as we are with derivation paths.

@hodlwave

This comment has been minimized.

Copy link
Contributor Author

hodlwave commented Oct 31, 2019

Good points. I think they actually speak to a bigger issue I hadn't considered. Since the MultisigWallet only stores up to the common_prefix, it doesn't know anything about the full derivation used until instructed by the PSBT during transaction signing.

So the open question is what derivations do you want to choose to show in an Address Explorer if any? Allowing the user to explore more derivation paths increases complexity. But reducing this ability or eliminating it entirely prevents users from verifying receive addresses on device.

What do you think?

@destrys

This comment has been minimized.

Copy link

destrys commented Feb 1, 2020

I don't believe Electrum allows you to specify custom suffixes, it's just /0/* and /1/*. That could be a good place to start, maybe with an explicit warning that the address explorer is only displaying electrum-compatible addresses?

We're excited about this feature...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.