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

Minor fixes for compatibility #11

Closed
wants to merge 2 commits into from

Conversation

brandonblack
Copy link

No description provided.

Signed-off-by: Brandon Black <brandonblack@bitgo.com>
0 and 1 are not valid private keys and some libraries will rightly reject them

Signed-off-by: Brandon Black <brandonblack@bitgo.com>
@brandonblack brandonblack changed the title make Network.bech32 optional for altcoin compatibilty Minor fixes for compatibility Sep 3, 2022
@landabaso
Copy link

landabaso commented Sep 28, 2022

You might want to take a look at the discussions below about removing cases with tweak = 0 in the tests in this PR:

@junderw
Copy link
Member

junderw commented Sep 28, 2022

  1. I don't understand the bech32 change.
  2. 1 is a valid private key. 0 is not. However, privateAdd args are (privateKey, scalar)... and scalar is [0, curve.n) so the only difference between private keys and scalars is the inclusion of 0.

This test was put there to test that privateAdd actually uses scalars for the second position, and it's doing its job.

@brandonblack
Copy link
Author

Thanks @junderw and @landabaso!

I don't understand the bech32 change.

Many forkcoins don't have a bech32 prefix. So, when using bitcoinjs libraries to support forkcoins, the network parameter will often not have a meaningful value for the bech32 member. I think we have this change made to our fork of bitcoinjs-lib as well, but haven't upstreamed it yet.

@paulmillr
Copy link

@junderw

the only difference between private keys and scalars is the inclusion of 0

Private key is scalar. What's the difference? Why would you allow 0 in privateKeyAdd, if it enables non-contributing behavior?

@paulmillr
Copy link

for example, it was said curve25519 keys should not be validated, however, "zero" keys enabled some bad behavior: https://research.kudelskisecurity.com/2017/04/25/should-ecdh-keys-be-validated/

@junderw
Copy link
Member

junderw commented Sep 28, 2022

Private key is scalar. What's the difference? Why would you allow 0 in privateKeyAdd, if it enables non-contributing behavior?

In the bitcoin secp256k1 library, the secp256k1_ec_privkey_tweak_add method internally uses secp256k1_scalar to represent both seckey and tweak32... but only seckey is eventually checked against 0, both on input and right before output (it is modified in-place)

Whereas tweak32 is only compared as being less than the order of the curve N.

If we define TypeA as being a type that represents in bytes secp256k1_scalar that is validated to be in the range [1, N), and TypeB as being a type that represents in bytes secp256k1_scalar that is [0, N)

The call signature for this function is privateAdd(TypeA, TypeB): TypeA.

The goal of the tiny-secp256k1 interface is to be in line with the secp256k1 library, but with a smaller interface than node-secp256k1, and the behavior of these functions was designed to match up with libsecp256k1.

https://github.com/bitcoin-core/secp256k1/blob/6a873cc4a97cccf234b019c064386c82f2df2111/src/tests.c#L5623-L5625

@paulmillr
Copy link

If the goal is to match btc libsecp api, that makes sense.

@junderw
Copy link
Member

junderw commented Sep 29, 2022

Since the bech32 change is for non-Bitcoin assets, I will say no.

And the privateAdd part has been explained so I will close this.

Thanks for the PR.

@junderw junderw closed this Sep 29, 2022
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 this pull request may close these issues.

None yet

5 participants