Add Legacy P2PKH Signing

[3rdIteration]

Description

Add support for Legacy P2PKH script type for both single and miltisig seeds.

This pull request is categorized as a:

• [X] New feature
• [ ] Bug fix
• [ ] Code refactor
• [ ] Documentation
• [ ] Other

Checklist

• [X] I’ve run pytest and made sure all unit tests pass before sumbitting the PR

If you modified or added functionality/workflow, did you add new unit tests?

• [ ] No, I’m a fool
• [x] Yes
• [ ] N/A

I have tested this PR on the following platforms/os:

• [ X] Raspberry Pi OS Manual Build
• [ ] SeedSigner OS on a Pi0/Pi0W board
• [ ] Other

Note: Keep your changes limited in scope; if you uncover other issues or improvements along the way, ideally submit those as a separate PR. The more complicated the PR the harder to review, test, and merge.

[jdlcdl]

Wow, seeing this a bit late Sunday evening. Will take another look in the morning.

[BamaHodl]

I have tested tx signing with both legacy p2pkh single-sig and legacy p2sh multi-sig. For both tests, I exported the relevant psbt from sparrow wallet, successfully signed with this SeedSigner code change, loaded the signed psbt back into sparrow wallet, and broadcast the valid transaction. I also tested Address Explorer and Address Verification for p2pkh script type successfully.

The only feature I found not working was exporting multi-sig xpub for the p2pkh (m/45') from SeedSigner into Sparrow wallet. Everything looks ok until Sparrow complains that p2sh was not sent as a possible tag (I ran into this with testing Electrum legacy multi-sig as well). It is fixed by adding the tag 400 (p2sh) for the hdkey to the outputs in src/seedsigner/models/encode_qr.py.
See https://github.com/BamaHodl/seedsigner/blob/LegacyElectrum/src/seedsigner/models/encode_qr.py for how I fixed it for my tests:

            elif origin.components[0].index == 44: # P2PKH
                ur_outputs.append(Output([SCRIPT_EXPRESSION_TAG_MAP[403]],self.ur_hdkey))
            elif origin.components[0].index == 45: # P2SH 
                ur_outputs.append(Output([SCRIPT_EXPRESSION_TAG_MAP[400]],self.ur_hdkey))

and later down also adding this when len(ur_outputs) is 0, i.e. derivation parsing didn't yield anything. This allows p2sh for electrum legacy seeds to work as well and covers all the bases with this addition:

            ur_outputs.append(Output([SCRIPT_EXPRESSION_TAG_MAP[400]],self.ur_hdkey))

I was able to find the tag map here for the descriptions of the tag numbers: https://github.com/selfcustody/urtypes/blob/main/src/urtypes/crypto/output.py

[jdlcdl]

Single-sig: I've tested this for single-sig legacy p2pkh w/o problems, worked great!

Multisig: I think labeling for legacy multisig should read "Legacy p2sh" instead of p2pkh? I had problems as well setting up the sparrow wallet, so more work here is needed (or more effort on my part. I didn't try BamaHodl's recommended solution above but I suspect it would have helped). I hope to discuss this before release 0.8.0 as another already-merged pr is related.

[3rdIteration]

Just added it in and can confirm that it behaves properly when exporting multisig xpubs to Sparrow. Thanks @BamaHodl and @jdlcdl

You are right that multisig is using P2SH, but as it stands, the script type label is set project-wide in the settings-definition.py, so perhaps just drop the explicit script type (So just call it "Legacy") It would be do-able to have it change the display name of the menu item for Legacy script type when exporting for Multisig, but that seems to be a messy way to do things...

[jdlcdl]

I'm late seeing this, but I have now tested both single-sig p2pkh and multisig p2sh exporting for both wallets, address explorer shows that addresses match, and can sign PSBTs.

As of 05f5f0e: this is working for me.

TO BE RESOLVED

• How to rename "Nested Segwit (Legacy)"? I think that "Nested Segwit" may be enough.
• How to rename "Legacy P2PKH"? I think that "Legacy" may be enough... else to have separate script type values for single-sig and multisig.
• And of course for others to take a look and share their thoughts.

[newtonick]

• How to rename "Nested Segwit (Legacy)"? I think that "Nested Segwit" may be enough.
• How to rename "Legacy P2PKH"? I think that "Legacy" may be enough.

"Nested Segwit" for P2SH-P2WSH (multisig) / P2SH-P2WPKH (single sig) and "Legacy" for P2SH (multisig) / P2PKH (single sig) would be my vote. This is what Sparrow Wallet uses.

[3rdIteration]

Made the tweak. It's certainly the cleanest solution.

[jdlcdl]

For my part, as of 4f6456b

ACK tested

p.s. pre-ack on the next pr/commit that alters "LEGACY_P2PKH" to "LEGACY" constant system-wide.

[jdlcdl]

I wonder if the above was an error taking both versions of a previous conflict. Now this pr is in conflict with 'dev' leaving a very similar looking situation in ._get_policy()

I suspect you'll want the "dev" version which is indented.

Note: 2 lines above the conflict is the elif "p2sh" in script_type which needs to become elif "p2sh" == script_type, which fixes single-sig nested-segwit psbt parsing, so if this is resolved at the same time, it won't create another conflict with Nick's pr572.

[3rdIteration]

I'll have a chance to have a proper look at it later on today. I'll also go through and add some relevant tests into the test_decodepsbtqr, as this is basically where the testing for PSBT parsing like this is happening.

[newtonick]

@BamaHodl and @jdlcdl I saw the comments above after I resolved the merge conflict. I probably should have waited before resolving the merge conflict. I'll review in more detail. Let me know if you feel differently about how I resolve the conflict.

[newtonick]

tACK