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.

Sign up for GitHub

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

Jump to bottom

Tracking issue for NEP-364: ed25519_verify host function #7567

Closed
7 tasks done
matklad opened this issue Sep 6, 2022 · 11 comments
Closed
7 tasks done

Tracking issue for NEP-364: ed25519_verify host function #7567

matklad opened this issue Sep 6, 2022 · 11 comments
Labels
A-contract-runtime Area: contract compilation and execution, virtual machines, etc C-tracking-issue Category: a tracking issue T-contract-runtime Team: issues relevant to the contract runtime team

Comments

@matklad
Copy link
Contributor

matklad commented Sep 6, 2022 •
edited by jakmeier
Loading

NEP: near/NEPs#364
Implementation: #7165

Stabilization PR:

Unresolved questions:

  • semantics for errors
  • correctly check signature length. The current code is buggy: length is either length or or a flag to load from register. We should be checking the length of the vector we read from memory or register.
  • add tests for all error conditions we have inside the function (invalid sig, invalid pk, wrong length of either, empty message)
  • use realistic benchmark for cost computation. Today we use short message (32 bytes), not clear if that's the worst case
  • make sure that estimator checks the result (it's not clear whether we estimate failing or successful verification)
  • put the realistic costs into configuration
@matklad matklad added A-contract-runtime Area: contract compilation and execution, virtual machines, etc T-contract-runtime Team: issues relevant to the contract runtime team labels Sep 6, 2022
This was referenced Sep 6, 2022
Merged
Merged
@akhi3030
Copy link
Collaborator

akhi3030 commented Sep 7, 2022

Do all of these need to be addressed before the feature can be stabilised? If not, should we highlight which are essential for stabilisation?

@matklad
Copy link
Contributor Author

matklad commented Sep 7, 2022 •
edited
Loading

Yes, they all are mandatory for the standard of quality we want to maintain. They are ordered roughtly in order of priority though.

@matklad matklad added the C-tracking-issue Category: a tracking issue label Sep 20, 2022
@matklad
Copy link
Contributor Author

matklad commented Sep 22, 2022

cc @blasrodri as the author of the original impl

@blasrodri
Copy link
Contributor

I'll start working on these next week. Will be needing your support on the cost part @jakmeier

blasrodri added a commit to blasrodri/nearcore that referenced this issue Oct 11, 2022
@blasrodri
logic: improve test coverage and error messages of ed25519_verify
bd8d375 
ref: near#7567
@blasrodri blasrodri mentioned this issue Oct 11, 2022
Merged
@matklad matklad changed the title Tracking issue for NEP-364 Tracking issue for NEP-364: ed25519_verify host function Oct 19, 2022
@ijerkovic ijerkovic mentioned this issue Oct 26, 2022
Open
near-bulldozer bot pushed a commit that referenced this issue Oct 28, 2022
@blasrodri
logic: improve test coverage and error messages of ed25519_verify (#7804
46dc515 
)

ref: #7567
@matklad matklad mentioned this issue Oct 28, 2022
Merged
near-bulldozer bot pushed a commit that referenced this issue Oct 31, 2022
@matklad
refactor: cleanup ed25519_verify (#7955)
bd1b60e 
* Align doc comment with other host functions (not that we are super-consistent here)
* Align code style with other host functions
* Reduce some duplication from tests
* Add tricky test-cases where a value of the wrong length is read from the register.

#7567
@jakmeier jakmeier mentioned this issue Nov 1, 2022
Merged
@matklad
Copy link
Contributor Author

matklad commented Nov 2, 2022

I guess another useful thing to have before stabilization is to use this nightly function in some contract on the beta net. So, if someone's already using this, please ping this issue, even if you don't have any problems )

nikurt pushed a commit that referenced this issue Nov 7, 2022
@blasrodri @nikurt
logic: improve test coverage and error messages of ed25519_verify (#7804
552a629 
)

ref: #7567
nikurt pushed a commit that referenced this issue Nov 7, 2022
@matklad @nikurt
refactor: cleanup ed25519_verify (#7955)
7dff269 
* Align doc comment with other host functions (not that we are super-consistent here)
* Align code style with other host functions
* Reduce some duplication from tests
* Add tricky test-cases where a value of the wrong length is read from the register.

#7567
nikurt pushed a commit that referenced this issue Nov 9, 2022
@blasrodri @nikurt
logic: improve test coverage and error messages of ed25519_verify (#7804
2dd4c90 
)

ref: #7567
nikurt pushed a commit that referenced this issue Nov 9, 2022
@matklad @nikurt
refactor: cleanup ed25519_verify (#7955)
4085ad6 
* Align doc comment with other host functions (not that we are super-consistent here)
* Align code style with other host functions
* Reduce some duplication from tests
* Add tricky test-cases where a value of the wrong length is read from the register.

#7567
@jakmeier jakmeier mentioned this issue Nov 11, 2022
Merged
@matklad
Copy link
Contributor Author

matklad commented Nov 11, 2022

This is more or less ready for stabilization I think, though

I guess another useful thing to have before stabilization is to use this nightly function in some contract on the beta net. So, if someone's already using this, please ping this issue, even if you don't have any problems )

would still be a nice sanity check

@jakmeier
Copy link
Contributor

jakmeier commented Jan 12, 2023 •
edited
Loading

@blasrodri to get this into the release pipeline, we still need a stabilization PR that stabilizes the feature flag and removes some of the #[cfg] that's no longer needed. Otherwise this stays in nightly (betanet) forever.

@blasrodri
Copy link
Contributor

@blasrodri to get this into the release pipeline, we still need a stabilization PR that stabilizes the feature flag and removes some of the #[cfg] that's no longer needed. Otherwise this stays in nightly (betanet) forever.

got it! will take care of it :)

@blasrodri
Copy link
Contributor

I have made this one some time ago:
#8098

@jakmeier
Copy link
Contributor

Oh, sorry I missed that completely :(
I will review it and once it's merge to master, it will go to testnet with the next testnet release.

@jakmeier
Copy link
Contributor

Closing this as the feature has been merged to master. (If anything comes up while testing on testnet, we can always reopen.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-contract-runtime Area: contract compilation and execution, virtual machines, etc C-tracking-issue Category: a tracking issue T-contract-runtime Team: issues relevant to the contract runtime team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@matklad @blasrodri @akhi3030 @jakmeier