NEP-364: Efficient signature verification host functions

[blasrodri]

Efficient signature verification and hashing precompile functions

[mikedotexe]

bump 😇

[nagisa]

Largely just wording and structure nits from me.

[bowenwang1996]

This NEP seems to imply that compiling these functions to wasm is actually feasible, though not desirable, for IBC use cases. I'd like to see a more detailed discussion on the alternatives and cost of not introducing this change. In general, we should be very carefully with adding more host functions.

[bowenwang1996]

What does "we" here refer to? How is the number 200 calculated?

[bowenwang1996]

Is one iteration one signature verification?

[blasrodri]

Yes

[blasrodri]

Agree. For each host function, please explain why exactly they are needed and their specification

I've added a reference in the motivation section. Would you like it to be more specific, being included in the comment of each host function?

[bowenwang1996]

I don't necessarily agree that it is a safe workaround. The cost of introducing a new host function is actually quite high:

• A bug in the host function code would cause the entire network to go down
• Once the host function is introduced, one cannot remove it as that could break contracts which already rely on the behavior of the host functions.

[blasrodri]

Maybe "safe workaround" isn't the right way of putting it. However, these are highly adopted libraries with operations that are part of multiple services and blockchains. In that way, I was trying to convey the fact that these are no new functions that haven't been battle-tested. If you have any suggestions on how to communicate it differently I'd be happy to update it.

[blasrodri]

Thanks for the feedback. I've incorporated your comments.

I've removed the hashing function from this NEP so that it only focuses on signature verification. We'll likely need another one for introducing new hashes that aren't supported.

[blasrodri]

This NEP seems to imply that compiling these functions to wasm is actually feasible, though not desirable, for IBC use cases. I'd like to see a more detailed discussion on the alternatives and cost of not introducing this change. In general, we should be very carefully with adding more host functions.

I've mentioned the size of the validator set, and how there are plans to increase it. With the current gas costs and performance of the wasm code, we can only verify 130 signatures on one transaction using the most expensive scheme. Based on this discussed proposal the validator set can be increased to 1000, which will require verifying at least 667 signatures.

Based on the performance improvement of 20-25x, doing this natively gives us sufficient breathing room for these numbers and makes the infrastructure a bit more "future-proof".

[abacabadabacaba]

I'm totally fine with adding Ed25519 (specifically, the variant that NEAR is already using), and we already have a host function for secp256k1. However, I am against the idea of adding Sr25519.

In general, I think that we should strive to reduce the number of host functions that we have, and only add new ones if there is a compelling need. Currently, almost all of the Math API host functions implement functionality that is available in the EVM. EVM is used in many blockchains, and is de facto a standard. Ed25519 is also widely used in NEAR and elsewhere. Sr25519, however, has very little usage. We don't want to add every cryptographic function existing somewhere, as this will increase our maintenance burden too much. In fact, I wasn't even able to find a specification for this function, and we certainly don't want to add a function without a specification.

I would recommend @blasrodri to explore the avenues for optimizing a pure-WASM implementation instead. It seems that Sr25519 implements some sort of aggregate signatures, which could be verified more quickly than verifying each signature individually. Also, it is probably possible to do batch verification, similar to Ed25519. I couldn't find it implemented though, which is yet another sign that it is not mature and/or popular enough.

[blasrodri]

I'm totally fine with adding Ed25519 (specifically, the variant that NEAR is already using), and we already have a host function for secp256k1. However, I am against the idea of adding Sr25519.

In general, I think that we should strive to reduce the number of host functions that we have, and only add new ones if there is a compelling need. Currently, almost all of the Math API host functions implement functionality that is available in the EVM. EVM is used in many blockchains, and is de facto a standard. Ed25519 is also widely used in NEAR and elsewhere. Sr25519, however, has very little usage. We don't want to add every cryptographic function existing somewhere, as this will increase our maintenance burden too much. In fact, I wasn't even able to find a specification for this function, and we certainly don't want to add a function without a specification.

I would recommend @blasrodri to explore the avenues for optimizing a pure-WASM implementation instead. It seems that Sr25519 implements some sort of aggregate signatures, which could be verified more quickly than verifying each signature individually. Also, it is probably possible to do batch verification, similar to Ed25519. I couldn't find it implemented though, which is yet another sign that it is not mature and/or popular enough.

Thanks for your input. I've cleaned up and left only Ed25519. We'll figure out Sr25519 if we need to do so. And, as you mentioned, we can use erecover for ECDSA. Please let me know if the current update is in line with what you want.

[abacabadabacaba]

@blasrodri The current update is good in terms of the functionality. There are some technical changes that are needed, though. In particular, the type of the arguments and the return value of host functions must be u32 or u64 for technical reasons (near-sdk can provide a wrapper function with a more convenient signature).

[matklad]

One interesting unresolved question came up during implementation: what's the proper error handling for invalid keys and signatures?

Not every byte sequence is a syntactically valid key/sig, so we have to do something about that.

Our choices are:

• return false (ie, just consider this to be invalid signature for the message)
• return some kind of error (so, the return would be ok | invalid key | invalid sig | wrong sig)
• consider this to be a programming error, and terminate contract execution.

There are two reasons why a key or a signature might be invalid.

First, the byte length could be wrong. I think it's pretty clear that this should just be programming error -- the contract should pass [u8; 32] to the most function call.

Second, even if the length is correct, the key or signature might still be considered invalid due to this conditions:

• https://docs.rs/ed25519-dalek/latest/src/ed25519_dalek/public.rs.html#142
• https://docs.rs/ed25519/latest/src/ed25519/lib.rs.html#302

In the case of invalid signature, I am somewhat confident that the right approach is just return false. It seems logical that, if the signature is trivially invalid, than it's just an invalid signature.

I don't know what's the right answer for invalid public key. On the one hand, public keys is usually something you get from the store, so you can consider them valid. On the other hand, if someone creates a public key, you'd have to have some mechanism to ensuring that it is in fact valid. I think return false is probably the right strategy here as well, but I'd love to hear from people more knowledgeable than me :)

In any case, error handling behavior should be documented as a part of the nep.

[frol]

@blasrodri Please, revert the changes to the README.md file. We will automate the process of listing NEPs, but for now, I will update it manually after we merge the NEP.

[frol]

Thank you to everyone who attended the first Protocol Working Group meeting today! The working group members reviewed the NEP and reached the following consensus:

Status: Approved with a request to address remaining comments (see below)

• @bowenwang1996
• @mfornet
• @mm-near

Short summary: All the decisions and major concerns were raised and addressed before the meeting, so the call was short and productive. It was clarified again that this NEP will allow IBC implementations to have a simpler design (as stated in the Summary and Motivation sections of this NEP). There were no blocking concerns to approving this NEP, so the Protocol Working Group approved it with a request to specify the corner cases in the NEP explicitly (see the list below) before merging it.

Meeting Recording: https://youtu.be/Uzvv6i1zKJY

@blasrodri Thank you for your contribution to the future of NEAR Protocol!

Remaining requests to @blasrodri:

• Address the comment from @matklad: NEP-364: Efficient signature verification host functions #364 (comment) (specify the invalid input behavior)
• Address the comment from @abacabadabacaba: NEP-364: Efficient signature verification host functions #364 (review) (specify the exact ED25519 mode that is expected to be used)

Next steps:

• @blasrodri should address the remaining comments
• @frol should merge NEP and list it in the README
• nearcore team should track the implementation and stabilization: Tracking issue for NEP-364: ed25519_verify host function nearcore#7567

[blasrodri]

@matklad @abacabadabacaba I've addressed your comments on commit
bdf64d0
Feel free to let me know if there's anything that can be improved.

[frol]

@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?

[abacabadabacaba]

@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.

[frol]

@abacabadabacaba Thank you!

@blasrodri Please, include the proposed details into the NEP

[blasrodri]

@frol updated it

[abacabadabacaba]

@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.

[frol]

@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?

[matklad]

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

[blasrodri]

Updated it

[frol]

@blasrodri It seems there is one last remaining suggestion from @abacabadabacaba.

@matklad Do you feel all your comments were addressed and we are good to merge this NEP?

[matklad]

Yeah, this looks good to me.

[blasrodri]

@blasrodri It seems there is one last remaining suggestion from @abacabadabacaba.

@matklad Do you feel all your comments were addressed and we are good to merge this NEP?

thanks for following up. I've updated @abacabadabacaba comments. Hopefully that's what he meant.

[abacabadabacaba]

LGTM

[abacabadabacaba]

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

[blasrodri]

removed

[frol]

Congrats everyone! I am starting the merge process now

@blasrodri Thanks for championing this effort!

Thanks to all the reviewers for your thorough work!

[jakmeier]

Stabilization has just been merged: near/nearcore#8098

Expect this to land in testnet with the next testnet release.