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

[Features] support bn254_algebra in aptos_stdlib #11142

Merged
merged 25 commits into from Dec 12, 2023
Merged

Conversation

nanne007
Copy link
Contributor

@nanne007 nanne007 commented Dec 1, 2023

Description

The PR tries to add native support for BN254 elliptic curves in aptos_std::crypto_algebra and aptos_std::bn254_algebra. This PR implements all the crypto_algebra natives for BN254, except hash_to_curve (as BN254 has no support on this).

TODOs

  • Docs and tests are still missing, which I intend to add in the following commits on the PR.
  • Figure out the gas for each operations in BN254. Done by @zjma.
  • Some concern about (de)serialization of curve points like G1, G2. See discussion on halo2curves and this arkworks-rs issue

@nanne007 nanne007 changed the title Bn256 [Features] support bn254_algebra in aptos_stdlib Dec 1, 2023
@zjma zjma added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Dec 1, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@zjma
Copy link
Contributor

zjma commented Dec 1, 2023

Currently, the crypto_algebra framework keeps the actual group element data in a VM extension. Only a pointer is stored on the Move side. The actual data won't be disposed until the end of the VM session, even if the corressponding Move object is dropped in the middle. Because of that, we set a 1MB limit to the total size of the actual data in a session.

However, according to @nanne007's test, zkmove txns can easily exceed this limit.

We probably need a bigger change. A few approaches:

  1. Bump the limit.
  2. When a crypto_algebra::Element object drops, notify the VM extension to clean up.
  3. Add a crypto_algebra::drop_element(e: Element) API to let the developer manage the data.
  4. switch back to non-generic API: actual data in Move, no VM extension, and computation-only native functions. (will heavily rely on those *_unchecked() library functions to save gas.)

I think: (1) is too naive; (2) is non-trivial (and not sure if it's possible); (3) works but is not cool; (4) is the best.

@alinush @vgao1996 @zekun000 @nanne007 thoughts?

@zekun000
Copy link
Contributor

zekun000 commented Dec 1, 2023

3 seems idiomatic way to me but I don't have much context

@nanne007
Copy link
Contributor Author

nanne007 commented Dec 2, 2023

Many thanks for the reply!
In my practice, most elements are just ephemeral temporary results, used once and then dropped. but they still incur the memory cost of vm session in the current implementation.
another problem is that elements cannot be stored in state due to they are just a pointer which will not be available after current session. Have to serialize them first and store them as vector.

As for the possible solutions that @zjma gave, I prefer the 4th.
most elements are just vector<u64>, and may fit into a or two u256 in move.

the 3rd is also fine to me, while the drawback is developers have to track every elements and drop it manually.
Many temporary results generated by field operations like add, sub, neg, mul must be handled. It's can be tedious.

In comparison, I would suggest that we go the 4th way.

@guangyuz
Copy link
Contributor

guangyuz commented Dec 2, 2023

Perhaps we can complete the current solution first and deploy it to the devnet, and then further optimize it after collecting more real data.

* [feat][aptos-stdlib] crypto algebra bn254

Signed-off-by: caojiafeng <funfriendcjf@gmail.com>

* scripts: fix update_algebra_gas_params.py

Signed-off-by: caojiafeng <funfriendcjf@gmail.com>

* add bn254 to default features to make unit test pass

* fix a bug mentioned in the comments

* initial

* update

* initial gas param

* benches for fq and fq2

* it compiles

* gas script now also specifies quantity types

* update scripts

---------

Signed-off-by: caojiafeng <funfriendcjf@gmail.com>
Co-authored-by: caojiafeng <funfriendcjf@gmail.com>

This comment has been minimized.

This comment has been minimized.

@@ -44,4 +46,4 @@
/// global operations.
/// - V1
/// - TBA
pub const LATEST_GAS_FEATURE_VERSION: u64 = 12;
pub const LATEST_GAS_FEATURE_VERSION: u64 = 13;
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, a gas version bump here.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite compat success on aptos-node-v1.7.3 ==> ce2f2043d1b9626e972187741bb76ecfb2e8b069

Compatibility test results for aptos-node-v1.7.3 ==> ce2f2043d1b9626e972187741bb76ecfb2e8b069 (PR)
1. Check liveness of validators at old version: aptos-node-v1.7.3
compatibility::simple-validator-upgrade::liveness-check : committed: 3302 txn/s, latency: 6485 ms, (p50: 6800 ms, p90: 9900 ms, p99: 10500 ms), latency samples: 184920
2. Upgrading first Validator to new version: ce2f2043d1b9626e972187741bb76ecfb2e8b069
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1855 txn/s, latency: 15526 ms, (p50: 18300 ms, p90: 21900 ms, p99: 22300 ms), latency samples: 92760
3. Upgrading rest of first batch to new version: ce2f2043d1b9626e972187741bb76ecfb2e8b069
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1013 txn/s, latency: 18226 ms, (p50: 18400 ms, p90: 23700 ms, p99: 24800 ms), latency samples: 75980
4. upgrading second batch to new version: ce2f2043d1b9626e972187741bb76ecfb2e8b069
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 2309 txn/s, latency: 9031 ms, (p50: 9600 ms, p90: 13500 ms, p99: 13800 ms), latency samples: 138560
5. check swarm health
Compatibility test for aptos-node-v1.7.3 ==> ce2f2043d1b9626e972187741bb76ecfb2e8b069 passed
Test Ok

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on ce2f2043d1b9626e972187741bb76ecfb2e8b069

two traffics test: inner traffic : committed: 8936 txn/s, latency: 4400 ms, (p50: 4200 ms, p90: 4800 ms, p99: 9000 ms), latency samples: 3851680
two traffics test : committed: 100 txn/s, latency: 2128 ms, (p50: 2100 ms, p90: 2300 ms, p99: 2900 ms), latency samples: 1800
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.203, avg: 0.197", "QsPosToProposal: max: 0.131, avg: 0.123", "ConsensusProposalToOrdered: max: 0.545, avg: 0.536", "ConsensusOrderedToCommit: max: 0.487, avg: 0.472", "ConsensusProposalToCommit: max: 1.028, avg: 1.008"]
Max round gap was 1 [limit 4] at version 1919636. Max no progress secs was 4.408409 [limit 10] at version 1919636.
Test Ok

This comment has been minimized.

This comment has been minimized.

@zjma zjma linked an issue Dec 12, 2023 that may be closed by this pull request
Copy link
Contributor

❌ Forge suite framework_upgrade failure on aptos-node-v1.7.3 ==> ce2f2043d1b9626e972187741bb76ecfb2e8b069

Compatibility test results for aptos-node-v1.7.3 ==> ce2f2043d1b9626e972187741bb76ecfb2e8b069 (PR)
Upgrade the nodes to version: ce2f2043d1b9626e972187741bb76ecfb2e8b069
Test Failed: API error: Unknown error error sending request for url (http://aptos-node-3-validator.forge-framework-upgrade-pr-11142.svc:8080/v1/estimate_gas_price): error trying to connect: dns error: failed to lookup address information: Name or service not known

Stack backtrace:
   0: <unknown>
   1: <unknown>
   2: <unknown>
   3: <unknown>
   4: <unknown>
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: __libc_start_main
  13: <unknown>
Trailing Log Lines:
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: __libc_start_main
  13: <unknown>


Swarm logs can be found here: See fgi output for more information.
{"level":"INFO","source":{"package":"aptos_forge","file":"testsuite/forge/src/backend/k8s/cluster_helper.rs:292"},"thread_name":"main","hostname":"forge-framework-upgrade-pr-11142-1702369187-aptos-node-v1-7-3","timestamp":"2023-12-12T08:45:07.690752Z","message":"Deleting namespace forge-framework-upgrade-pr-11142: Some(NamespaceStatus { conditions: Some([NamespaceCondition { last_transition_time: Some(Time(2023-12-12T08:45:07Z)), message: Some(\"All resources successfully discovered\"), reason: Some(\"ResourcesDiscovered\"), status: \"False\", type_: \"NamespaceDeletionDiscoveryFailure\" }, NamespaceCondition { last_transition_time: Some(Time(2023-12-12T08:45:07Z)), message: Some(\"All legacy kube types successfully parsed\"), reason: Some(\"ParsedGroupVersions\"), status: \"False\", type_: \"NamespaceDeletionGroupVersionParsingFailure\" }, NamespaceCondition { last_transition_time: Some(Time(2023-12-12T08:45:07Z)), message: Some(\"All content successfully deleted, may be waiting on finalization\"), reason: Some(\"ContentDeleted\"), status: \"False\", type_: \"NamespaceDeletionContentFailure\" }, NamespaceCondition { last_transition_time: Some(Time(2023-12-12T08:45:07Z)), message: Some(\"Some resources are remaining: persistentvolumeclaims. has 4 resource instances\"), reason: Some(\"SomeResourcesRemain\"), status: \"True\", type_: \"NamespaceContentRemaining\" }, NamespaceCondition { last_transition_time: Some(Time(2023-12-12T08:45:07Z)), message: Some(\"Some content in the namespace has finalizers remaining: kubernetes.io/pvc-protection in 4 resource instances\"), reason: Some(\"SomeFinalizersRemain\"), status: \"True\", type_: \"NamespaceFinalizersRemaining\" }]), phase: Some(\"Terminating\") })"}
{"level":"INFO","source":{"package":"aptos_forge","file":"testsuite/forge/src/backend/k8s/cluster_helper.rs:400"},"thread_name":"main","hostname":"forge-framework-upgrade-pr-11142-1702369187-aptos-node-v1-7-3","timestamp":"2023-12-12T08:45:07.690792Z","message":"aptos-node resources for Forge removed in namespace: forge-framework-upgrade-pr-11142"}
Failed to run tests:
Tests Failed

failures:
    framework_upgrade::framework-upgrade

test result: FAILED. 0 passed; 1 failed; 0 filtered out

Error: Tests Failed

Stack backtrace:
   0: <unknown>
   1: <unknown>
   2: <unknown>
   3: <unknown>
   4: <unknown>
   5: __libc_start_main
   6: <unknown>
Debugging output:

@zjma zjma merged commit 3686979 into aptos-labs:main Dec 12, 2023
54 of 55 checks passed
@nanne007 nanne007 deleted the bn256 branch December 13, 2023 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request][aptos-stdlib] bn254 algebra operations
5 participants