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

retry on failure secp256k1 #1409

Closed
dvc94ch opened this issue Sep 27, 2022 · 10 comments · Fixed by #1774
Closed

retry on failure secp256k1 #1409

dvc94ch opened this issue Sep 27, 2022 · 10 comments · Fixed by #1774

Comments

@dvc94ch
Copy link

dvc94ch commented Sep 27, 2022

is BIP32 modified for the secp256k1 curve too? what should a correct implementation do in case of failure on secp256k1?

@prusnak
Copy link
Member

prusnak commented Sep 27, 2022

Quoting from BIP32:

In case parse256(IL) ≥ n or ki = 0, the resulting key is invalid, and one should proceed with the next value for i. (Note: this has probability lower than 1 in 2^127.)

@prusnak prusnak closed this as not planned Won't fix, can't repro, duplicate, stale Sep 27, 2022
@dvc94ch
Copy link
Author

dvc94ch commented Sep 27, 2022

two different error handling strategies seem a bit inconvenient. is there a technical reason why SLIP-0010 differs in strategy? Is probability of an error higher than secp256k1?

@prusnak
Copy link
Member

prusnak commented Sep 27, 2022

Is probability of an error higher than secp256k1?

Exactly.

Also I find the BIP32 handling of the issue very unlucky, because almost no-one implements the check and the result can be catastrophic.

@steed717
Copy link

steed717 commented Jan 31, 2024

Quoting from BIP32:

In case parse256(IL) ≥ n or ki = 0, the resulting key is invalid, and one should proceed with the next value for i. (Note: this has probability lower than 1 in 2^127.)

So, if using BIP32 and strictly implementing the error handling above, BIP32 may skip some index (let say index N) in the key tree, which SLIP 10 will not. Does this break their compatibility since the BIP32 tree will differ from the SLIP10 key tree? What will happen, if the the key under the N subtree was used in BIP32 implementation?

@prusnak
Copy link
Member

prusnak commented Jan 31, 2024

Does this break their compatibility since the BIP32 tree will differ from the SLIP10 key tree?

BIP32 is defined for secp256k1 curve

SLIP10 is defined for curves that are not secp256k1

There never was a compatibility

@steed717
Copy link

steed717 commented Jan 31, 2024

Does this break their compatibility since the BIP32 tree will differ from the SLIP10 key tree?

BIP32 is defined for secp256k1 curve

SLIP10 is defined for curves that are not secp256k1

There never was a compatibility
Sorry, confused you, when I am talking the skip index N, I mean when SLIP10 uses secpk1, the key tree will differ. What will happen, if the the SLIP10 k1's key under the N subtree was used in BIP32 implementation? is it safe and will not break and fine to use the BIP32 implementation?

@prusnak
Copy link
Member

prusnak commented Jan 31, 2024

You should not use SLIP10 with secp256k1

@steed717
Copy link

steed717 commented Jan 31, 2024

You should not use SLIP10 with secp256k1
This isn't very clear. See the statement from SLIP10

The supported curves are
    Curve = "Bitcoin seed" for the secp256k1 curve (this is compatible with BIP-0032).
    Curve = "Nist256p1 seed" for the NIST P-256 curve.
    Curve = "ed25519 seed" for the ed25519 curve.

Do you mean the secp256k1 extended keys generated from SLIP10 should not be used in the BIP32 application?

Any suggestion with this question?

@andrewkozlik
Copy link
Contributor

andrewkozlik commented Feb 15, 2024

You should not use SLIP10 with secp256k1

I have to say that I agree with @steed717. SLIP-10 is not entirely clear about this. Although several statements in the spec indicate that it is intended for curve types different from secp256k1, e.g. the abstract, there are at least two things in the spec that contradict this:

  1. The already mentioned quote:

    The supported curves are

    • Curve = "Bitcoin seed" for the secp256k1 curve (this is compatible with BIP-0032).

    which is clearly untrue.

  2. SLIP-10 includes test vectors for secp256k1, which seems strange if it's not intended to be used with secp256k1.

Finally, our own trezor-crypto actually uses SLIP-10 for secp256k1 key derivation. Of course, this is identical to BIP-32 with near certainty (probability greater than 1−2−127 per derivation operation), but this only adds to the contradiction. @matejcik

I do prefer SLIP-10 over BIP-32, because it simplifies error handling. When applied to secp256k1, SLIP-10 generalizes BIP-32, because it fills in the possible holes left by BIP-32 in the derivation tree. So I think the choice made in trezor-crypto is not entirely bad. The only problem is that there is a chance (with infinitesimal probability) that when a seed is ported from Trezor to another wallet, which uses pure BIP-32, that some addresses will not be discovered.

@andrewkozlik andrewkozlik reopened this Feb 15, 2024
@andrewkozlik
Copy link
Contributor

For the record, we had a meeting on 27th February 2024 with @prusnak and @slush0 where we agreed that SLIP-10 should be updated to say that it also applies to the Bitcoin curve. I will update the spec accordingly.

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

Successfully merging a pull request may close this issue.

4 participants