bip32: add some of the new bip32 checks to bip32_key_unserialize#522
bip32: add some of the new bip32 checks to bip32_key_unserialize#522jgriffiths merged 1 commit intomasterfrom
Conversation
That´s interesting. I understand the design choice. Do you have any concrete examples of implementations where fingerprint/index fields are known to be incorrect? |
There was some discussion in this repo long ago about tightening these checks/inferring meaning from the depth/fingerprint etc; there's probably some background there. We've found that various tools/wallets over the years can fail to populate some fields correctly, see e.g. Blockstream/gdk@cb324bb. Also some fields may be skipped by choice, for example in wally you can skip the fingerprint calculation for speed when deriving (the fingerprint is meant to be an optimization for locating potential matches in some use cases, and was not originally imbued with any meaning such as zero meaning a master key). In general unless the data included would cause invalid derivations/addresses to be produced we should attempt to be liberal in what we accept and conservative in what we output. So checking invalid pubkeys (where we haven't derived it from a private key) makes sense to check, for example. |
| # and index are inconsistent, as some implementations do | ||
| # not set them correctly (but nonetheless work for their | ||
| # intended purpose(s)). | ||
| # zero depth with non-zero parent fingerprint (xprv) | ||
| #'xprv9s2SPatNQ9Vc6GTbVMFPFo7jsaZySyzk7L8n2uqKXJen3KUmvQNT' | ||
| #'uLh3fhZMBoG3G4ZW1N2kZuHEPY53qmbZzCHshoQnNf4GvELZfqTUrcv', | ||
| # zero depth with non-zero parent fingerprint (xpub) | ||
| #'xpub661no6RGEX3uJkY4bNnPcw4URcQTrSibUZ4NqJEw5eBkv7ovTwgi' | ||
| #'T91XX27VbEXGENhYRCf7hyEbWrR3FewATdCEebj6znwMfQkhRYHRLpJ', | ||
| # zero depth with non-zero index (xprv) | ||
| #'xprv9s21ZrQH4r4TsiLvyLXqM9P7k1K3EYhA1kkD6xuquB5i39AU8KF4' | ||
| #'2acDyL3qsDbU9NmZn6MsGSUYZEsuoePmjzsB3eFKSUEh3Gu1N3cqVUN', | ||
| # zero depth with non-zero index (xpub) | ||
| #'xpub661MyMwAuDcm6CRQ5N4qiHKrJ39Xe1R1NyfouMKTTWcguwVcfrZJ' | ||
| #'aNvhpebzGerh7gucBvzEQWRugZDuDXjNDRmXzSZe4c7mnTK97pTvGS8', | ||
| # unknown extended key version (1) |
There was a problem hiding this comment.
Possible nit: From looking into src/test/* I see that one can skip tests. Would it maybe be a bit clearer if these test vectors were explicitly skipped instead of commented out? That is, have a custom test for checking the fingerprint and skip it.
There was a problem hiding this comment.
ATM we only use skipTest for 2 reasons: whether elements is enabled or to skip exhaustive/expensive tests. I'd like to keep usage down to just those two reasons. I think if we revisit and want to enable these tests at some point in the future there won't be any difficulty finding them.
Interesting, so extended keys are malleable. Not saying in what context could this be a problem, just did not think about them in this way. |
in toto yes, via the key metadata (which I distinguish from the 'core' data required to derive). Many (most?) users of extended keys prior to PSBT don't need the fingerprints at all for example, so you could put anything in there most of the time. Thats billions of different representations of the same key. Descriptor expressions are also malleable in many ways by (poor) design. |
Based on the patch from @kuliq23 in #521
Note as per the test comments, we don't fail keys for zero fingerprints or inconsistent indices.