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

feat: Add precompiled contracts for alt_bn128 curve [rebased] #3971

Merged
merged 38 commits into from
Mar 2, 2021

Conversation

evgenykuzyakov
Copy link
Collaborator

@evgenykuzyakov evgenykuzyakov commented Feb 17, 2021

Merged latest.

Based on #2842 by @snjax

This feature adds to near following precompiled contracts:

TODO:

  • Update Cargo.toml files to properly expose feature
  • Update test-contract to use feature
  • Use latest VMLogic to pass protocol version
  • Expose nightly feature
  • Add contract tests
  • Wrap errors with feature
  • Update param estimator to use different versions of test contracts
  • Re-run param estimator.

Test plan:

  • CI
  • Added tests to test-contract-rs in near-vm-runner.
  • Doc tests for alt_bn128 within near-vm-logic

@artob artob added the A-EVM Area: Native EVM implementation and support label Feb 19, 2021
@artob
Copy link
Contributor

artob commented Feb 19, 2021

@evgenykuzyakov Depending on when this would be merged, let's coordinate with #3954.

@evgenykuzyakov
Copy link
Collaborator Author

Here is the param estimator run with 200000/20000 accounts: https://gist.github.com/evgenykuzyakov/5ac71f68da3eab1d37dae33c60c35eda

      "alt_bn128_g1_multiexp_base": 237668976500,
      "alt_bn128_g1_multiexp_byte": 1111697487,
      "alt_bn128_g1_sum_base": 1058438125,
      "alt_bn128_g1_sum_byte": 25406181,
      "alt_bn128_g1_multiexp_sublinear": 1441698,
      "alt_bn128_pairing_check_base": 3228502967000,
      "alt_bn128_pairing_check_byte": 8858396182

@evgenykuzyakov evgenykuzyakov merged commit 8d04bcb into master Mar 2, 2021
@evgenykuzyakov evgenykuzyakov deleted the feature/alt_bn128_fork branch March 2, 2021 20:55
@abacabadabacaba
Copy link
Collaborator

There is an issue with the way these functions accept arguments. Basically, the arguments to all these functions are arrays of fixed-sized structures. The functions take their arguments in Borsh-serialized form: first 4 bytes encode the length, and the remaining bytes are the concatenation of encoded elements.

It is easy to notice that the length is redundant: since the size of each element is the same, the number of elements can be easily computed from the length of the input. Also, any code that is going to call these functions (e.g. SDK code) will already have these structures as an array, but without the length prepended. So, it will probably have to copy the entire array just to prepend this value which is useless anyway.

Also, this code charges gas per byte, while it would be more correct to charge it per element. So, replacing Borsh deserialization with a trivial one (check that the input length is a multiple of the number of bytes per element, then decode each element) would make the code more efficient both on the contract side and on the runtime side.

@bowenwang1996
Copy link
Collaborator

@evgenykuzyakov please take a look at what @abacabadabacaba said

@evgenykuzyakov
Copy link
Collaborator Author

evgenykuzyakov commented Mar 17, 2021

There is an issue with the way these functions accept arguments. Basically, the arguments to all these functions are arrays of fixed-sized structures. The functions take their arguments in Borsh-serialized form: first 4 bytes encode the length, and the remaining bytes are the concatenation of encoded elements.

@abacabadabacaba

But all other parameters still has to be serialized with Borsh. You don't just copy them in. So you have to manually serialize every element to save 4 bytes. You still need a copy.

@abacabadabacaba
Copy link
Collaborator

@evgenykuzyakov Isn't it possible to take the values directly from a slice, without Borsh serialization? Suppose that you have a Rust slice with those structures inside a smart contract, and you pass its address and length (in bytes) to the host function, can't the host function just decode this data? In this case, no Borsh is needed on the smart contract side at least.

@evgenykuzyakov
Copy link
Collaborator Author

Isn't it possible to take the values directly from a slice, without Borsh serialization? Suppose that you have a Rust slice with those structures inside a smart contract, and you pass its address and length (in bytes) to the host function, can't the host function just decode this data? In this case, no Borsh is needed on the smart contract side at least.

it's possible to do, but the inner structure are still Borsh encoded. The inner structure memory alignment is not guaranteed to match Borsh alignment. While Borsh specific serialization gives such guarantee.

@abacabadabacaba
Copy link
Collaborator

As far as I understand, the inner structures basically consist of fixed-size integers, and their in-memory representation matches their Borsh representation. So Borsh serialization will mostly just copy data in memory (but in an inefficient way), and that can be avoided just by not using Borsh serialization and instead passing the in-memory data directly. In any case, the alignment of any values in smart contract memory must be deterministic and repeatable (because the execution of smart contracts in general is deterministic and repeatable).

@joshuajbouw
Copy link
Member

Just to add into this a little bit, shouldn't we maintain our own bn lib since we rely on a not very well used zeropool-bn crate which forked bn? I already had a head start over here: https://github.com/aurora-is-near/aurora-bn which can be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cryptography Area: Cryptography A-EVM Area: Native EVM implementation and support
Projects
None yet
Development

Successfully merging this pull request may close these issues.