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

NEP-364: Efficient signature verification host functions #364

Merged
merged 24 commits into from
Oct 6, 2022
Merged

NEP-364: Efficient signature verification host functions #364

Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
34a9391
NEP-364
blasrodri Jun 15, 2022
1d4dc44
correct some grammar
blasrodri Jun 15, 2022
56d08f0
add more benchmarks
blasrodri Jun 15, 2022
0129422
Update README.md
blasrodri Jul 6, 2022
8962147
incorporate comments
blasrodri Jul 12, 2022
67ac13b
comment on the number of signers
blasrodri Jul 12, 2022
055bd09
Update neps/nep-0364.md
blasrodri Jul 12, 2022
70265a5
Update neps/nep-0364.md
blasrodri Jul 12, 2022
d6c0771
add alternatives
blasrodri Jul 12, 2022
a7789b3
add comments and specs on functions
blasrodri Jul 13, 2022
1d2777f
cleanup algorithms except for ed25519
blasrodri Jul 21, 2022
479edff
rm reference to old functions
blasrodri Jul 21, 2022
5ed5b18
typos and spec conforming with wasm
blasrodri Jul 21, 2022
1e8d3df
return u64
blasrodri Jul 22, 2022
80ac231
minor adjustments
blasrodri Aug 17, 2022
f3b5c39
update cost (probably WIP)
blasrodri Aug 17, 2022
e3af2cb
update docs
blasrodri Aug 17, 2022
ba8a3d3
revert README
blasrodri Sep 6, 2022
bdf64d0
addressing comments
blasrodri Sep 7, 2022
59257ca
address comments
blasrodri Sep 22, 2022
d25d8f8
rm curly brace
blasrodri Sep 22, 2022
3dac4ea
document crate decisions
blasrodri Oct 3, 2022
73e0290
updated comments
blasrodri Oct 5, 2022
003493e
rm redundant sentence
blasrodri Oct 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions neps/nep-0364.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,15 @@ This NEP aims to introduce the following host function:
///
/// `input_cost(num_bytes_signature + num_bytes_message, num_bytes_public_key) +
/// ed25519_verify_base + ed25519_verify_byte * (num_bytes_signature + num_bytes_message)`
matklad marked this conversation as resolved.
Show resolved Hide resolved
///
/// # Errors
///
/// The signature size is fixed and equal to SIGNATURE_LENGTH (64 bytes). In case the length of signature input is not equal to
/// SIGNATURE_LENGTH, then the function returns HostError::Ed25519VerifyInvalidInput with the message "invalid signature length".
///
/// A similar case occurs with the public key. Its size is known and its equal to PUBLIC_KEY_LENGTH (32 bytes). In case the
/// input length provided for the publc key doesn't match PUBLIC_KEY_LENGTH, the function will return the following error:
/// HostError::Ed25519VerifyInvalidInput with the message "invalid public key length".
Copy link
Collaborator

Choose a reason for hiding this comment

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

@matklad Do you think it is a good direction to go with extending HostError type or should we keep the scope of potential errors minimal?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this doesn't matter much: from the protocol perspective, how exactly HostError looks is unobservable. Implementation wise, adding a new variant there is OK.

But this actually points to the problem with wording of this NEP: what is written here is how the function would look inside logic.rs, but that's an impl detail which is completely irrelevant from the specification point of view.

NEPs should say how the WASM import looks, not how it is implemented. For the time being, I'd rephrase this section as:

== Specification

The following host function is added

extern "C"{

/// Verify an ED25519 signature given a message and a public key.
/// # Returns
/// - 1 if the signature was properly verified
/// - 0 if the signature failed to be verified
///
/// # Cost
///
/// Each input can either be in memory or in a register. Set the length of the input to `u64::MAX`
/// to declare that the input is a register number and not a pointer.
/// Each input has a gas cost input_cost(num_bytes) that depends on whether it is from memory
/// or from a register. It is either read_memory_base + num_bytes * read_memory_byte in the
/// former case or read_register_base + num_bytes * read_register_byte in the latter. This function
/// is labeled as `input_cost` below.
///
/// `input_cost(num_bytes_signature + num_bytes_message + num_bytes_public_key) +
///  ed25519_verify_base + ed25519_verify_byte * num_bytes_message`
///
/// # Errors
///
/// If the signature size is not equal to 64 bytes, or public key length is not equal to 32 bytes, contract execution is terminated with an error. 
  fn ed25519_verify(
    sig_len: u64,
    sig_ptr: u64,
    msg_len: u64,
    msg_ptr: u64,
    pub_key_len: u64,
    pub_key_ptr: u64,
  ) -> u64;
}

That is:

  • write the exten function from perspecive of the contract
  • don't mention HostError, as that is unobservable to contracts (it is more or less like panic)

We could perhaps consider using proper wasm for the spec here

(module 
  (import "env" "ed25519_verify" (func (param 64) ... (result i64)))
)

but I feel that Rust-syntax to experss that is more accessible, and better matches what we've been doing historically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it

pub fn ed25519_verify(
&mut self,
sig_len: u64,
Expand All @@ -164,6 +173,8 @@ And a `rust-sdk` possible implementation could look like this:
pub fn ed25519_verify(sig: &ed25519::Signature, msg: &[u8], pub_key: &ed25519::Public) -> bool;

```
The current implementation is imported from the crate `ed25519-dalek`, version 1. It uses no feature flags
except from the default one. This is the exact same setup used in the crate `near-crypto`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@abacabadabacaba Can you maybe help to identify how to state this requirement in a crate-agnostic way, so we don't get tied to the exact implementation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@frol It is difficult to describe the specific behavior concisely without referring to a specific implementation. This blog post describes the various ways in which the existing Ed25519 implementations differ in practice. The behavior that we are using, which is shared by Go crypto/ed25519, Rust ed25519-dalek (using verify function with legacy_compatibility feature turned off) and several others, makes the following decisions:

  • The encoding of the values $R$ and $s$ must be canonical, while the encoding of $A$ doesn't need to.
  • The verification equation is $R=[s]B-[k]A$.
  • No additional checks are performed. In particular, the points outside of the order-$l$ subgroup are accepted, as are the points in the torsion subgroup.

While the above hopefully describes the behavior that we seek in an unambiguous manner, we may still want to point to a specific implementation to refer to in case of an ambiguity.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@abacabadabacaba Thank you!

@blasrodri Please, include the proposed details into the NEP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@frol updated it

Copy link
Collaborator

Choose a reason for hiding this comment

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

@blasrodri Probably Security Implications is not the most appropriate place for this, this should probably rather go just above this thread. Also, whenever the ed25519-dalek crate is mentioned, it should be specified that verify function should be used. This is because there are other verification functions in this crate (verify_strict, verify_batch) that behave differently.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This sentence now seems redundant, given that the expected behavior is described below more thoroughly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed


Once this NEP is approved and integrated, these functions will be available in the `near_sdk` crate in the
`env` module.
Expand Down