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

refactor: cleanup ed25519_verify #7955

Merged
merged 3 commits into from
Oct 31, 2022
Merged

refactor: cleanup ed25519_verify #7955

merged 3 commits into from
Oct 31, 2022

Conversation

matklad
Copy link
Contributor

@matklad matklad commented Oct 28, 2022

  • 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

* 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.
@matklad matklad requested a review from jakmeier October 28, 2022 14:23
@matklad matklad requested a review from a team as a code owner October 28, 2022 14:23
@matklad
Copy link
Contributor Author

matklad commented Oct 28, 2022

cc @blasrodri

@matklad
Copy link
Contributor Author

matklad commented Oct 28, 2022

After this PR is done merged, I think what's left is only estimator work. @jakmeier sounds like a good chance to write some docs on how to add estimations :)

@blasrodri
Copy link
Contributor

After this PR is done merged, I think what's left is only estimator work. @jakmeier sounds like a good chance to write some docs on how to add estimations :)

LGTM

let num_bytes = msg.len();
self.gas_counter.pay_per(ed25519_verify_byte, num_bytes as _)?;
let message = self.get_vec_from_memory_or_register(message_ptr, message_len)?;
self.gas_counter.pay_per(ed25519_verify_byte, message.len() as u64)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, this is something I am not sure about: in which order should we load and charge for things? It seems not unreasonable to, eg, charge this after we load public key, so that we dont' charge if the pk is invalid.

Basically, I see two valid strategies:

  • charges at the earliest possible point, as soon as we know parameters
  • as late as possible, right before we execute operation which is being charged for

I don't see any important differences between the two, though they are definitely observable.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point. I agree that it doesn't matter from a security perspective.

Since the order is part of protocol spec, perhaps the more important question should be which order is more natural for (future) client implementations. But I think here it doesn't matter. We dictate the order in which parameters are read anyway because get_vec_from_memory_or_register charges gas and could fail. So, I don't really see a difference, any choice seems to be fine.

Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

Looks good. Optionally clarify the return value but I am fine also if we say it's obvious that validate returning 0 means false which means failure.

runtime/near-vm-logic/src/logic.rs Outdated Show resolved Hide resolved
Co-authored-by: Jakob Meier <mail@jakobmeier.ch>
@near-bulldozer near-bulldozer bot merged commit bd1b60e into near:master Oct 31, 2022
@matklad matklad deleted the m/ed branch October 31, 2022 11:57
nikurt pushed a commit that referenced this pull request Nov 7, 2022
* 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 pull request Nov 9, 2022
* 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
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.

3 participants