cache the VK when the VM is created #12975

alinush · 2024-04-22T20:58:36Z

Description

This optimization defers updating the on-chain Groth16 VK to the end of a (consensus) epoch using @zjma's ConfigBuffer. (Originally, tried a different approach in this PR).

This is done for two reasons:

It is the new correct way of updating on-chain configs, post randomness / DKGs.
This allows us to maintain a fresh view of the deserialized VK in Rust when creating a new VM, and speed up TXN validation.

This PR also contains a few minor changes:

Small refactoring
Adds a few tests and benchmarks

Authenticator size tests

Tested the byte sizes of a serialized keyless pubkey and signature:

ZKP-less TXN sizes
------------
KeylessSignature BCS size: 1033
KeylessPublicKey BCS size: 61

(Signature size would have been 294 if the extra_field was set to None.)

ZKP-based TXN sizes
------------
With Google as the OIDC provider:
 - KeylessPublicKey BCS size: 61

With extra_field set to "family_name":"Straka",
 - KeylessSignature BCS size: 318

Without extra_field:
 - KeylessSignature BCS size: 294

Keyless TXN verification times

Before this change, for each TXN verification we incurred:

VK deserialization time: 1.92ms
 + If we avoid point validation: 1.00ms
Public inputs hash time: 1.609917ms
Proof deserialization time: 418.5µs
Proof verification time: 1.069708ms
------------------------------------
Total time: 5.01ms

After this change:

VK deserialization time: 0
Public inputs hash time: 1.609917ms
Proof deserialization time: 418.5µs
Proof verification time: 1.069708ms
------------------------------------
Total time: 3.10ms

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

VM tests in aptos-move/e2e-move-tests
Smoke tests in testsuite/smoke

In both test suites, we submit keyless TXNs to test that:

Old proof verifies for old VK
New proof does not verify for old VK
Change the old VK to the new VK via a governance proposal
Old proof does not verify for new VK
New proof verifies for new VK

Key Areas to Review

The changes to Aptos VM; i.e., reading the on-chain VK and remembering it inside the VM
The changes to keyless_account.move; i.e., are properly deferring all on-chain config updates via config_buffer.move
The early-abort in AptosVM::validate_signed_transaction when no VK is cached in the VM (in aptos-move/aptos-vm/src/aptos_vm.rs)

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-04-22T20:58:39Z

⏱️ 26h 32m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-targeted-unit-tests	5h 43m	🟩 ⬜ ⬜ 🟩 🟩 (+15 more)
rust-move-unit-coverage	4h 37m	⬜ 🟩 ⬜ 🟩 🟩 (+14 more)
rust-move-tests	2h 58m	🟩 ⬜ 🟩 🟩 🟩 (+15 more)
windows-build	1h 58m	🟩 🟩 🟩
rust-lints	1h 46m	🟩 ⬜ 🟩 🟩 🟩 (+15 more)
run-tests-main-branch	1h 38m	🟩 ⬜ 🟩 🟩 🟩 (+18 more)
rust-smoke-tests	1h 23m	⬜ 🟩 🟩
rust-unit-tests	1h 22m	⬜ 🟥 ⬜ 🟩
execution-performance / single-node-performance	1h 15m	🟩 🟩 🟩
rust-images / rust-all	42m	⬜ 🟩 🟩
general-lints	35m	🟩 ⬜ 🟩 🟩 🟩 (+15 more)
forge-e2e-test / forge	34m	🟩 🟩
check-dynamic-deps	32m	🟩 🟩 🟩 🟩 🟩 (+15 more)
forge-compat-test / forge	24m	🟩 🟩
rust-build-cached-packages	14m	🟩 🟩 🟩
check	14m	🟩 🟩 🟩
cli-e2e-tests / run-cli-tests	12m	🟩 🟩
semgrep/ci	9m	🟩 🟩 🟩 🟩 🟩 (+14 more)
file_change_determinator	4m	🟩 🟩 🟩 🟩 🟩 (+17 more)
file_change_determinator	4m	🟩 🟩 🟩 🟩 🟩 (+14 more)
node-api-compatibility-tests / node-api-compatibility-tests	2m	🟩 🟩
permission-check	1m	🟩 🟩 🟩 🟩 🟩 (+17 more)
permission-check	1m	🟩 🟩 🟩 🟩 🟩 (+16 more)
permission-check	1m	🟩 🟩 🟩 🟩 🟩 (+17 more)
permission-check	1m	🟩 🟩 🟩 🟩 🟩 (+17 more)
file_change_determinator	32s	🟩 🟩 🟩
execution-performance / file_change_determinator	25s	🟩 🟩 🟩
permission-check	9s	🟩 🟩 🟩
determine-docker-build-metadata	8s	🟩 🟩 🟩

🚨 4 jobs on the last run were significantly faster/slower than expected

Job	Duration	vs 7d avg
rust-targeted-unit-tests	22m	16m
rust-move-tests	12m	10m
forge-e2e-test / forge	18m	15m
windows-build	33m	27m

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

codecov · 2024-04-22T21:20:04Z

Codecov Report

Attention: Patch coverage is 0% with 209 lines in your changes are missing coverage. Please review.

Project coverage is 62.2%. Comparing base (3b3edf1) to head (69c706c).
Report is 2 commits behind head on main.

Files	Patch %	Lines
types/src/keyless/circuit_testcases.rs	0.0%	80 Missing ⚠️
aptos-move/aptos-vm/src/keyless_validation.rs	0.0%	52 Missing ⚠️
types/src/keyless/test_utils.rs	0.0%	24 Missing ⚠️
aptos-move/aptos-vm/src/aptos_vm.rs	0.0%	18 Missing ⚠️
types/src/keyless/groth16_sig.rs	0.0%	16 Missing ⚠️
types/src/keyless/groth16_vk.rs	0.0%	15 Missing ⚠️
types/src/keyless/bn254_circom.rs	0.0%	2 Missing ⚠️
types/src/jwks/rsa/mod.rs	0.0%	1 Missing ⚠️
types/src/keyless/openid_sig.rs	0.0%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #12975     +/-   ##
=========================================
- Coverage    62.3%    62.2%   -0.1%     
=========================================
  Files         828      828             
  Lines      185844   185996    +152     
=========================================
  Hits       115858   115858             
- Misses      69986    70138    +152

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

igor-aptos

you have a lot of commented out printlns throughout the code, is that intentional?

aptos-move/aptos-vm/src/aptos_vm.rs

aptos-move/e2e-move-tests/src/tests/keyless_feature_gating.rs

testsuite/smoke-test/src/keyless.rs

igor-aptos

I completely missed the keyless_account.move change. looks good now

zjma · 2024-04-24T19:34:26Z

aptos-move/framework/aptos-framework/sources/keyless_account.move

@@ -31,7 +44,7 @@ module aptos_framework::keyless_account {
    }

    #[resource_group_member(group = aptos_framework::keyless_account::Group)]
-    struct Configuration has key, store {
+    struct Configuration has key, store, drop, copy {


i think an initialization func for this resource is missing

i guess update_configuration() played the role before but not any more.

actually not an issue because on_new_epoch plays this role now.

hmm but you will still need an initialize() for genesis...

Indeed. The genesis initialization will merely queue up the changes... A reconfiguration will be needed to apply the buffer.

zjma · 2024-04-24T19:56:10Z

aptos-move/framework/aptos-framework/sources/keyless_account.move

+    /// Pre-validate the VK to actively-prevent incorrect VKs from being set on-chain.
+    fun validate_groth16_vk(vk: &Groth16VerificationKey) {
+        // Could be leveraged to speed up the VM deserialization of the VK by 2x, since it can assume the points are valid.
+        assert!(option::is_some(&crypto_algebra::deserialize<bn254_algebra::G1, bn254_algebra::FormatG1Compr>(&vk.alpha_g1)), E_INVALID_BN254_G1_SERIALIZATION);


if bn254Structures is disabled, we can't update vk?

Yep. Is that a problem? I figured it would be wise to check validity here & preempt bad VKs from being set.

Cargo.toml

sitalkedia

minor nits.

aptos-move/aptos-vm/src/aptos_vm.rs

aptos-move/aptos-vm/src/keyless_validation.rs

sitalkedia · 2024-04-24T21:23:03Z

testsuite/smoke-test/src/keyless.rs

+    let result = info
+        .client()
+        .submit_without_serializing_response(&signed_txn)
+        .await;


nit: you can just unwrap here.

zekun000

there's too many debug code in the pr

zekun000 · 2024-04-24T22:41:03Z

aptos-move/framework/aptos-framework/sources/keyless_account.move

        config.max_exp_horizon_secs = max_exp_horizon_secs;
+
+        update_configuration(fx, config);


this is not used in the cache right?

alinush · 2024-04-24T22:52:25Z

One problem left in this PR: setting the VK and Configuration in genesis for devnet will no longer work instantly. The updates will only get applied after an epoch change... Might want to fix that to avoid surprises when we redeploy devnet.

igor-aptos · 2024-04-24T22:56:13Z

first two epochs are only 1 transaction long, so that should be fine? can we confirm that from the forge run, are the grafana dashboards that you can see when VK kicks in?

alinush · 2024-04-24T23:24:49Z

types/src/keyless/circuit_testcases.rs

@@ -63,8 +66,8 @@ pub(crate) static SAMPLE_JWT_PAYLOAD_JSON: Lazy<String> = Lazy::new(|| {
            {}
            "locale":"en",
            "iat":1700255944,
-            "exp":2700259544,
-            "nonce":"{}"
+            "nonce":"{}",


cc @heliuchuan, not sure how you succeeded in generating a proof in the past for this since there was no comma after nonce... nor a }

alinush · 2024-04-25T17:06:09Z

One problem left in this PR: setting the VK and Configuration in genesis for devnet will no longer work instantly. The updates will only get applied after an epoch change... Might want to fix that to avoid surprises when we redeploy devnet.

Fixed this by directly writing things in genesis.

this optimization defers the update of the VK to the end of an epoch, which allows us to maintain a fresh view of the deserialized VK in Rust when creating a new VM

github-actions · 2024-04-25T19:40:36Z

✅ Forge suite `compat` success on `aptos-node-v1.10.1` ==> `69c706c88808ab17bcecd2276be4f8ee16b6417a`

Compatibility test results for aptos-node-v1.10.1 ==> 69c706c88808ab17bcecd2276be4f8ee16b6417a (PR)
1. Check liveness of validators at old version: aptos-node-v1.10.1
compatibility::simple-validator-upgrade::liveness-check : committed: 6167 txn/s, latency: 5320 ms, (p50: 4800 ms, p90: 9900 ms, p99: 12100 ms), latency samples: 228180
2. Upgrading first Validator to new version: 69c706c88808ab17bcecd2276be4f8ee16b6417a
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1338 txn/s, latency: 18220 ms, (p50: 19400 ms, p90: 28600 ms, p99: 28900 ms), latency samples: 85640
3. Upgrading rest of first batch to new version: 69c706c88808ab17bcecd2276be4f8ee16b6417a
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1575 txn/s, latency: 18178 ms, (p50: 19000 ms, p90: 28000 ms, p99: 28300 ms), latency samples: 91380
4. upgrading second batch to new version: 69c706c88808ab17bcecd2276be4f8ee16b6417a
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3394 txn/s, latency: 9313 ms, (p50: 9700 ms, p90: 12600 ms, p99: 12900 ms), latency samples: 142560
5. check swarm health
Compatibility test for aptos-node-v1.10.1 ==> 69c706c88808ab17bcecd2276be4f8ee16b6417a passed
Test Ok

github-actions · 2024-04-25T19:46:58Z

✅ Forge suite `realistic_env_max_load` success on `69c706c88808ab17bcecd2276be4f8ee16b6417a`

two traffics test: inner traffic : committed: 8117 txn/s, latency: 4830 ms, (p50: 4600 ms, p90: 5700 ms, p99: 10200 ms), latency samples: 3506920
two traffics test : committed: 100 txn/s, latency: 1997 ms, (p50: 1900 ms, p90: 2200 ms, p99: 6300 ms), latency samples: 1840
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.209, avg: 0.205", "QsPosToProposal: max: 0.217, avg: 0.204", "ConsensusProposalToOrdered: max: 0.425, avg: 0.400", "ConsensusOrderedToCommit: max: 0.410, avg: 0.384", "ConsensusProposalToCommit: max: 0.812, avg: 0.784"]
Max round gap was 1 [limit 4] at version 1717. Max no progress secs was 4.8698587 [limit 15] at version 2049597.
Test Ok

alinush force-pushed the alin/keyless-vk-cache branch 7 times, most recently from 1587f53 to 96f2bf2 Compare April 24, 2024 16:28

alinush marked this pull request as ready for review April 24, 2024 16:30

alinush requested review from heliuchuan, zjma, mstraka100, rex1fernando, davidiw, movekevin, wrwg, zekun000, vgao1996 and georgemitenkov as code owners April 24, 2024 16:30

alinush force-pushed the alin/keyless-vk-cache branch from 96f2bf2 to 6a2557e Compare April 24, 2024 16:34

igor-aptos reviewed Apr 24, 2024

View reviewed changes

aptos-move/aptos-vm/src/aptos_vm.rs Show resolved Hide resolved

alinush force-pushed the alin/keyless-vk-cache branch from 6a2557e to e71b0c5 Compare April 24, 2024 17:49

alinush commented Apr 24, 2024

View reviewed changes

aptos-move/e2e-move-tests/src/tests/keyless_feature_gating.rs Show resolved Hide resolved

alinush commented Apr 24, 2024

View reviewed changes

testsuite/smoke-test/src/keyless.rs Show resolved Hide resolved

igor-aptos approved these changes Apr 24, 2024

View reviewed changes

alinush force-pushed the alin/keyless-vk-cache branch from e71b0c5 to 109a583 Compare April 24, 2024 18:06

zjma reviewed Apr 24, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

sitalkedia approved these changes Apr 24, 2024

View reviewed changes

zekun000 reviewed Apr 24, 2024

View reviewed changes

alinush force-pushed the alin/keyless-vk-cache branch 2 times, most recently from 0e79cf1 to 1c88448 Compare April 24, 2024 23:20

alinush commented Apr 24, 2024

View reviewed changes

alinush force-pushed the alin/keyless-vk-cache branch from 1c88448 to e59b2f5 Compare April 24, 2024 23:26

alinush requested a review from junkil-park as a code owner April 25, 2024 00:56

alinush force-pushed the alin/keyless-vk-cache branch 2 times, most recently from cd98e22 to 0fca756 Compare April 25, 2024 17:05

alinush enabled auto-merge (squash) April 25, 2024 17:05

alinush force-pushed the alin/keyless-vk-cache branch from 0fca756 to 05978aa Compare April 25, 2024 17:21

This comment has been minimized.

Sign in to view

alinush added 3 commits April 25, 2024 12:14

cache the VK when the VM is created

f8f0081

this optimization defers the update of the VK to the end of an epoch, which allows us to maintain a fresh view of the deserialized VK in Rust when creating a new VM

add tests for VK change

f2ed5f6

genesis now writes the keyless resources directly

69c706c

alinush force-pushed the alin/keyless-vk-cache branch from 05978aa to 69c706c Compare April 25, 2024 19:14

This comment has been minimized.

Sign in to view

alinush merged commit 750f9ae into main Apr 25, 2024
53 checks passed

alinush deleted the alin/keyless-vk-cache branch April 25, 2024 19:48

alinush mentioned this pull request Apr 26, 2024

fast Poseidon-BN254 hashing via neptune and caching #13050

Merged

21 tasks

alinush mentioned this pull request May 23, 2024

Lower gas for keyless TXNs by 3x in transaction.rs #13397

Merged

21 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cache the VK when the VM is created #12975

cache the VK when the VM is created #12975

alinush commented Apr 22, 2024 •

edited

Loading

trunk-io bot commented Apr 22, 2024 •

edited

Loading

codecov bot commented Apr 22, 2024 •

edited

Loading

igor-aptos left a comment

igor-aptos left a comment

zjma Apr 24, 2024

zjma Apr 24, 2024

zjma Apr 24, 2024

zjma Apr 24, 2024

alinush Apr 24, 2024 •

edited

Loading

zjma Apr 24, 2024 •

edited

Loading

alinush Apr 24, 2024

sitalkedia left a comment

sitalkedia Apr 24, 2024

zekun000 left a comment

zekun000 Apr 24, 2024

alinush commented Apr 24, 2024

igor-aptos commented Apr 24, 2024

alinush Apr 24, 2024 •

edited

Loading

alinush commented Apr 25, 2024

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.

github-actions bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

		config.max_exp_horizon_secs = max_exp_horizon_secs;

		update_configuration(fx, config);

cache the VK when the VM is created #12975

cache the VK when the VM is created #12975

Conversation

alinush commented Apr 22, 2024 • edited Loading

Description

Authenticator size tests

Keyless TXN verification times

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

trunk-io bot commented Apr 22, 2024 • edited Loading

codecov bot commented Apr 22, 2024 • edited Loading

Codecov Report

igor-aptos left a comment

Choose a reason for hiding this comment

igor-aptos left a comment

Choose a reason for hiding this comment

zjma Apr 24, 2024

Choose a reason for hiding this comment

zjma Apr 24, 2024

Choose a reason for hiding this comment

zjma Apr 24, 2024

Choose a reason for hiding this comment

zjma Apr 24, 2024

Choose a reason for hiding this comment

alinush Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

zjma Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

alinush Apr 24, 2024

Choose a reason for hiding this comment

sitalkedia left a comment

Choose a reason for hiding this comment

sitalkedia Apr 24, 2024

Choose a reason for hiding this comment

zekun000 left a comment

Choose a reason for hiding this comment

zekun000 Apr 24, 2024

Choose a reason for hiding this comment

alinush commented Apr 24, 2024

igor-aptos commented Apr 24, 2024

alinush Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

alinush commented Apr 25, 2024

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.

github-actions bot commented Apr 25, 2024

✅ Forge suite compat success on aptos-node-v1.10.1 ==> 69c706c88808ab17bcecd2276be4f8ee16b6417a

github-actions bot commented Apr 25, 2024

✅ Forge suite realistic_env_max_load success on 69c706c88808ab17bcecd2276be4f8ee16b6417a

alinush commented Apr 22, 2024 •

edited

Loading

trunk-io bot commented Apr 22, 2024 •

edited

Loading

codecov bot commented Apr 22, 2024 •

edited

Loading

alinush Apr 24, 2024 •

edited

Loading

zjma Apr 24, 2024 •

edited

Loading

alinush Apr 24, 2024 •

edited

Loading

✅ Forge suite `compat` success on `aptos-node-v1.10.1` ==> `69c706c88808ab17bcecd2276be4f8ee16b6417a`

✅ Forge suite `realistic_env_max_load` success on `69c706c88808ab17bcecd2276be4f8ee16b6417a`