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

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

Merged
merged 1 commit into from
May 23, 2024

Conversation

alinush
Copy link
Contributor

@alinush alinush commented May 23, 2024

Description

There have been 2 performance improvements to keyless TXN verification times:

  1. Caching the deserialized VK in the VM (cache the VK when the VM is created #12975), which lowered the time from 5ms to 3.10 ms
  2. Faster Poseidon hashing (fast Poseidon-BN254 hashing via neptune and caching #13050), which lowered the gas from 3.10ms to 1.66ms

Overall, this is a ~3x reduction in gas costs and should be reflected in how we charge for keyless TXNs (previous PR was #13005).

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Other (specify)

How Has This Been Tested?

It has not. @vgao1996 do we need to bump up gas versions?

Key Areas to Review

  1. Do we need to bump up gas version?
  2. Do we need feature gating?

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

Copy link

trunk-io bot commented May 23, 2024

⏱️ 1h 39m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-targeted-unit-tests 43m 🟩🟩
rust-move-tests 28m 🟩🟩
rust-lints 11m 🟩🟩
run-tests-main-branch 8m 🟩🟩
general-lints 3m 🟩🟩
check-dynamic-deps 3m 🟩🟩
semgrep/ci 41s 🟩🟩
file_change_determinator 28s 🟩🟩
file_change_determinator 22s 🟩🟩
permission-check 10s 🟩🟩
permission-check 6s 🟩🟩
permission-check 5s 🟩🟩
permission-check 4s 🟩🟩

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

Job Duration vs 7d avg Delta
rust-move-tests 16m 8m +92%

settingsfeedbackdocs ⋅ learn more about trunk.io

@vgao1996
Copy link
Contributor

vgao1996 commented May 23, 2024

No need to bump the gas feature version if you're just changing the value

@alinush
Copy link
Contributor Author

alinush commented May 23, 2024

No need to bump the gas feature version if you're just changing the value

Cool, thank you @vgao1996! Can you stamp please? 🙏

@alinush alinush marked this pull request as ready for review May 23, 2024 16:31
@alinush alinush force-pushed the alin/lower-keyless-txn-gas branch from b3c407d to 3a2cd10 Compare May 23, 2024 16:31
@sherry-x sherry-x enabled auto-merge (squash) May 23, 2024 18:46
@alinush alinush force-pushed the alin/lower-keyless-txn-gas branch from 3a2cd10 to 9f27033 Compare May 23, 2024 18:54

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 9f270334a324f983321905a296015ca2fe40a672

two traffics test: inner traffic : committed: 8084.3907382798325 txn/s, latency: 4847.721626135298 ms, (p50: 4500 ms, p90: 6400 ms, p99: 11700 ms), latency samples: 3496880
two traffics test : committed: 99.88269897970748 txn/s, latency: 1834.1977777777777 ms, (p50: 1800 ms, p90: 2100 ms, p99: 2400 ms), latency samples: 1800
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.218, avg: 0.208", "QsPosToProposal: max: 0.308, avg: 0.241", "ConsensusProposalToOrdered: max: 0.445, avg: 0.431", "ConsensusOrderedToCommit: max: 0.366, avg: 0.358", "ConsensusProposalToCommit: max: 0.799, avg: 0.789"]
Max round gap was 1 [limit 4] at version 1758250. Max no progress secs was 4.8007917 [limit 15] at version 1758250.
Test Ok

Copy link
Contributor

✅ Forge suite compat success on 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 9f270334a324f983321905a296015ca2fe40a672

Compatibility test results for 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 9f270334a324f983321905a296015ca2fe40a672 (PR)
1. Check liveness of validators at old version: 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411
compatibility::simple-validator-upgrade::liveness-check : committed: 6506.384315146194 txn/s, latency: 4863.337363116965 ms, (p50: 4800 ms, p90: 5300 ms, p99: 8400 ms), latency samples: 252040
2. Upgrading first Validator to new version: 9f270334a324f983321905a296015ca2fe40a672
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 2819.0590375461425 txn/s, latency: 10922.99816552901 ms, (p50: 11800 ms, p90: 14100 ms, p99: 14200 ms), latency samples: 117200
3. Upgrading rest of first batch to new version: 9f270334a324f983321905a296015ca2fe40a672
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 3406.8622068319914 txn/s, latency: 9133.163097652854 ms, (p50: 9400 ms, p90: 13900 ms, p99: 14300 ms), latency samples: 138040
4. upgrading second batch to new version: 9f270334a324f983321905a296015ca2fe40a672
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 4418.541575882359 txn/s, latency: 7323.802815343005 ms, (p50: 6400 ms, p90: 14100 ms, p99: 17400 ms), latency samples: 162680
5. check swarm health
Compatibility test for 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 9f270334a324f983321905a296015ca2fe40a672 passed
Test Ok

Copy link
Contributor

✅ Forge suite framework_upgrade success on 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 9f270334a324f983321905a296015ca2fe40a672

Compatibility test results for 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 9f270334a324f983321905a296015ca2fe40a672 (PR)
Upgrade the nodes to version: 9f270334a324f983321905a296015ca2fe40a672
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1309.6355754468786 txn/s, submitted: 1312.1536211601008 txn/s, failed submission: 2.518045713222224 txn/s, expired: 2.518045713222224 txn/s, latency: 2527.2418957892714 ms, (p50: 2000 ms, p90: 4800 ms, p99: 7800 ms), latency samples: 104020
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1041.6678372358224 txn/s, submitted: 1044.1044871474853 txn/s, failed submission: 2.4366499116627427 txn/s, expired: 2.4366499116627427 txn/s, latency: 3049.1675789473684 ms, (p50: 2300 ms, p90: 6000 ms, p99: 9600 ms), latency samples: 85500
5. check swarm health
Compatibility test for 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 9f270334a324f983321905a296015ca2fe40a672 passed
Upgrade the remaining nodes to version: 9f270334a324f983321905a296015ca2fe40a672
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1018.839334877293 txn/s, submitted: 1020.6930339173244 txn/s, failed submission: 1.8536990400314632 txn/s, expired: 1.8536990400314632 txn/s, latency: 3060.402672276552 ms, (p50: 2100 ms, p90: 6300 ms, p99: 11100 ms), latency samples: 87940
Test Ok

@sherry-x sherry-x merged commit dd9b79e into main May 23, 2024
50 of 51 checks passed
@sherry-x sherry-x deleted the alin/lower-keyless-txn-gas branch May 23, 2024 19:37
sherry-x pushed a commit that referenced this pull request May 23, 2024
… (#13405)

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

* add default gas
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.

None yet

4 participants