Add Legacy P2PKH Signing #567

3rdIteration · 2024-07-08T00:06:50Z

Description

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

This pull request is categorized as a:

Checklist

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
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.

Initial copy from Seedsigner+Satochip

Initial copy from Seedsigner+Satochip fork

…eedsigner into add-p2pkh-signing

jdlcdl · 2024-07-08T01:21:48Z

src/seedsigner/helpers/embit_utils.py

        elif script_type == SettingsConstants.TAPROOT:
            return f"m/86'/{network_path}/0'"
        else:
            raise Exception("Unexpected script type")

    elif wallet_type == SettingsConstants.MULTISIG:
-        if script_type == SettingsConstants.NATIVE_SEGWIT:
-            return f"m/48'/{network_path}/0'/2'"
+        if script_type == SettingsConstants.LEGACY_P2PKH:


??? Does this belong?

Wasn't sure, but now I see that "legacy" is not just meant as p2pkh.

The native segwit block got moved down, basically just trying to be mostly consistent in terms of the code logic reflecting the BIP numbers (So older types with smaller BIP numbers start fist and then iterate through the BIPs in order of their release, so legacy first, Taproot last)

jdlcdl · 2024-07-08T01:41:03Z

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

BamaHodl · 2024-07-08T23:52:44Z

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 · 2024-07-09T22:09:57Z

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 · 2024-07-10T01:36:48Z

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 · 2024-07-10T19:05:33Z

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 · 2024-07-11T00:57:03Z

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 · 2024-07-12T01:15:22Z

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

jdlcdl · 2024-07-12T09:07:18Z

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.

src/seedsigner/models/psbt_parser.py

Remove duplicate UTXO addition

jdlcdl · 2024-07-19T16:20:26Z

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 · 2024-07-19T17:48:46Z

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 · 2024-07-19T18:36:56Z

@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 · 2024-07-19T20:18:33Z

tACK

jdlcdl · 2024-07-19T20:29:38Z

src/seedsigner/models/psbt_parser.py

@@ -277,16 +277,15 @@ def _get_policy(scope, scriptpubkey, xpubs):
            elif "p2sh" in script_type and scope.redeem_script is not None:


~~Does this need to be a "p2sh" == script_type for native segwit single-sig? I think this was corrected in 572, making sure nothing snuck back in.~~
I confirm that this conflict was properly resolved.

3rdIteration added 5 commits July 7, 2024 19:13

Add Legacy P2PKH Signing

4cd5522

Initial copy from Seedsigner+Satochip

Remove not-implemented error

300a44f

Add Legacy P2PKH Signing

451639f

Initial copy from Seedsigner+Satochip fork

Add Legacy tests to embit_utils CI test

b88d0cd

Merge branch 'add-p2pkh-signing' of https://github.com/3rdIteration/s…

284e894

…eedsigner into add-p2pkh-signing

jdlcdl reviewed Jul 8, 2024

View reviewed changes

Fix Multisix Xpub Export not working correctly for P2PKH Legacy Wallets

05f5f0e

Simplify Script Type Descriptions

4f6456b

newtonick reviewed Jul 19, 2024

View reviewed changes

src/seedsigner/models/psbt_parser.py Outdated Show resolved Hide resolved

Remove duplicate UTXO addition

909d7f5

Remove duplicate UTXO addition

Merge branch 'dev' into add-p2pkh-signing

4456ce4

newtonick merged commit 0377024 into SeedSigner:dev Jul 19, 2024
2 checks passed

jdlcdl reviewed Jul 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Legacy P2PKH Signing #567

Add Legacy P2PKH Signing #567

3rdIteration commented Jul 8, 2024 •

edited

Loading

jdlcdl Jul 8, 2024 •

edited

Loading

3rdIteration Jul 8, 2024

jdlcdl commented Jul 8, 2024

BamaHodl commented Jul 8, 2024 •

edited

Loading

jdlcdl commented Jul 9, 2024

3rdIteration commented Jul 10, 2024 •

edited

Loading

jdlcdl commented Jul 10, 2024 •

edited

Loading

newtonick commented Jul 11, 2024

3rdIteration commented Jul 12, 2024

jdlcdl commented Jul 12, 2024 •

edited

Loading

jdlcdl commented Jul 19, 2024 •

edited

Loading

3rdIteration commented Jul 19, 2024

newtonick commented Jul 19, 2024

newtonick commented Jul 19, 2024

jdlcdl Jul 19, 2024 •

edited

Loading

		@@ -277,16 +277,15 @@ def _get_policy(scope, scriptpubkey, xpubs):
		elif "p2sh" in script_type and scope.redeem_script is not None:

Add Legacy P2PKH Signing #567

Add Legacy P2PKH Signing #567

Conversation

3rdIteration commented Jul 8, 2024 • edited Loading

Description

Checklist

jdlcdl Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

3rdIteration Jul 8, 2024

Choose a reason for hiding this comment

jdlcdl commented Jul 8, 2024

BamaHodl commented Jul 8, 2024 • edited Loading

jdlcdl commented Jul 9, 2024

3rdIteration commented Jul 10, 2024 • edited Loading

jdlcdl commented Jul 10, 2024 • edited Loading

newtonick commented Jul 11, 2024

3rdIteration commented Jul 12, 2024

jdlcdl commented Jul 12, 2024 • edited Loading

jdlcdl commented Jul 19, 2024 • edited Loading

3rdIteration commented Jul 19, 2024

newtonick commented Jul 19, 2024

newtonick commented Jul 19, 2024

jdlcdl Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

3rdIteration commented Jul 8, 2024 •

edited

Loading

jdlcdl Jul 8, 2024 •

edited

Loading

BamaHodl commented Jul 8, 2024 •

edited

Loading

3rdIteration commented Jul 10, 2024 •

edited

Loading

jdlcdl commented Jul 10, 2024 •

edited

Loading

jdlcdl commented Jul 12, 2024 •

edited

Loading

jdlcdl commented Jul 19, 2024 •

edited

Loading

jdlcdl Jul 19, 2024 •

edited

Loading