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

[gas] generate delta when upgrading the gas schedule #13391

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

vgao1996
Copy link
Contributor

@vgao1996 vgao1996 commented May 22, 2024

Description

This introduces some major improvements to the gas schedule upgrade process

  • New gas_schedule::set_for_next_epoch_check_hash interface to help establish a known base for upgrades, preventing unintended changes.
  • New release builder command generate-gas-schedule to create a snapshot of the gas schedule based on the values defined in Rust, stored to a json file.
  • Consolidated the various gas related release entries (DefaultGas, DefaultGasWithOverride etc.) into a single one named Gas for clarity.
    • This has two fields: old and new'. Valid values are , orcurrent` (using the Rust-defined values).
    • new is required but old is optional. It however not recommended you skip old for a real release. It's kept mainly for testing and easier migration.
      • If the release builder is connected to a remote endpoint, the on-chain gas schedule will be used if old is not specified.
    • Overrides are no longer supported. If you want to override entries, generate a gas schedule snapshot and modify the file instead.

Here's how the diff looks like

// Old Gas Schedule Hash: fbfbe099538bff3f5171254eaf9cb31fa3d9ac8d4f38959ef28a1ed481b12e6c
// Changes
//   Feature version: 16 -> 18
//   Parameters
//      +  aptos_framework.dispatchable_fungible_asset.dispatch.base                           :  551
//      +  aptos_framework.function_info.check_dispatch_type_compatibility_impl.base           :  1002
//      +  aptos_framework.function_info.is_identifier.base                                    :  551
//      +  aptos_framework.function_info.is_identifier.per_byte                                :  3
//      +  aptos_framework.function_info.load_function.base                                    :  551
//      +  aptos_framework.object.user_derived_address.base                                    :  14704
//      +  aptos_framework.transaction_context.chain_id.base                                   :  735
//      +  aptos_framework.transaction_context.entry_function_payload.base                     :  735
//      +  aptos_framework.transaction_context.entry_function_payload.per_abstract_memory_unit :  18
//      +  aptos_framework.transaction_context.fee_payer.base                                  :  735
//      +  aptos_framework.transaction_context.gas_unit_price.base                             :  735
//      +  aptos_framework.transaction_context.max_gas_amount.base                             :  735
//      +  aptos_framework.transaction_context.multisig_payload.base                           :  735
//      +  aptos_framework.transaction_context.multisig_payload.per_abstract_memory_unit       :  18
//      +  aptos_framework.transaction_context.secondary_signers.base                          :  735
//      +  aptos_framework.transaction_context.secondary_signers.per_signer                    :  576
//      +  aptos_framework.transaction_context.sender.base                                     :  735
//         txn.dependency_per_byte                                                             :  100 -> 42
//         txn.dependency_per_module                                                           :  4000 -> 74460
//      +  txn.keyless.base                                                                    :  414000000
//      +  txn.max_execution_gas.gov                                                           :  4000000000
//      +  txn.max_io_gas.gov                                                                  :  2000000000
//         txn.max_num_dependencies                                                            :  420 -> 512
//      +  txn.max_storage_fee.gov                                                             :  200000000
//      +  txn.max_transaction_size_in_bytes.gov                                               :  1048576
//         txn.storage_io_per_event_byte_write                                                 :  0 -> 89
//         txn.storage_io_per_transaction_byte_write   

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: Release Tooling

How Has This Been Tested?

TBA

Key Areas to Review

  • gas_schedule.move: fun set_for_next_epoch_check_hash
  • aptos-release-builder: changes to governance proposal generation, emitting delta

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 22, 2024

⏱️ 1h 6m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-targeted-unit-tests 31m 🟥
rust-move-unit-coverage 12m 🟥
rust-move-tests 11m 🟥
run-tests-main-branch 4m 🟩
rust-lints 3m 🟥
general-lints 2m 🟩
check-dynamic-deps 1m 🟩
semgrep/ci 28s 🟩
file_change_determinator 11s 🟩
file_change_determinator 10s 🟩
permission-check 4s 🟩
permission-check 4s 🟩
permission-check 3s 🟩
permission-check 2s 🟩

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

Job Duration vs 7d avg Delta
rust-targeted-unit-tests 31m 18m +71%

settingsfeedbackdocs ⋅ learn more about trunk.io

@vgao1996 vgao1996 force-pushed the gas-delta branch 2 times, most recently from e83d2bf to 0ce7761 Compare June 11, 2024 23:14
Copy link

codecov bot commented Jun 11, 2024

Codecov Report

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

Project coverage is 58.4%. Comparing base (bbce0f1) to head (0ce7761).
Report is 3 commits behind head on main.

Current head 0ce7761 differs from pull request most recent head b27da8c

Please upload reports for the commit b27da8c to get more accurate results.

Files Patch % Lines
types/src/on_chain_config/gas_schedule.rs 0.0% 30 Missing ⚠️
aptos-move/aptos-vm/src/gas.rs 0.0% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             main   #13391     +/-   ##
=========================================
- Coverage    58.4%    58.4%   -0.1%     
=========================================
  Files         823      822      -1     
  Lines      197664   197521    -143     
=========================================
- Hits       115603   115413    -190     
- Misses      82061    82108     +47     

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

@vgao1996 vgao1996 added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Jun 12, 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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@vgao1996 vgao1996 changed the title [draft][gas] generate delta when upgrading the gas schedule [gas] generate delta when upgrading the gas schedule Jun 12, 2024
@vgao1996 vgao1996 marked this pull request as ready for review June 12, 2024 16:33
@vgao1996 vgao1996 requested a review from 0xmaayan as a code owner June 12, 2024 16:33

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

@rahxephon89 rahxephon89 left a comment

Choose a reason for hiding this comment

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

The overall implementation looks good to me, thanks!

Copy link
Contributor

@runtian-zhou runtian-zhou left a comment

Choose a reason for hiding this comment

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

Was wondering how. does the gas schedule locator work? seems that it could be an existing URL or a local file. Where can those items be stored? I don't quite see how that option would work in practice.

@vgao1996
Copy link
Contributor Author

vgao1996 commented Jun 12, 2024

@runtian-zhou this is something Perry and I discussed extensively. I'll share more details later, but the gist is that

  • We will store the gas schedule snapshots in https://github.com/aptos-labs/aptos-networks or https://github.com/aptos-foundation/mainnet-proposals
    • @perryjrandall might be able to tell which is more suitable
    • Initially we may need to manually create the snapshots and sync them to the repo
    • Later however, automation will be added, so that the syncing will be done automatically after we tag commits in aptos-core
    • We are also planning to introduce semver style version selection, so that you say something like "fetch me the latest dot release of 1.15". This will even allow us to not have to update release.yaml when there's a fix.

@runtian-zhou
Copy link
Contributor

Got it! So we need more automation on the network repo to generate snapshots of different network automatically.

@vgao1996 vgao1996 enabled auto-merge (squash) June 12, 2024 18:40

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite compat success on a68e71c05caebf01504d4499110f3fba213fb53d ==> b27da8c0834ccca034c3343b44a83bd75f02304e

Compatibility test results for a68e71c05caebf01504d4499110f3fba213fb53d ==> b27da8c0834ccca034c3343b44a83bd75f02304e (PR)
1. Check liveness of validators at old version: a68e71c05caebf01504d4499110f3fba213fb53d
compatibility::simple-validator-upgrade::liveness-check : committed: 10692.57362242427 txn/s, latency: 3137.767414553473 ms, (p50: 2400 ms, p90: 6300 ms, p99: 11400 ms), latency samples: 362800
2. Upgrading first Validator to new version: b27da8c0834ccca034c3343b44a83bd75f02304e
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 5494.3593470537135 txn/s, latency: 5811.383632703623 ms, (p50: 5400 ms, p90: 8600 ms, p99: 9100 ms), latency samples: 193740
3. Upgrading rest of first batch to new version: b27da8c0834ccca034c3343b44a83bd75f02304e
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7219.928461778355 txn/s, latency: 4199.08078831312 ms, (p50: 3900 ms, p90: 5800 ms, p99: 6300 ms), latency samples: 253960
4. upgrading second batch to new version: b27da8c0834ccca034c3343b44a83bd75f02304e
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 12406.025713387711 txn/s, latency: 2544.8113774937806 ms, (p50: 2300 ms, p90: 2700 ms, p99: 6200 ms), latency samples: 410020
5. check swarm health
Compatibility test for a68e71c05caebf01504d4499110f3fba213fb53d ==> b27da8c0834ccca034c3343b44a83bd75f02304e passed
Test Ok

This comment has been minimized.

Copy link
Contributor

✅ Forge suite framework_upgrade success on a68e71c05caebf01504d4499110f3fba213fb53d ==> b27da8c0834ccca034c3343b44a83bd75f02304e

Compatibility test results for a68e71c05caebf01504d4499110f3fba213fb53d ==> b27da8c0834ccca034c3343b44a83bd75f02304e (PR)
Upgrade the nodes to version: b27da8c0834ccca034c3343b44a83bd75f02304e
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 816.3332046991109 txn/s, submitted: 818.3018539065846 txn/s, failed submission: 1.96864920747374 txn/s, expired: 1.96864920747374 txn/s, latency: 3657.864348874598 ms, (p50: 2600 ms, p90: 6900 ms, p99: 10300 ms), latency samples: 74640
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1318.9843657698943 txn/s, submitted: 1320.9625950398904 txn/s, failed submission: 1.9782292699960917 txn/s, expired: 1.9782292699960917 txn/s, latency: 2491.7317397825273 ms, (p50: 1800 ms, p90: 4200 ms, p99: 10500 ms), latency samples: 106680
5. check swarm health
Compatibility test for a68e71c05caebf01504d4499110f3fba213fb53d ==> b27da8c0834ccca034c3343b44a83bd75f02304e passed
Upgrade the remaining nodes to version: b27da8c0834ccca034c3343b44a83bd75f02304e
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1114.534160442045 txn/s, submitted: 1118.328129350238 txn/s, failed submission: 3.7939689081927845 txn/s, expired: 3.7939689081927845 txn/s, latency: 2787.086583900681 ms, (p50: 2100 ms, p90: 4800 ms, p99: 10200 ms), latency samples: 99880
Test Ok

This comment has been minimized.

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on b27da8c0834ccca034c3343b44a83bd75f02304e

two traffics test: inner traffic : committed: 9241.153497599496 txn/s, latency: 4255.194904501738 ms, (p50: 4200 ms, p90: 4500 ms, p99: 8600 ms), latency samples: 3986460
two traffics test : committed: 100.01640193595117 txn/s, latency: 2096.346590909091 ms, (p50: 2000 ms, p90: 2300 ms, p99: 8500 ms), latency samples: 1760
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.235, avg: 0.215", "QsPosToProposal: max: 1.978, avg: 1.916", "ConsensusProposalToOrdered: max: 0.303, avg: 0.286", "ConsensusOrderedToCommit: max: 0.392, avg: 0.382", "ConsensusProposalToCommit: max: 0.679, avg: 0.668"]
Max round gap was 1 [limit 4] at version 1755439. Max no progress secs was 5.178467 [limit 15] at version 1755439.
Test Ok

@vgao1996 vgao1996 merged commit 097921e into aptos-labs:main Jun 12, 2024
96 of 98 checks passed
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 CICD:run-framework-upgrade-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants