Output Descriptors #1363

prusnak · 2020-11-21T11:29:16Z

In order to fully support Output Descriptors we need to change things on couple of places:

common
- add GetPublicKey.ignore_xpub_magic and PublicKey.root_fingerprint fields
python
- add trezorctl btc get-descriptor command
- add tests for trezorctl btc get-descriptor command`
- update get-descriptor test to include correct root_fingerprint
legacy
- implement ignore_xpub_magic
- implement root_fingerprint
- show XPUB instead of Pubkey in GetPublicKey screen
- add ignore_xpub_magic tests
- add root_fingerprint tests
core
- implement ignore_xpub_magic
- implement root_fingerprint
- show XPUB instead of Pubkey in GetPublicKey screen
- add ignore_xpub_magic tests
- add root_fingerprint tests

matejcik · 2020-11-30T10:56:48Z

what is the reason for ignore_xpub_magic? wouldn't it be better for the client to strip/recalculate that?

prusnak · 2020-11-30T12:11:23Z

what is the reason for ignore_xpub_magic? wouldn't it be better for the client to strip/recalculate that?

We need to show the same xpub on the display, without that flag the trezor shows ypub... or zpub... and the descriptor shows xpub...

matejcik · 2020-11-30T12:35:20Z

We need to show the same xpub on the display

I'm wondering whether there should be a GetDescriptor call then. Comparing display XPUB and host-side full descriptor is weird.

OTOH I can see uses for the xpub display other than the descriptor, so 🤷‍♀️

matejcik · 2020-11-30T12:59:15Z

python/src/trezorlib/btc.py

+    )
+
+    fingerprint = pub.root_fingerprint if pub.root_fingerprint is not None else 0
+    external = "[{:08x}{}]{}/0/*".format(fingerprint, path[1:], pub.xpub)


please use f-strings

Addressed in 15c3753

matejcik · 2020-11-30T12:59:24Z

python/src/trezorlib/btc.py

+    external = "[{:08x}{}]{}/0/*".format(fingerprint, path[1:], pub.xpub)
+    internal = "[{:08x}{}]{}/1/*".format(fingerprint, path[1:], pub.xpub)
+
+    return (fmt.format(external), fmt.format(internal))


please don't use outer parens

Addressed in 15c3753

matejcik · 2020-11-30T13:50:40Z

python/src/trezorlib/btc.py

        )
    )


+def get_descriptor(


this function should move to trezorlib.cli.btc as it's too high-level for this file

it should also probably accept address_n, and the get-descriptor command should also accept -n/--address, plus possibly -a/--account as a convenience shortcut

I prefer this won't accept address_n - I don't see a good reason why it should.

Will move this to trezorlib.cli.btc

If I moved this to trezorlib.cli.btc, where should I move the test from 9af06e7?

I really want to have this feature properly covered by tests.

well the reason to accept address_n is to allow use by coins other than Bitcoin, but i don't care that much so eh

re tests, we can keep them where they are, importing from trezorlib.cli namespace is not discouraged or anything

i'm thinking that the bulk of the functionality should be tested in python/tests presumably by mocking out the btc.get_public_node call?
(i don't think it is super important though)

Function moved to cli.btc in 15c3753

Tests moved in ec4d3c3

matejcik · 2020-11-30T13:51:40Z

python/src/trezorlib/btc.py

+    else:
+        raise exceptions.TrezorException("Unsupported account type")
+
+    # TODO: correctly lookup SLIP44 for coin provided


In context of accepting address_n, it's not important that we don't have the full SLIP44 lookup here. (fwiw we don't bundle the data for this anymore)

Let's drop the SLIP44 lookup completely and accept only Bitcoin/Testnet in the func

matejcik · 2020-11-30T13:53:22Z

left some comments on python part

LGTM on core side

did not review legacy, i might look at it later if nobody else does

matejcik · 2020-12-01T11:00:54Z

reviewed legacy as well, looks good

prusnak · 2020-12-01T11:28:26Z

I updated the tests to test the cli module in ec4d3c3, but now they fail with:

E           TypeError: 'TrezorClientDebugLink' object is not iterable

Can you please advise?

prusnak · 2020-12-02T09:44:52Z

I updated the tests to test the cli module in ec4d3c3, but now they fail with:

Fixed in 1bad419

prusnak · 2021-01-05T14:51:21Z

This is now ready for review. One things I am unsure about:

There are two ways how to obtain the root fingerprint:

hack the derive function to return not only a derived node, but also a root fingerprint
derive a dummy node (e.g. m/0') and return a parent fingerprint stored in that node

I used method 1 in Legacy and method 2 in Core. My reasoning is that using method 2 in Core would introduce lots of code/typing changes, but I do agree that method 2 is quite a hack. We might consider using method 2 for Legacy as well.

Let me know what you think.

andrewkozlik · 2021-01-05T15:40:38Z

There are two ways how to obtain the root fingerprint:

hack the derive function to return not only a derived node, but also a root fingerprint

derive a dummy node (e.g. m/0') and return a parent fingerprint stored in that node

I used method 1 in Legacy and method 2 in Core. My reasoning is that using method 2 in Core would introduce lots of code/typing changes, but I do agree that method 2 is quite a hack. We might consider using method 2 for Legacy as well.

Let me know what you think.

Method 2 is cleaner, but I don't have a problem with either of the two approaches. It's no big deal if this is implemented slightly differently in core and legacy, so I'd say it's OK as is.

matejcik · 2021-01-08T10:02:57Z

There are two ways how to obtain the root fingerprint:

hack the derive function to return not only a derived node, but also a root fingerprint

derive a dummy node (e.g. m/0') and return a parent fingerprint stored in that node

I used method 1 in Legacy and method 2 in Core. My reasoning is that using method 2 in Core would introduce lots of code/typing changes, but I do agree that method 2 is quite a hack. We might consider using method 2 for Legacy as well.

I'm a little confused here, ISTM both core and legacy are using the same method?
Personally I'd think the cleanest solution would be a root_fingerprint() function in crypto, but that seems needless, the employed solution looks good enough

prusnak · 2021-01-08T10:08:13Z

I'm a little confused here, ISTM both core and legacy are using the same method?

I reworked it to use the same method and force-pushed.

prusnak · 2021-01-08T10:14:15Z

Can you please @andrewkozlik have one last look at reworked legacy change (that uses the same method as core, i.e. deriving m/0'): c76145a

andrewkozlik

utACK for the root_fingerprint derivation.
However, note that the UI test are failing.

prusnak · 2021-01-08T11:40:55Z

Hm, about the tests: for some strange reason I'm getting different hashes each time I run pytest test_descriptors.py --ui=record. Can someone else have a look at it? Maybe @tsusanka has an idea?

andrewkozlik · 2021-01-08T11:46:31Z

Hm, about the tests: for some strange reason I'm getting different hashes each time I run pytest test_descriptors.py --ui=record. Can someone else have a look at it? Maybe @tsusanka has an idea?

I suppose it could be the pulsating swipe arrow, but we have the same thing in test_msg_backup_device.py, so the UI tests should be accustomed to it.

tsusanka · 2021-01-08T11:54:33Z

I suppose it could be the pulsating swipe arrow, but we have the same thing in test_msg_backup_device.py, so the UI tests should be accustomed to it.

Yes, the pulsing should disabled as the tests are run against emulator with disabled animations. I'll double check though.

prusnak · 2021-01-08T11:57:11Z

I confirm I have reproducible hashes using TREZOR_DISABLE_ANIMATION=1 when running the tests.

Let's wait for the CI.

tsusanka · 2021-01-08T11:58:58Z

Yes and it is in the Makefile so we should be all good.

prusnak requested review from matejcik and tsusanka as code owners November 21, 2020 11:29

prusnak marked this pull request as draft November 21, 2020 11:29

prusnak mentioned this pull request Nov 21, 2020

trezorctl trezorctl btc get-descriptor command #1362

Closed

prusnak added this to the 2021-01 milestone Nov 21, 2020

prusnak force-pushed the descriptors branch 2 times, most recently from bf0ed99 to 9af06e7 Compare November 21, 2020 21:55

matejcik reviewed Nov 30, 2020

View reviewed changes

tsusanka removed their request for review December 1, 2020 07:58

tsusanka modified the milestones: 2021-01, 2021-02 Dec 29, 2020

prusnak force-pushed the descriptors branch 3 times, most recently from 92f4dd1 to f5b8161 Compare January 5, 2021 14:46

prusnak marked this pull request as ready for review January 5, 2021 14:47

prusnak requested a review from matejcik January 5, 2021 14:55

prusnak added 5 commits January 7, 2021 18:59

feat(proto): add descriptors related fields to GetPublicKey/PublicKey

f5f3356

feat(python): add btc get-descriptor command

76074ad

fix(legacy): show xpub instead of pubkey in GetPublicKey dialog

d3e4b53

feat(legacy): implement GetPublicKey.ignore_xpub_magic behaviour

e33455d

fix(core): show xpub instead of pubkey in GetPublicKey dialog

bf556f3

prusnak added 4 commits January 7, 2021 18:59

feat(core): implement GetPublicKey.ignore_xpub_magic behaviour

aacd6ce

feat(tests): test GetPublicKey.ignore_xpub_magic behaviour

d7e0014

legacy: return root_fingerprint in PublicKey

c76145a

core: return root_fingerprint in PublicKey

8e36b23

prusnak force-pushed the descriptors branch from f5b8161 to 5a32ce3 Compare January 7, 2021 18:16

matejcik approved these changes Jan 8, 2021

View reviewed changes

prusnak force-pushed the descriptors branch from 5a32ce3 to 20bab30 Compare January 8, 2021 10:08

andrewkozlik approved these changes Jan 8, 2021

View reviewed changes

feat(tests): add device tests for btc.get_descriptor

bc5f96f

prusnak force-pushed the descriptors branch from 20bab30 to bc5f96f Compare January 8, 2021 11:56

prusnak merged commit 7b9f3c0 into master Jan 8, 2021

prusnak deleted the descriptors branch January 8, 2021 13:17

mmilata mentioned this pull request Feb 20, 2024

Send descriptor in GetPublicKey response for Taproot accounts #3539

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Output Descriptors #1363

Output Descriptors #1363

prusnak commented Nov 21, 2020 •

edited

matejcik commented Nov 30, 2020

prusnak commented Nov 30, 2020 •

edited

matejcik commented Nov 30, 2020

matejcik Nov 30, 2020

prusnak Dec 1, 2020

matejcik Nov 30, 2020

prusnak Dec 1, 2020

matejcik Nov 30, 2020

prusnak Nov 30, 2020 •

edited

prusnak Nov 30, 2020 •

edited

matejcik Dec 1, 2020

prusnak Dec 1, 2020

matejcik Nov 30, 2020

prusnak Dec 1, 2020

matejcik commented Nov 30, 2020

matejcik commented Dec 1, 2020

prusnak commented Dec 1, 2020

prusnak commented Dec 2, 2020

prusnak commented Jan 5, 2021

andrewkozlik commented Jan 5, 2021

matejcik commented Jan 8, 2021

prusnak commented Jan 8, 2021

prusnak commented Jan 8, 2021

andrewkozlik left a comment

prusnak commented Jan 8, 2021

andrewkozlik commented Jan 8, 2021

tsusanka commented Jan 8, 2021 •

edited

prusnak commented Jan 8, 2021

tsusanka commented Jan 8, 2021

Output Descriptors #1363

Output Descriptors #1363

Conversation

prusnak commented Nov 21, 2020 • edited

matejcik commented Nov 30, 2020

prusnak commented Nov 30, 2020 • edited

matejcik commented Nov 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prusnak Nov 30, 2020 • edited

Choose a reason for hiding this comment

prusnak Nov 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejcik commented Nov 30, 2020

matejcik commented Dec 1, 2020

prusnak commented Dec 1, 2020

prusnak commented Dec 2, 2020

prusnak commented Jan 5, 2021

andrewkozlik commented Jan 5, 2021

matejcik commented Jan 8, 2021

prusnak commented Jan 8, 2021

prusnak commented Jan 8, 2021

andrewkozlik left a comment

Choose a reason for hiding this comment

prusnak commented Jan 8, 2021

andrewkozlik commented Jan 8, 2021

tsusanka commented Jan 8, 2021 • edited

prusnak commented Jan 8, 2021

tsusanka commented Jan 8, 2021

prusnak commented Nov 21, 2020 •

edited

prusnak commented Nov 30, 2020 •

edited

prusnak Nov 30, 2020 •

edited

prusnak Nov 30, 2020 •

edited

tsusanka commented Jan 8, 2021 •

edited