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

Fix iss-related bug in Groth16 path & refactor #12017

Merged
merged 8 commits into from
Feb 19, 2024
Merged

Conversation

alinush
Copy link
Contributor

@alinush alinush commented Feb 14, 2024

Description

  • zkID TXNs now store only the val of the iss, not the full field.
  • zkID TXNs no longer unnecessarily include a dummy field for the extra field match
  • Test cases are now isolated in a single file (to be updated automatically in the future)
  • Unit tests and smoke tests are more generic
  • Refactored types/src/zkid.rs into multiple files in types/src/zkid/

Copy link

trunk-io bot commented Feb 14, 2024

⏱️ 45h 46m total CI duration on this PR
Job Cumulative Duration Recent Runs
windows-build 10h 36m 🟩🟩🟩🟩🟩 (+27 more)
rust-unit-tests 10h 12m 🟩 (+23 more)
rust-move-unit-coverage 7h 16m 🟩 (+20 more)
rust-move-tests 5h 🟩🟩🟩🟩 (+20 more)
run-tests-main-branch 2h 57m 🟥🟥🟥🟥 (+26 more)
rust-lints 2h 22m 🟩🟥🟩🟩 (+23 more)
check 1h 54m 🟩🟩🟩🟩 (+27 more)
check-dynamic-deps 1h 5m 🟩🟩🟩🟩🟩 (+27 more)
general-lints 1h 2m 🟩🟩🟩🟩🟩 (+23 more)
rust-smoke-tests 53m 🟩
execution-performance / single-node-performance 42m 🟩🟩
rust-images / rust-all 32m 🟩🟥🟩
forge-e2e-test / forge 17m 🟩
forge-compat-test / forge 15m 🟩
semgrep/ci 13m 🟩🟩🟩🟩🟩 (+27 more)
cli-e2e-tests / run-cli-tests 12m 🟥
file_change_determinator 5m 🟩🟩🟩🟩🟩 (+26 more)
file_change_determinator 5m 🟩🟩🟩🟩🟩 (+25 more)
node-api-compatibility-tests / node-api-compatibility-tests 2m 🟩🟩
permission-check 2m 🟩🟩🟩🟩🟩 (+27 more)
permission-check 2m 🟩🟩🟩🟩🟩 (+27 more)
permission-check 2m 🟩🟩🟩🟩 (+27 more)
permission-check 2m 🟩🟩🟩🟩 (+27 more)
execution-performance / file_change_determinator 24s 🟩🟩
file_change_determinator 22s 🟩🟩
determine-docker-build-metadata 12s 🟩🟩
permission-check 5s 🟩🟩

🚨 1 job on the last run was significantly faster/slower than expected

Job Duration vs 7d avg Delta
cli-e2e-tests / run-cli-tests 9m 7m +34%

settingsfeedbackdocs ⋅ learn more about trunk.io

@alinush alinush force-pushed the alin/authenticator-fixes branch 4 times, most recently from bc49d15 to b1e8caf Compare February 15, 2024 17:30
Copy link

codecov bot commented Feb 15, 2024

Codecov Report

Attention: 554 lines in your changes are missing coverage. Please review.

Comparison is base (8575537) 69.9% compared to head (5ba290a) 71.4%.

Files Patch % Lines
types/src/zkid/circuit_testcases.rs 0.0% 112 Missing ⚠️
types/src/zkid/openid_sig.rs 0.0% 101 Missing ⚠️
types/src/zkid/mod.rs 11.2% 95 Missing ⚠️
types/src/zkid/test_utils.rs 0.0% 75 Missing ⚠️
types/src/zkid/bn254_circom.rs 45.5% 74 Missing ⚠️
types/src/zkid/groth16_sig.rs 0.0% 38 Missing ⚠️
types/src/zkid/groth16_vk.rs 54.5% 30 Missing ⚠️
types/src/zkid/configuration.rs 55.8% 19 Missing ⚠️
aptos-move/aptos-vm/src/zkid_validation.rs 0.0% 8 Missing ⚠️
types/src/transaction/authenticator.rs 0.0% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main   #12017       +/-   ##
===========================================
+ Coverage    69.9%    71.4%     +1.4%     
===========================================
  Files        2222      809     -1413     
  Lines      419683   184616   -235067     
===========================================
- Hits       293473   131838   -161635     
+ Misses     126210    52778    -73432     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alinush alinush force-pushed the alin/authenticator-fixes branch 9 times, most recently from 931f3f4 to beb8ccd Compare February 18, 2024 02:51
@alinush alinush changed the title Fix iss-related bug in Groth16 path Fix iss-related bug in Groth16 path & refactor Feb 18, 2024
@alinush alinush force-pushed the alin/authenticator-fixes branch from beb8ccd to 98ccbd4 Compare February 18, 2024 03:08
@alinush alinush marked this pull request as ready for review February 18, 2024 03:09
@alinush alinush force-pushed the alin/authenticator-fixes branch from e4967d9 to f6e04f1 Compare February 19, 2024 00:08
const STRUCT_NAME: &'static IdentStr = ident_str!("Groth16VerificationKey");
}

impl TryFrom<Groth16VerificationKey> for PreparedVerifyingKey<Bn254> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: worth having some UTs for all the impl TryFrom across the PR.
If they are already covered by some smoke tests, then it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Worth having a lot more tests! Next PR!

@alinush alinush enabled auto-merge (squash) February 19, 2024 01:10
@heliuchuan heliuchuan disabled auto-merge February 19, 2024 01:12

This comment has been minimized.

This comment has been minimized.

@alinush alinush enabled auto-merge (squash) February 19, 2024 01:54

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite compat success on aptos-node-v1.9.5 ==> 5ba290aaef75f9e2232e96aa9fb4067911b3fc56

Compatibility test results for aptos-node-v1.9.5 ==> 5ba290aaef75f9e2232e96aa9fb4067911b3fc56 (PR)
1. Check liveness of validators at old version: aptos-node-v1.9.5
compatibility::simple-validator-upgrade::liveness-check : committed: 6996 txn/s, latency: 4730 ms, (p50: 4800 ms, p90: 7200 ms, p99: 7800 ms), latency samples: 244860
2. Upgrading first Validator to new version: 5ba290aaef75f9e2232e96aa9fb4067911b3fc56
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1236 txn/s, latency: 23478 ms, (p50: 24600 ms, p90: 31600 ms, p99: 33400 ms), latency samples: 64320
3. Upgrading rest of first batch to new version: 5ba290aaef75f9e2232e96aa9fb4067911b3fc56
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 205 txn/s, submitted: 484 txn/s, expired: 279 txn/s, latency: 31320 ms, (p50: 29300 ms, p90: 46200 ms, p99: 73000 ms), latency samples: 20720
4. upgrading second batch to new version: 5ba290aaef75f9e2232e96aa9fb4067911b3fc56
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 2603 txn/s, latency: 11304 ms, (p50: 11400 ms, p90: 17600 ms, p99: 18000 ms), latency samples: 119780
5. check swarm health
Compatibility test for aptos-node-v1.9.5 ==> 5ba290aaef75f9e2232e96aa9fb4067911b3fc56 passed
Test Ok

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 5ba290aaef75f9e2232e96aa9fb4067911b3fc56

two traffics test: inner traffic : committed: 6964 txn/s, latency: 5468 ms, (p50: 5100 ms, p90: 7100 ms, p99: 12000 ms), latency samples: 3015520
two traffics test : committed: 100 txn/s, latency: 2270 ms, (p50: 2300 ms, p90: 2500 ms, p99: 3600 ms), latency samples: 1800
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.251, avg: 0.207", "QsPosToProposal: max: 0.184, avg: 0.171", "ConsensusProposalToOrdered: max: 0.636, avg: 0.584", "ConsensusOrderedToCommit: max: 0.558, avg: 0.516", "ConsensusProposalToCommit: max: 1.163, avg: 1.099"]
Max round gap was 1 [limit 4] at version 1234592. Max no progress secs was 5.261234 [limit 15] at version 1234592.
Test Ok

@alinush alinush merged commit 0d29ccd into main Feb 19, 2024
82 of 88 checks passed
@alinush alinush deleted the alin/authenticator-fixes branch February 19, 2024 03:06
zjma added a commit that referenced this pull request Feb 22, 2024
* Fix `iss`-related bug in Groth16 path & refactor (#12017)

Co-authored-by: Oliver <[email protected]>

* [aptosvm] Simplify VM flows (#11888)

* Duplicated logic for creating the gas meter for view functions has been removed.
* Duplicated logic for calculating gas used for view functions has been removed.
* There was unreachable code in failure transaction cleanup, where the discarded
status has been returned immediately, but then re-checked again. The first check
is shifted inside.
* No more default transaction metadata.
* Scripts are now validated consistently.
* Simplifies transaction execution function signature to avoid `Option<String>`.
* Removes duplicated features from `AptosVM` and keeps them in `MoveVMExt`.
* Fixes a bug when script hash was not computed for `RunOnAbort`.

Related tests are moved  to `move-e2e-tests`.

* [Compiler V2] Critical edge elimination (#11894)

Implement a pass to eliminate critical edges by splitting them with empty blocks

* [consensus configs] reduce sending block size from 2500 to 1900 (#12091)

### Description

The block output limit is no longer hit with p2p txns.

### Test Plan

Forge `realistic_env_max_load` TPS improves.

* [Indexer-grpc] Add profiling support. (#12034)

* Minor aggregator cleanup (#12013)

* Minor aggregator cleanup

* Addressing PR comments

* [move] rotate_authentication_key_call should not modify OriginatingAddress (#12108)

Co-authored-by: Alin Tomescu <[email protected]>

* [Data Streaming Service] Add dynamic prefetching support

* [Data Streaming Service] Add dynamic prefetching unit tests.

* [Data Streaming Service] Update existing integration tests.

* [State Sync] Add backpressure to fast sync receiver.

* Update perf baseline for gas charging coverage improvements (reducing throughput) (#12124)

* Reduce latency of cloning network sender using Arc pointers (#12103)

* Avoid cloning network sender using Arc pointers

* Removing a clone

* 100 node sweep test

* Removing a few clone operations

* reset forge test

* Removing some clones

* Removing clones

* adopt AIP-61 terminology for consistency (#12123)

adopt AIP-61 terminology for consistency

* [Consensus] Remove non-decoupled execution and refactor for cleaner interfaces (#12104)

* fix jwk key logging (#12090)

* remove spurious error lines (#12137)

* randomness #1: types update from randomnet (#12106)

* types update from randomnet

* update

* lint

* lint

* All validators broadcast commit vote messages (#12059)

* All validators broadcast commit messages

* Forge testing

* Increase timeout for forge

* test forge realistic_env_workload_sweep_test

* run realistic_env_workload_sweep_test

* run realistic_env_workload_sweep_test

* run sweep test

* increase forge runner duration

* forge testing

* Letting the proposer also broadcast commit decision for backward compatibility

* removing forge changes

* Added a TODO

* [vm] Resource access control: runtime engine (#10544)

* [vm] Resource access control: runtime engine

Implements the runtime engine for resource access control:

- a representation of access control specifiers in `loaded_data::runtime_access_specifiers`.
- a loader for access specifiers in `runtime::loader::access_specifier_loader`.
- a new stateful object representing the access control logic in `runtime::access_control`.
- finally the use of the `AccessControlState` in `runtime::interpreter`.

* Addressing reviewer comments.

* Addressing reviewer comments.

* typo: PTLA -> PTAL

* Rebasing: adjusting to upstream changes

* Rebasing

* ObjectCodeDeployment API cleanup update (#12133)

* ObjectCodeDeployment API cleanup update (#12141)

* [Compiler-v2] porting more V1 unit tests to V2 (#12085)

* update tests

* fix bug

* fix-12116

* fix missing space

* add expected got

* remove live-var tests

* fix had_erros

* fix

* Enable the max object nesting check (#12129)

* Resolved the warning for unused variable (#12157)

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* Squashed commit of the following:

commit a50ffec
Author: Zhoujun Ma <[email protected]>
Date:   Thu Feb 22 21:10:12 2024 +0000

    lint

commit 388350f
Author: zhoujun.ma <[email protected]>
Date:   Thu Feb 22 13:04:28 2024 -0800

    update

commit 76f7eca
Author: zhoujun.ma <[email protected]>
Date:   Thu Feb 22 12:56:04 2024 -0800

    update

commit a663542
Author: zhoujun.ma <[email protected]>
Date:   Thu Feb 22 12:54:18 2024 -0800

    update

commit b439449
Author: zhoujun.ma <[email protected]>
Date:   Thu Feb 22 12:34:14 2024 -0800

    update

commit 3378ceb
Author: zhoujun.ma <[email protected]>
Date:   Thu Feb 22 12:17:06 2024 -0800

    update

commit 6cd6685
Author: zhoujun.ma <[email protected]>
Date:   Thu Feb 22 12:15:05 2024 -0800

    update

commit 6d89f37
Author: zhoujun.ma <[email protected]>
Date:   Thu Feb 22 12:13:51 2024 -0800

    update

commit 980f257
Author: zhoujun.ma <[email protected]>
Date:   Thu Feb 22 12:12:04 2024 -0800

    update

commit 16e9349
Author: Zhoujun Ma <[email protected]>
Date:   Thu Feb 22 18:25:08 2024 +0000

    lint

---------

Co-authored-by: Alin Tomescu <[email protected]>
Co-authored-by: Oliver <[email protected]>
Co-authored-by: George Mitenkov <[email protected]>
Co-authored-by: Zekun Wang <[email protected]>
Co-authored-by: Brian (Sunghoon) Cho <[email protected]>
Co-authored-by: Guoteng Rao <[email protected]>
Co-authored-by: Satya Vusirikala <[email protected]>
Co-authored-by: David Wolinsky <[email protected]>
Co-authored-by: Josh Lind <[email protected]>
Co-authored-by: igor-aptos <[email protected]>
Co-authored-by: Sital Kedia <[email protected]>
Co-authored-by: Wolfgang Grieskamp <[email protected]>
Co-authored-by: Teng Zhang <[email protected]>
Co-authored-by: Junkil Park <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants