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

Explain that ecrecover only accepts v={27,28} #860

Merged
merged 3 commits into from Jun 8, 2022

Conversation

axic
Copy link
Member

@axic axic commented Jun 7, 2022

Two conditions have been implemented in clients:

  1. The leading 31 bytes of the v value MUST be 0
  2. For the last byte the values 27 and 28 are the only accepted ones

The first condition has been thoroughly tested (CALLCODEEcrecoverV_prefixedf0), the second one partially.

Client implementations:

Paper.tex Outdated Show resolved Hide resolved
Co-authored-by: Alex Beregszaszi <alex@rtfs.hu>
Paper.tex Outdated Show resolved Hide resolved
Co-authored-by: Andrew Ashikhmin <34320705+yperbasis@users.noreply.github.com>
@axic
Copy link
Member Author

axic commented Jun 8, 2022

And finally this section doesn't mention that the precompile is not performing the check defined under (311).

@yperbasis how about adding another argument to ecdsarecover() for enabling that extra check? Would do it in a separate PR though.

@yperbasis
Copy link
Member

And finally this section doesn't mention that the precompile is not performing the check defined under (311).

@yperbasis how about adding another argument to ecdsarecover() for enabling that extra check? Would do it in a separate PR though.

Instead of another argument to ecdsarecover() I'd add the checks to the precompile formula. But agree that it'd be better done in a separate PR. I can draft it after this one is merged.

@yperbasis yperbasis merged commit c44e0fc into ethereum:master Jun 8, 2022
@axic axic deleted the patch-1 branch June 8, 2022 13:36
@axic
Copy link
Member Author

axic commented Jun 8, 2022

I can draft it after this one is merged.

Thanks! Probably would also move the h, v, r, s definitions after g_r.

@charles-cooper
Copy link

note for future reference: py-evm implementation here

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

3 participants