NEP-364 #7165

blasrodri · 2022-07-05T07:37:45Z

Implementing the following host functions according to the design proposed in NEP-364

    pub fn ed25519_verify(
        &mut self,
        sig_len: u64,
        sig_ptr: u64,
        msg_len: u64,
        msg_ptr: u64,
        pub_key_len: u64,
        pub_key_ptr: u64,
        register_id: u64,
    ) -> Result<()>;

added unit tests
added costs (this needs to be reviewed, since I'm not sure how to infer the right numbers)

WIP

seunlanlege · 2022-07-05T15:30:37Z

Absolutely epic work 🙌🏿

For tracking we're missing:

in-memory trie construction: https://github.com/paritytech/substrate/blob/6a946fc36d68b89599d7ca1ab03803d10c78468c/primitives/io/src/lib.rs#L515-L520
membership and non-membership trie verification: https://github.com/ComposableFi/composable/blob/be2aebf23aa38888a7ceba1c4a2b698500497e83/frame/ibc/src/host_functions.rs#L36-L61

hitting gas limits

akhi3030 · 2022-07-11T14:11:41Z

As someone with the most context here, assigning self as reviewer here. Once the PR is ready for proper review, will do a detailed review and / or delegate as needed.

runtime/near-vm-logic/src/logic.rs

akhi3030 · 2022-07-12T14:27:37Z

@blasrodri: PTAL: https://near.zulipchat.com/#narrow/stream/295306-pagoda.2Fcontract-runtime/topic/NEP-364. There are some interesting discussions about this PR.

blasrodri · 2022-07-13T08:53:55Z

@akhi3030 I've removed all non signature verification related code. So that hashing related functions are moved onto another NEP (will add it shortly).

I'm not sure about #7165 (comment)
What's needed from my side?

…signature-verif-host-functions

akhi3030 · 2022-07-13T09:15:52Z

@akhi3030 I've removed all non signature verification related code. So that hashing related functions are moved onto another NEP (will add it shortly).

I'm not sure about #7165 (comment) What's needed from my side?

Thanks @blasrodri! This comment in the zulip discussion was particularly interesting: https://near.zulipchat.com/#narrow/stream/295306-pagoda.2Fcontract-runtime/topic/NEP-364/near/289315918. We have to figure out how to make sure that all the validations are deterministic. I recommend having a chat with @evgenykuzyakov to further understand the concerns.

runtime/runtime-params-estimator/costs.txt

runtime/near-vm-logic/src/logic.rs

Co-authored-by: Jakob Meier <mail@jakobmeier.ch> Signed-off-by: Blas Rodriguez Irizar <rodrigblas@gmail.com>

blasrodri · 2022-08-22T09:41:25Z

@jakmeier I just ran it on another machine (M1)
using

export CARGO_PROFILE_RELEASE_CODEGEN_UNITS=1;
export CARGO_PROFILE_RELEASE_LTO=fat;

     Running `target/release/runtime-params-estimator --accounts-num 20000 --additional-accounts-num 20000 --iters 5 --warmup-iters 1 --metric time --costs Ed25519VerifyBase,Ed25519VerifyByte`
[elapsed 00:00:03 remaining 00:00:00] Writing into storage ████████████████████   20000/20000  
Ed25519VerifyBase                                   73_232_640_700 gas [                 73.233µs]           (computed in 17.18s) 
Ed25519VerifyByte                                      754_106_187 gas [                    754ns]           (computed in 9.14s)

jakmeier · 2022-08-22T12:07:44Z

@jakmeier I just ran it on another machine (M1) using

export CARGO_PROFILE_RELEASE_CODEGEN_UNITS=1;
export CARGO_PROFILE_RELEASE_LTO=fat;

     Running `target/release/runtime-params-estimator --accounts-num 20000 --additional-accounts-num 20000 --iters 5 --warmup-iters 1 --metric time --costs Ed25519VerifyBase,Ed25519VerifyByte`
[elapsed 00:00:03 remaining 00:00:00] Writing into storage ████████████████████   20000/20000  
Ed25519VerifyBase                                   73_232_640_700 gas [                 73.233µs]           (computed in 17.18s) 
Ed25519VerifyByte                                      754_106_187 gas [                    754ns]           (computed in 9.14s)

Yeah that looks about right. Good to see the uncertainty warning is gone now!

But let's keep the numbers that you already had. The M1 platform is not suited well for our estimations for a number of reasons. And we will need to do fine-tuning anyway before stabilising this.

I think the estimation part is looking good now for this PR.

@matklad do you want to take another more holistic look at the code before we merge it?

bowenwang1996 · 2022-08-26T04:36:18Z

@matklad do you mind reviewing this PR again?

matklad

Sorry, missed all the repated repings here. In general, if I don't respond to a direct @mention within a day or to, please DM me on Zulip,

I think all the infrastructure is setup correctly here, so i suggest landing this. However, the actual implementation I don't think is quite right yet, but I suggest working on that in a follow up PRs.

runtime/near-vm-logic/src/logic.rs

matklad · 2022-09-02T14:57:05Z

runtime/near-vm-logic/src/logic.rs

+        self.gas_counter.pay_base(ed25519_verify_base)?;
+        let num_bytes = msg
+            .len()
+            .checked_add(signature_array.len())


signature has a fixed size, so it doesn't makes sense to charge per-byte cost for it. So:

don'd so this check_add here, use just msg.len() to charge per-byte cost.

move signature decoding Signature::from_bytes( right after reading the signature.

@jakmeier conflicting suggestion here with regards to the byte cost:

/// ed25519_verify_base + ed25519_verify_byte * (num_bytes_signature + num_bytes_message)`

Right. I thought because we have sig_len: u64 as a parameter there is somehow a possibility that the signature length can vary now or maybe in the future. But I guess the function name makes it clear that this is only used with ed25519 which fully defines the signature and public key length.

I would say don't charge per-byte costs for PK and signature. Only for the message. And return Ed25519VerifyInvalidInput if the input length is not equal to the corresponding constant.

Unfortunately, we still need both sig_len: u64 and pub_key_len: u64 as a parameters to decide on whether to read from a register or from memory. I would love to see something more elegant but cannot think of a simple solution. It's okay to keep them IMO.

runtime/near-vm-logic/src/logic.rs

runtime/near-vm-runner/src/imports.rs

matklad · 2022-09-02T15:30:17Z

runtime/near-vm-logic/src/logic.rs

+        let signature = Signature::from_bytes(&signature_array).map_err(|e| {
+            VMLogicError::HostError(HostError::Ed25519VerifyInvalidInput { msg: e.to_string() })
+        })?;


So this could fail for two reasons:

wrong lenght of the sgnature

result.0[Signature::BYTE_SIZE - 1] & 0b1110_0000 == 0 condition

I wonder if the latter should lead to HostError, or to a 0 result? It naively seems that 0 is a better option here? This should be explicitly decided in the NEP.

I'm wondering if giving a HostError is better to provide the caller with more context?

…ure-verif-host-functions

matklad · 2022-09-06T12:40:56Z

@blasrodri sorry, I forgot auto-merge label last time, could you update this, so that we get this in?

matklad · 2022-09-06T16:25:45Z

Created a tracking issue for a bunch of unresolved questions we have here: #7567

Makes estimations run again (with and) without `--features=nightly`. near#7165 introduced new methods in the estimator contract but we forgot to hide them behind cfg.

Makes estimations run again (with and) without `--features=nightly`. #7165 introduced new methods in the estimator contract but we forgot to hide them behind cfg.

Implementing the following host functions according to the design proposed in [NEP-364](near/NEPs#364) ```rs pub fn ed25519_verify( &mut self, sig_len: u64, sig_ptr: u64, msg_len: u64, msg_ptr: u64, pub_key_len: u64, pub_key_ptr: u64, register_id: u64, ) -> Result<()>; ``` - [x] added unit tests - [x] added costs (this needs to be reviewed, since I'm not sure how to infer the right numbers)

Makes estimations run again (with and) without `--features=nightly`. #7165 introduced new methods in the estimator contract but we forgot to hide them behind cfg.

blasrodri added 11 commits June 23, 2022 13:12

NEP-364

4fb409b

WIP

add sr25519_verify

fa80a57

add ecdsa_recover_compressed

af0a3a5

add sha512, sha512_truncate, sha3_512 and blake2_256

1ea1db4

correct docs

a36b0d6

rm ecdsa_recover_compressed since ecrecover exists

1bfa699

add gas for ed25519_verify

aa6e705

gas for sr25519_verify

a214459

add code ...

cc16399

add gas cost for sha3_512 and blake2_256

1b76834

doc costs in logic

6ce6538

blasrodri requested a review from a team as a code owner July 5, 2022 07:37

blasrodri requested a review from mina86 July 5, 2022 07:37

blasrodri added 2 commits July 5, 2022 09:50

Merge branch 'master' into blas-implement-signature-verif-host-functions

9b692bb

sha512 missing

3005273

blasrodri mentioned this pull request Jul 5, 2022

NEP-364 ComposableFi/nearcore#1

Draft

blasrodri added 3 commits July 7, 2022 13:27

implement verify_membership_trie_proof

cdb663a

adding test_verify_membership_trie_proof

ccc63e2

add verify_non_membership_trie_proof and test

8f32a0f

hitting gas limits

akhi3030 requested review from akhi3030 and removed request for mina86 July 11, 2022 14:10

nagisa reviewed Jul 11, 2022

View reviewed changes

keep only signature verification functions

6cc895f

Merge remote-tracking branch 'composable/master' into blas-implement-…

894353d

…signature-verif-host-functions

blasrodri mentioned this pull request Aug 3, 2022

NEP-364: Efficient signature verification host functions near/NEPs#364

Merged

blasrodri added 2 commits August 11, 2022 12:36

estimations are working

fab9f1f

improve docs on gas costs for ed25519_verify

76a7e78

bowenwang1996 reviewed Aug 12, 2022

View reviewed changes

runtime/runtime-params-estimator/costs.txt Outdated Show resolved Hide resolved

adjust params and cost docs

39a69b3

jakmeier reviewed Aug 17, 2022

View reviewed changes

runtime/near-vm-logic/src/logic.rs Outdated Show resolved Hide resolved

Update runtime/near-vm-logic/src/logic.rs

75c195c

Co-authored-by: Jakob Meier <mail@jakobmeier.ch> Signed-off-by: Blas Rodriguez Irizar <rodrigblas@gmail.com>

jakmeier self-requested a review August 22, 2022 12:08

jakmeier approved these changes Aug 22, 2022

View reviewed changes

matklad approved these changes Sep 2, 2022

View reviewed changes

blasrodri added 6 commits September 5, 2022 14:46

base cost paid up-front

afe8bf3

move ed25519_verify

ca87553

move Signature::from_bytes after data read

faa0acc

format args

74e5123

add check for signature size + not pay for it

1e01af2

Merge remote-tracking branch 'near/master' into blas-implement-signat…

f56f712

…ure-verif-host-functions

matklad added the S-automerge label Sep 6, 2022

Merge branch 'master' into blas-implement-signature-verif-host-functions

5cba661

near-bulldozer bot merged commit ea96faf into near:master Sep 6, 2022

matklad mentioned this pull request Sep 6, 2022

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

Closed

7 tasks

jakmeier mentioned this pull request Sep 20, 2022

fix: estimator contract and feature_ed25519_verify #7644

Merged

jakmeier mentioned this pull request Dec 2, 2022

Exposing Function Call Access Key Info At Runtime near/NEPs#371

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEP-364 #7165

NEP-364 #7165

blasrodri commented Jul 5, 2022 •

edited

Loading

seunlanlege commented Jul 5, 2022 •

edited

Loading

akhi3030 commented Jul 11, 2022

akhi3030 commented Jul 12, 2022

blasrodri commented Jul 13, 2022

akhi3030 commented Jul 13, 2022

blasrodri commented Aug 22, 2022 •

edited

Loading

jakmeier commented Aug 22, 2022

bowenwang1996 commented Aug 26, 2022

matklad left a comment

matklad Sep 2, 2022

blasrodri Sep 5, 2022

jakmeier Sep 5, 2022

matklad Sep 2, 2022

blasrodri Sep 5, 2022

matklad commented Sep 6, 2022

matklad commented Sep 6, 2022

NEP-364 #7165

NEP-364 #7165

Conversation

blasrodri commented Jul 5, 2022 • edited Loading

seunlanlege commented Jul 5, 2022 • edited Loading

akhi3030 commented Jul 11, 2022

akhi3030 commented Jul 12, 2022

blasrodri commented Jul 13, 2022

akhi3030 commented Jul 13, 2022

blasrodri commented Aug 22, 2022 • edited Loading

jakmeier commented Aug 22, 2022

bowenwang1996 commented Aug 26, 2022

matklad left a comment

Choose a reason for hiding this comment

matklad Sep 2, 2022

Choose a reason for hiding this comment

blasrodri Sep 5, 2022

Choose a reason for hiding this comment

jakmeier Sep 5, 2022

Choose a reason for hiding this comment

matklad Sep 2, 2022

Choose a reason for hiding this comment

blasrodri Sep 5, 2022

Choose a reason for hiding this comment

matklad commented Sep 6, 2022

matklad commented Sep 6, 2022

blasrodri commented Jul 5, 2022 •

edited

Loading

seunlanlege commented Jul 5, 2022 •

edited

Loading

blasrodri commented Aug 22, 2022 •

edited

Loading