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

[cli] Make large packages module address configurable #15118

Merged
merged 3 commits into from
Oct 30, 2024

Conversation

0xjunha
Copy link
Contributor

@0xjunha 0xjunha commented Oct 30, 2024

Description

  • Added a new --large-packages-module-address command option for Move package publishing:

    • aptos move publish
    • aptos move deploy-object
    • aptos move upgrade-object
    • aptos move create-object-and-publish-package
    • aptos move upgrade-object-package
    • aptos move clear-staging-area
  • This option will be used alongside the --chunked-publish flag. If provided, the specified large packages module address will be used.

  • Note that the CLI does not verify if the module actually exists at the given address; it will simply abort if an incorrect address is supplied.

  • The default address will be used if no address is provided.

  • This feature allows users to utilize the chunked publishing in the localnet testing environment by manually deploying the large_packages.move module.

Example

aptos move deploy-object <other options> --chunked-publish --large-packages-module-address 0x1234...

How Has This Been Tested?

  • e2e-move-tests
  • Manually tested CLI options

Copy link

trunk-io bot commented Oct 30, 2024

⏱️ 2h 24m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
execution-performance / single-node-performance 23m 🟩
rust-targeted-unit-tests 17m 🟩
rust-targeted-unit-tests 16m 🟩
rust-move-tests 10m 🟩
rust-move-tests 10m 🟩
rust-move-tests 9m 🟩
rust-move-tests 9m 🟩
check 8m 🟩
rust-cargo-deny 7m 🟩🟩🟩🟩
check-dynamic-deps 6m 🟩🟩🟩🟩
rust-doc-tests 5m 🟩
test-target-determinator 5m 🟩
execution-performance / test-target-determinator 4m 🟩
rust-lints 4m 🟩
rust-lints 4m 🟩

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

Job Duration vs 7d avg Delta
check 8m 4m +107%

settingsfeedbackdocs ⋅ learn more about trunk.io

Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

looks good, fun thing was I had an env variable version that I was going to ship, but I needed to configure the new laptop with github support

please run cargo xclippy before shipping PRs, it will save you time!

@0xjunha
Copy link
Contributor Author

0xjunha commented Oct 30, 2024

Ahh I should have messaged you earlier. Yes I think I missed the test case - thanks for pointing that out!

@@ -2350,4 +2351,16 @@ pub struct ChunkedPublishOption {
/// Use this option for publishing large packages exceeding `MAX_PUBLISH_PACKAGE_SIZE`.
#[clap(long)]
pub(crate) chunked_publish: bool,

/// Address of the `large_packages` move module for chunked publishing
#[clap(long)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this is Option instead of just String with a default value at the clap level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That will be better, I'll fix it. Thanks!

- Added a new command option `--large-packages-module-address` to commands related to Move package publishing.

- The default address will be used if no value is provided.
@0xjunha 0xjunha force-pushed the large_packages_configurable branch from 916b8c0 to 7237d82 Compare October 30, 2024 08:32
@0xjunha 0xjunha requested a review from banool October 30, 2024 08:59
Copy link
Contributor

@gregnazario gregnazario left a comment

Choose a reason for hiding this comment

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

Did you test it out on devnet?

@@ -1649,6 +1700,10 @@ async fn is_staging_area_empty(txn_options: &TransactionOptions) -> CliTypedResu
pub struct ClearStagingArea {
#[clap(flatten)]
pub(crate) txn_options: TransactionOptions,

/// Address of the `large_packages` move module for chunked publishing
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add another line that you only have to override when not on testnet or mainnet

@gregnazario gregnazario enabled auto-merge (squash) October 30, 2024 14:14

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 7237d82b6d642bef073e03737630645fa622cc9f

two traffics test: inner traffic : committed: 14319.31 txn/s, latency: 2774.96 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3300 ms), latency samples: 5444560
two traffics test : committed: 99.98 txn/s, latency: 1558.44 ms, (p50: 1400 ms, p70: 1400, p90: 1500 ms, p99: 7700 ms), latency samples: 1780
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 2.024, avg: 1.580", "ConsensusProposalToOrdered: max: 0.326, avg: 0.295", "ConsensusOrderedToCommit: max: 0.371, avg: 0.359", "ConsensusProposalToCommit: max: 0.665, avg: 0.654"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.15s no progress at version 38692 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.86s no progress at version 2802939 (avg 8.86s) [limit 15].
Test Ok

Copy link
Contributor

✅ Forge suite framework_upgrade success on 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd ==> 7237d82b6d642bef073e03737630645fa622cc9f

Compatibility test results for 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd ==> 7237d82b6d642bef073e03737630645fa622cc9f (PR)
Upgrade the nodes to version: 7237d82b6d642bef073e03737630645fa622cc9f
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1238.78 txn/s, submitted: 1242.14 txn/s, failed submission: 3.36 txn/s, expired: 3.36 txn/s, latency: 2528.86 ms, (p50: 2100 ms, p70: 2500, p90: 3900 ms, p99: 5800 ms), latency samples: 110620
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1203.63 txn/s, submitted: 1207.26 txn/s, failed submission: 3.63 txn/s, expired: 3.63 txn/s, latency: 2467.90 ms, (p50: 2100 ms, p70: 2400, p90: 4500 ms, p99: 7200 ms), latency samples: 106120
5. check swarm health
Compatibility test for 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd ==> 7237d82b6d642bef073e03737630645fa622cc9f passed
Upgrade the remaining nodes to version: 7237d82b6d642bef073e03737630645fa622cc9f
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1303.09 txn/s, submitted: 1306.22 txn/s, failed submission: 3.12 txn/s, expired: 3.12 txn/s, latency: 2293.77 ms, (p50: 2100 ms, p70: 2400, p90: 3000 ms, p99: 4500 ms), latency samples: 116860
Test Ok

Copy link
Contributor

✅ Forge suite compat success on 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd ==> 7237d82b6d642bef073e03737630645fa622cc9f

Compatibility test results for 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd ==> 7237d82b6d642bef073e03737630645fa622cc9f (PR)
1. Check liveness of validators at old version: 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd
compatibility::simple-validator-upgrade::liveness-check : committed: 14819.31 txn/s, latency: 2237.00 ms, (p50: 1900 ms, p70: 2100, p90: 2600 ms, p99: 8100 ms), latency samples: 562260
2. Upgrading first Validator to new version: 7237d82b6d642bef073e03737630645fa622cc9f
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 6752.11 txn/s, latency: 4164.06 ms, (p50: 4800 ms, p70: 5100, p90: 5200 ms, p99: 5300 ms), latency samples: 123780
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6367.03 txn/s, latency: 5013.09 ms, (p50: 5100 ms, p70: 5200, p90: 7000 ms, p99: 7200 ms), latency samples: 210720
3. Upgrading rest of first batch to new version: 7237d82b6d642bef073e03737630645fa622cc9f
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 5992.32 txn/s, latency: 4732.53 ms, (p50: 5200 ms, p70: 5500, p90: 6100 ms, p99: 6300 ms), latency samples: 116260
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 5917.14 txn/s, latency: 5517.31 ms, (p50: 5900 ms, p70: 6300, p90: 6800 ms, p99: 7100 ms), latency samples: 206640
4. upgrading second batch to new version: 7237d82b6d642bef073e03737630645fa622cc9f
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 9083.70 txn/s, latency: 3156.87 ms, (p50: 3600 ms, p70: 3700, p90: 3900 ms, p99: 4200 ms), latency samples: 157760
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 8005.00 txn/s, latency: 3735.58 ms, (p50: 3700 ms, p70: 3900, p90: 4300 ms, p99: 5400 ms), latency samples: 294620
5. check swarm health
Compatibility test for 9c922ebe94f5ff4b58df4617f3ff003e2ce10ccd ==> 7237d82b6d642bef073e03737630645fa622cc9f passed
Test Ok

@gregnazario gregnazario merged commit c44f1a3 into aptos-labs:main Oct 30, 2024
92 checks passed
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.

4 participants