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

Create slip-0077.md #662

Merged
merged 2 commits into from Jul 3, 2019

Conversation

Projects
None yet
5 participants
@romanz
Copy link
Contributor

commented Jun 15, 2019

@romanz romanz force-pushed the romanz:master branch from 19546c2 to bd336bb Jun 15, 2019

@romanz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

Updated the SLIP following trezor/trezor-firmware#244 (comment) according to Proposal 3.

Show resolved Hide resolved slip-0077.md Outdated
Show resolved Hide resolved slip-0077.md Outdated
Show resolved Hide resolved slip-0077.md Outdated
@prusnak

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

@romanz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

Many thanks, will update the PR.

@romanz romanz force-pushed the romanz:master branch from 12d7940 to 2992f3f Jun 27, 2019

@romanz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

Fixed the comments, squashed and force-pushed 2992f3f.

@andrewkozlik andrewkozlik requested a review from onvej-sl Jun 28, 2019

@onvej-sl
Copy link

left a comment

Just one note. In the very unlikely event (< 2^-127) that blinding_private_key is bigger than or equal to the curve order, secp256k1_publickey fails, since it assumes the private key is less than the order.

@onvej-sl onvej-sl requested review from andrewkozlik and removed request for andrewkozlik Jun 30, 2019

@andrewkozlik

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

Just one note. In the very unlikely event (< 2^-127) that blinding_private_key is bigger than or equal to the curve order, secp256k1_publickey fails, since it assumes the private key is less than the order.

I see the following options:

  1. Leave it as is. Mathematically the operation blinding_public_key = blinding_private_key * G is well defined. The fact that trezor-crypto requires the private key to be in a certain range is merely a restriction of the implementation. The programmer implementing SLIP-0077 with trezor-crypto should use blinding_private_key mod secp256k1_curve_order to comply with this restriction.
  2. Define blinding_private_key := HMAC_SHA256(key=master_blinding_key, msg=script_pubkey) mod secp256k1_curve_order. This is equivalent to the previous option, but ensures that nobody makes a mistake implementing it.
  3. Define blinding_private_key := HMAC_SHA256(key=master_blinding_key, msg=script_pubkey) mod 2^255. This is not equivalent to either of the above. It loses one bit of entropy, but ensures that all possible blinding keys have uniform distribution of probability.

In cases 1 and 2 the distribution of the keys is non-uniform, which is something I was a little concerned about when I first saw your proposal. However, realizing that the likelihood of hitting one of the keys which have higher probability is about 2^-127, I wasn't bothered about it. If you are already using this scheme in production, then 3 isn't an option. In that case I would see 2 as the best solution just to be on the safe side, but I am fine with any of the above.

@romanz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

Good catch, thanks!
IIUC, Elements' code returns an "invalid" key if the private key is bigger than the group order:
https://github.com/ElementsProject/elements/blob/7b302eb99d2d1bf861429d6bd209771f634b8e8e/src/wallet/wallet.cpp#L5583-L5617
If it happens in getnewaddress RPC, the resulting address is not blinded (i.e. it returns a regular bitcoin address, without a blinding key).
@real-or-random WDYT?

@real-or-random
Copy link

left a comment

Actually option 3 is the dangerous one here. Yes, it ensures that the key is uniformly distributed among all keys generated by this method but what we want is that the key is uniformly distributed in the keyspace. 1 and 2 are both fine, they're negligibly close to what we need. The easiest thing to do for the implementation is to bail out if this case happens (because it won't happen), so I suggest option 1, ignoring the issue.

blinding_public_key := secp256k1_publickey(private_key=blinding_private_key)
```

The receiver is using `blinding_public_key` construct a "blinded address", which is used by the sender to blind the relevant transaction outputs. Each such blinded transaction output also contains the sender's ECDH public key, so the receiver would be able to recover the shared nonce using its `blinding_private_key`.

This comment has been minimized.

Copy link
@real-or-random

real-or-random Jul 1, 2019

nits:

  • typo: "to" construct
  • hm, "blinded address" is a somewhat misleading term because the address itself is not blinded

This comment has been minimized.

Copy link
@romanz

romanz Jul 1, 2019

Author Contributor

Thanks!
Should be fixed at e394776.

@onvej-sl

This comment has been minimized.

Copy link

commented Jul 1, 2019

Mathematically the operation blinding_public_key = blinding_private_key * G is well defined. The fact that trezor-crypto requires the private key to be in a certain range is merely a restriction of the implementation.

The operation blinding_public_key = blinding_private_key * G is well defined, but the standard refers to the function secp256k1_publickey, which is defined here.

EDITED: Not sure whether it is an coincidence or purpose.

@onvej-sl

This comment has been minimized.

Copy link

commented Jul 1, 2019

IIUC, Elements' code returns an "invalid" key if the private key is bigger than the group order:
https://github.com/ElementsProject/elements/blob/7b302eb99d2d1bf861429d6bd209771f634b8e8e/src/wallet/wallet.cpp#L5583-L5617

Well done. Maybe it should be written in the standard, just to show we are aware of the possibility of 'overflow'.

@romanz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

Maybe it should be written in the standard, just to show we are aware of the possibility of 'overflow'.

Thanks!
Added a note about this issue at e394776.

@andrewkozlik andrewkozlik merged commit 936569f into satoshilabs:master Jul 3, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.