Skip to content
This repository has been archived by the owner on Aug 16, 2021. It is now read-only.

Test demonstrating ECRECOVER opcode #4183

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

codingupastorm
Copy link
Collaborator

Need to test that it is seamless to enable this in contracts

public static class EcRecover
{
// TODO: Not sure what this is yet.
private const int RecId = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so my understanding is, normally an uncompressed secp256k1 pubkey has a lot of redundant information. For a given x coordinate there are only 2 possible y values. So instead of storing the complete x and y coordinates (64 bytes), you can almost halve the space requirement by only storing x with a single byte (32 + 1 = 33 bytes) to indicate which of the solutions it is. So recId is that indicator in this case.

I would need to look into it a little to see how you are supposed to infer it when recovering from a signature. Dark magic.

Copy link
Contributor

@zeptin zeptin Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, it's coming back to me a little. There was a softfork mandating that all signature S-values past a certain block height be the 'low' value (at most N/2, where N is the curve order). So I think you'd have to try both the potential pubkeys and see which one gives that outcome.

May be mistaken, still researching.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so after looking at the NBitcoin implementation in more detail:

  1. I don't think recId should be hardcoded like this, as it can have several different values depending on whether the pubkey is compressed, and other characteristics of the curve point that comprises the pubkey. 1 may be working in your test by coincidence more than anything else.
  2. recId gets stored in the first byte of an encoded signature if you are using NBitcoin's message signing functionality anyway. An ECDSASignature instance does not have this data as it is only the raw R & S values.
  3. I would therefore suggest using PubKey.RecoverCompact() instead. See Key.SignCompact() for the operation that will return the signature in the encoded form with recId prepended. For RecoverCompact we do obviously need the hash of the message available, but that is a minor tweak of the methods you've already implemented.

Copy link
Collaborator

@rowandh rowandh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Structurally LGTM. No comment on the RecId/crypto side, will await @zeptin's continued feedback

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

Successfully merging this pull request may close these issues.

None yet

3 participants