Optimize APT FA transactions - gas charging and transfering #13194

igor-aptos · 2024-05-03T17:55:36Z

Description

optimize apt PFS operations by specializing them in primary_fungible_store, and shortcircuiting unnecessary checks (owner/frozen/dispatchable/burned)
Add feature flags to default to FA for new accounts, and use exclusively FA (i.e. for apt transfers/gas charging). these not yet ready to be enabled on devnet, but we can for some tests.
add to genesis to create the mapping (i.e. so it's on on devnet/forge). migrate root account to have FA - as nobody will look at it in the explorer, but useful to test e2e
make concurrent supply default

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

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-05-03T17:55:39Z

⏱️ 6h 37m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-targeted-unit-tests	2h 22m	🟥 🟥 🟥 🟥 🟥
rust-move-unit-coverage	1h 44m	🟩 🟩 🟩 🟩 🟩
rust-move-tests	1h 8m	🟥 🟥 🟥 🟥 🟥
rust-lints	39m	🟩 🟥 🟩 🟩 🟩
run-tests-main-branch	25m	🟩 🟩 🟩 🟩 🟩 (+1 more)
general-lints	8m	🟩 🟩 🟩 🟩 🟩
check-dynamic-deps	6m	🟩 🟩 🟩 🟩 🟩
semgrep/ci	2m	🟩 🟩 🟩 🟩 🟩
file_change_determinator	58s	🟩 🟩 🟩 🟩 🟩
file_change_determinator	47s	🟩 🟩 🟩 🟩 🟩
permission-check	17s	🟩 🟩 🟩 🟩 🟩
permission-check	16s	🟩 🟩 🟩 🟩 🟩
permission-check	15s	🟩 🟩 🟩 🟩 🟩
permission-check	14s	🟩 🟩 🟩 🟩 🟩

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

Job	Duration	vs 7d avg	Delta
rust-targeted-unit-tests	34m	19m
rust-lints	10m	9m

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

codecov · 2024-05-03T18:16:48Z

Codecov Report

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

Project coverage is 33.1%. Comparing base (270917f) to head (305937c).
Report is 2 commits behind head on main.

Files	Patch %	Lines
...cached-packages/src/aptos_framework_sdk_builder.rs	0.0%	26 Missing ⚠️
crates/transaction-emitter-lib/src/emitter/mod.rs	0.0%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #13194     +/-   ##
=========================================
- Coverage    33.1%    33.1%   -0.1%     
=========================================
  Files        1753     1753             
  Lines      337763   337795     +32     
=========================================
- Hits       111995   111964     -31     
- Misses     225768   225831     +63

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

aptos-move/framework/aptos-framework/sources/aptos_account.move

movekevin · 2024-05-03T18:52:55Z

aptos-move/framework/aptos-framework/sources/coin.move

@@ -478,6 +478,20 @@ module aptos_framework::coin {
        (option::extract(burn_ref_opt), BurnRefReceipt { metadata })
    }

+    // Permanently convert to BurnRef, and take it from the pairing.
+    // (i.e. future calls to borrow/convert BurnRef will fail)
+    public fun convert_and_take_paired_burn_ref<CoinType>(


cc @lightmark. This might not make sense here before the end of the migration, as taking BurnRef means that the conversion for migration no longer works

conversion doesn't use BurnRef, but uses fungible_asset::burn_internal

This prevents calling get_paired_burn_ref afterwards. I am not sure if we can clone BurnRef somehow, and return it and keep it in pairing as well.

Also I am not sure if caller can have multiple BurnCapability objects, that each needs to be converted.

@lightmark , thoughts?

For APT, we call this only after features::operations_default_to_fa_apt_store() flag has been set, which will not be set until there are no more APT CoinStores

Refs do not allow clone/copy, so that why we use hot potato pattern here. As @igor-aptos said, the conversion does not need this for better perf so you can take Refs out anytime. The issue is this is a one-time call since there is only one copy of each ref but multiple for caps.

@lightmark does framework have multiple caps? If so - how are we going to convert them to Refs? do we need to actually clone/copy here with some friend function?

We have two mint caps...lemme think about it.

I think we cannot generate refs... mint cap should be stored in stake.move and burn is in transaction_fee.move ( transaction_fee is a friend of stake)

ok cool, so for this PR we are good (it is burn ref we are touching).
for transforming mint ref, we need to consolidate transaction_fee and stake modules

movekevin · 2024-05-03T18:53:59Z

aptos-move/framework/aptos-framework/sources/fungible_asset.move

+        store_exists_inline(store)
+    }
+
+    fun store_exists_inline(store: address): bool {


Why this change?

argh it's missing inline, but intention was for cheaper (and lower) gas costs

To save one function call? I'm not sure if it's worth making the code longer

store_exists_inline is a single operation - so inling leaves the byte representation the same. If you prefer - I could just not use store_exists in this file and do "exists<>" itself throughout.

aptos-move/framework/aptos-framework/sources/fungible_asset.move

movekevin · 2024-05-03T18:56:00Z

aptos-move/framework/aptos-framework/sources/primary_fungible_store.move

+        }
+    }
+
+    inline fun apt_ensure_primary_store_exists(


Why is this inline? A long inline function used multiple times = bloating up the bytecode

I made not huge private functions inline, to make things faster/cheaper, and was suggested to inline more things on other PRs.
What's the threshold for "function being too long for inline" in your view? here it is like if + 4 function calls, and below one is if + 2 function calls

bloating up the bytecode

@movekevin why is this a problem? The Aptos Framework does not face the same package size constraints as dApps, so what is the concern with trading off more bytecode size for less execution gas?

It's not but I'd rather not bloat the bytecode if possible. Also people look at the framework for examples

bloat the bytecode

@movekevin it it appropriate to consider increased bytecode size "bloat" if the bytecode size is a function of reducing gas size?

What is the problem with people looking at the framework for an example of gas-optimized code, especially for critical execution paths?

You can always add in a func comment explaining that things are inlined for the sake of performance

#13194 (comment)

and in general - inline is always done for performance, there is no other reasons to inline.

movekevin · 2024-05-03T18:56:13Z

aptos-move/framework/aptos-framework/sources/primary_fungible_store.move

-        if (!primary_store_exists(owner, metadata)) {
+        let store_addr = primary_store_address(owner, metadata);
+        if (fungible_asset::store_exists(store_addr)) {
+            object::address_to_object<FungibleStore>(store_addr)


nit: < FungibleStore > is not needed

movekevin · 2024-05-03T18:57:24Z

aptos-move/framework/aptos-framework/sources/primary_fungible_store.move

@@ -132,6 +147,31 @@ module aptos_framework::primary_fungible_store {
        }
    }

+    inline fun apt_store_address(account: address): address {


Same question here - why is this inline?

movekevin · 2024-05-03T18:58:05Z

aptos-move/framework/aptos-framework/sources/primary_fungible_store.move

+        }
+    }
+
+    public(friend) fun is_apt_balance_at_least(account: address, amount: u64): bool {


Doesn't feel like any apt-specific logic should be in PFS as it should remain generic. Can this be in aptos_account?

we are specializing APT PFS to be more efficient, so these are all variants of the same functions defined in this file. (so is easier to look and see)

but I can create aptos_primary_fungible_store.move if you prefer to move them out

moves to aptos_primary_fungible_store.move.

let me know if apt_primary_fungible_store name (i.e. aptos -> apt) makes more sense

Yeah apt makes more sense

igor-aptos · 2024-05-07T18:11:45Z

aptos-move/framework/aptos-framework/sources/aptos_primary_fungible_store.move

+        // use internal APIs, as they skip:
+        // - owner, frozen and dispatchable checks
+        // as APT cannot be frozen or have dispatch, and PFS cannot be transfered
+        // (PFS could potentially be burned. regular transfer would permanently unburn the store.
+        // Ignoring the check here has the equivalent of unburning, transfers, and then burning again)


to reviewers - want to call this out, as this biggest not-purely-optimization change, but a logical change in which invariants are checked.

igor-aptos · 2024-05-07T22:52:21Z

chatted with @movekevin offline - will measure what inline brings, but if it has measurable perf improvement, there are no concerns of using inlining for these short functions.

lightmark

Object is really annoying sometimes

lightmark · 2024-05-09T22:05:34Z

aptos-move/framework/aptos-framework/sources/apt_primary_fungible_store.move

+        }
+    }
+
+    public entry fun transfer(


My hunch is we should make all the functions in this module public(friend) for internal use only.

how should you transfer APT efficiently without wanting to create full account?

only aptos_account::transfer and primary_fungible_store::transfer are options, first one creates account, second one is much less performant.

lightmark · 2024-05-09T22:07:58Z

aptos-move/framework/aptos-framework/sources/aptos_account.move

-        coin::register<AptosCoin>(&signer);
+        let account_signer = account::create_account(auth_key);
+        if (features::new_accounts_default_to_fa_apt_store_enabled()) {
+            apt_primary_fungible_store::ensure_primary_store_exists(signer::address_of(&account_signer));


I don't think we need this line.

your coin.move functions, to keep same invariants, fail if target doesn't have neither coin store nor PFS

so this is required, if people use coin module to transfer APT

basically after I create an account, I should be able to receive APT

lightmark · 2024-05-09T22:16:37Z

aptos-move/framework/aptos-framework/sources/coin.move

@@ -478,6 +478,20 @@ module aptos_framework::coin {
        (option::extract(burn_ref_opt), BurnRefReceipt { metadata })
    }

+    // Permanently convert to BurnRef, and take it from the pairing.
+    // (i.e. future calls to borrow/convert BurnRef will fail)
+    public fun convert_and_take_paired_burn_ref<CoinType>(


Refs do not allow clone/copy, so that why we use hot potato pattern here. As @igor-aptos said, the conversion does not need this for better perf so you can take Refs out anytime. The issue is this is a one-time call since there is only one copy of each ref but multiple for caps.

lightmark · 2024-05-09T22:26:24Z

aptos-move/framework/aptos-framework/sources/transaction_fee.move

-            &borrow_global<AptosCoinCapabilities>(@aptos_framework).burn_cap,
-        );
+    public(friend) fun burn_fee(account: address, fee: u64) acquires AptosFABurnCapabilities, AptosCoinCapabilities {
+        if (exists<AptosFABurnCapabilities>(@aptos_framework)) {


after migration is done, right?

AptosFABurnCapabilities can be created only after migration is done.

if AptosFABurnCapabilities exists, other one doesn't, and so this is the only way to charge gas from that point

lightmark · 2024-05-09T22:27:17Z

aptos-move/framework/aptos-framework/sources/transaction_fee.move

+                let (burn_ref, burn_receipt) = coin::get_paired_burn_ref(burn_cap);
+                apt_primary_fungible_store::burn_from(&burn_ref, account, fee);
+                coin::return_paired_burn_ref(burn_ref, burn_receipt);


perf concering?

this will be slow, but we should convert to AptosFABurnCapabilities after nobody has CoinStore, and then this path will not be executed

aptos-move/framework/aptos-framework/sources/fungible_asset.move

…on flags also add unit tests

github-actions · 2024-05-28T17:57:30Z

✅ Forge suite `compat` success on `3ffe0986b5fe4acb76544ae7ae85d73b91a6a411` ==> `305937c24f6fc3f5cde35045da6715b0d7616dbb`

Compatibility test results for 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 305937c24f6fc3f5cde35045da6715b0d7616dbb (PR)
1. Check liveness of validators at old version: 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411
compatibility::simple-validator-upgrade::liveness-check : committed: 6532.268900886151 txn/s, latency: 4687.347972434672 ms, (p50: 4800 ms, p90: 5300 ms, p99: 7800 ms), latency samples: 251040
2. Upgrading first Validator to new version: 305937c24f6fc3f5cde35045da6715b0d7616dbb
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 3325.7361729596823 txn/s, latency: 9338.678352873892 ms, (p50: 9500 ms, p90: 13900 ms, p99: 14500 ms), latency samples: 139880
3. Upgrading rest of first batch to new version: 305937c24f6fc3f5cde35045da6715b0d7616dbb
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 3348.3818270570355 txn/s, latency: 9228.036518380775 ms, (p50: 9300 ms, p90: 13900 ms, p99: 14400 ms), latency samples: 139820
4. upgrading second batch to new version: 305937c24f6fc3f5cde35045da6715b0d7616dbb
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 6359.512078262484 txn/s, latency: 5132.229266226063 ms, (p50: 4800 ms, p90: 8400 ms, p99: 9600 ms), latency samples: 236040
5. check swarm health
Compatibility test for 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 305937c24f6fc3f5cde35045da6715b0d7616dbb passed
Test Ok

github-actions · 2024-05-28T17:58:38Z

✅ Forge suite `realistic_env_max_load` success on `305937c24f6fc3f5cde35045da6715b0d7616dbb`

two traffics test: inner traffic : committed: 8195.109241869624 txn/s, latency: 4784.082090091514 ms, (p50: 4800 ms, p90: 6000 ms, p99: 10200 ms), latency samples: 3542620
two traffics test : committed: 100.05585950337796 txn/s, latency: 1816.5713483146067 ms, (p50: 1800 ms, p90: 2000 ms, p99: 2400 ms), latency samples: 1780
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.218, avg: 0.205", "QsPosToProposal: max: 0.258, avg: 0.240", "ConsensusProposalToOrdered: max: 0.447, avg: 0.411", "ConsensusOrderedToCommit: max: 0.370, avg: 0.356", "ConsensusProposalToCommit: max: 0.779, avg: 0.767"]
Max round gap was 1 [limit 4] at version 1604050. Max no progress secs was 4.871368 [limit 15] at version 1604050.
Test Ok

github-actions · 2024-05-28T17:59:57Z

✅ Forge suite `framework_upgrade` success on `3ffe0986b5fe4acb76544ae7ae85d73b91a6a411` ==> `305937c24f6fc3f5cde35045da6715b0d7616dbb`

Compatibility test results for 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 305937c24f6fc3f5cde35045da6715b0d7616dbb (PR)
Upgrade the nodes to version: 305937c24f6fc3f5cde35045da6715b0d7616dbb
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1235.6989206824774 txn/s, submitted: 1237.2945474016763 txn/s, failed submission: 1.5956267191989193 txn/s, expired: 1.5956267191989193 txn/s, latency: 2669.8135491606713 ms, (p50: 2100 ms, p90: 4500 ms, p99: 9600 ms), latency samples: 108420
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1170.799721686851 txn/s, submitted: 1172.2281068698767 txn/s, failed submission: 1.428385183025845 txn/s, expired: 1.428385183025845 txn/s, latency: 2739.6702826352175 ms, (p50: 2100 ms, p90: 4800 ms, p99: 9500 ms), latency samples: 98360
5. check swarm health
Compatibility test for 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 305937c24f6fc3f5cde35045da6715b0d7616dbb passed
Upgrade the remaining nodes to version: 305937c24f6fc3f5cde35045da6715b0d7616dbb
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1266.493918226074 txn/s, submitted: 1269.0468654682377 txn/s, failed submission: 2.552947242163609 txn/s, expired: 2.552947242163609 txn/s, latency: 2629.498772219168 ms, (p50: 2100 ms, p90: 4500 ms, p99: 10400 ms), latency samples: 109140
Test Ok

igor-aptos requested review from davidiw, lightmark and movekevin May 3, 2024 17:55

igor-aptos requested a review from wrwg as a code owner May 3, 2024 17:55

movekevin reviewed May 3, 2024

View reviewed changes

aptos-move/framework/aptos-framework/sources/aptos_account.move Show resolved Hide resolved

movekevin reviewed May 3, 2024

View reviewed changes

aptos-move/framework/aptos-framework/sources/fungible_asset.move Show resolved Hide resolved

movekevin reviewed May 3, 2024

View reviewed changes

igor-aptos mentioned this pull request May 4, 2024

Eliminate call path inefficiencies for concurrent balance FA APT transfers, make assorted fixes #13203

Closed

igor-aptos force-pushed the igor/optimize_fa_apt branch from c5ab597 to 3c74ed1 Compare May 7, 2024 14:37

igor-aptos requested a review from junkil-park as a code owner May 7, 2024 14:37

igor-aptos requested a review from movekevin May 7, 2024 18:04

igor-aptos commented May 7, 2024

View reviewed changes

igor-aptos force-pushed the igor/optimize_fa_apt branch from 6d29055 to dcc10e9 Compare May 9, 2024 17:09

lightmark approved these changes May 10, 2024

View reviewed changes

igor-aptos requested a review from runtian-zhou May 10, 2024 01:07

igor-aptos requested review from banool, gregnazario and 0xmaayan as code owners May 10, 2024 17:51

This was referenced May 16, 2024

Enable CONCURRENT_FUNGIBLE_ASSET_SUPPLY by default #13295

Closed

enable APT coin <-> FA mapping in genesis #13296

Merged

igor-aptos force-pushed the igor/optimize_fa_apt branch 2 times, most recently from 1d511ec to 5285f5e Compare May 16, 2024 20:09

igor-aptos and others added 3 commits May 26, 2024 21:44

specs

5820cb8

making root account use flags, and make MigrationFlag check be based …

47bcfd6

…on flags also add unit tests

mockup spec for register_apt to unblock the Prover test failure

1afc3d5

igor-aptos force-pushed the igor/optimize_fa_apt branch 2 times, most recently from ae06727 to b2a904d Compare May 28, 2024 06:54

review comments, moving apt_primary_fungible_store => aptos_account

4a19bb1

igor-aptos force-pushed the igor/optimize_fa_apt branch from b2a904d to 4a19bb1 Compare May 28, 2024 14:37

igor-aptos enabled auto-merge (squash) May 28, 2024 15:19

This comment has been minimized.

Sign in to view

adding back feature being deprecated, as compatibility test failed

305937c

igor-aptos force-pushed the igor/optimize_fa_apt branch from 9ae9494 to 305937c Compare May 28, 2024 17:25

This comment has been minimized.

Sign in to view

igor-aptos merged commit d972e92 into main May 28, 2024
53 of 54 checks passed

igor-aptos deleted the igor/optimize_fa_apt branch May 28, 2024 18:03

Optimize APT FA transactions - gas charging and transfering #13194

Optimize APT FA transactions - gas charging and transfering #13194

Conversation

igor-aptos commented May 3, 2024 • edited

Description

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 May 3, 2024 • edited

codecov bot commented May 3, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alnoki May 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

movekevin May 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igor-aptos commented May 7, 2024

lightmark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igor-aptos May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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.

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 May 28, 2024

✅ Forge suite compat success on 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 305937c24f6fc3f5cde35045da6715b0d7616dbb

github-actions bot commented May 28, 2024

✅ Forge suite realistic_env_max_load success on 305937c24f6fc3f5cde35045da6715b0d7616dbb

github-actions bot commented May 28, 2024

✅ Forge suite framework_upgrade success on 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> 305937c24f6fc3f5cde35045da6715b0d7616dbb

igor-aptos commented May 3, 2024 •

edited

trunk-io bot commented May 3, 2024 •

edited

codecov bot commented May 3, 2024 •

edited

alnoki May 4, 2024 •

edited

movekevin May 3, 2024 •

edited

igor-aptos May 10, 2024 •

edited

✅ Forge suite `compat` success on `3ffe0986b5fe4acb76544ae7ae85d73b91a6a411` ==> `305937c24f6fc3f5cde35045da6715b0d7616dbb`

✅ Forge suite `realistic_env_max_load` success on `305937c24f6fc3f5cde35045da6715b0d7616dbb`

✅ Forge suite `framework_upgrade` success on `3ffe0986b5fe4acb76544ae7ae85d73b91a6a411` ==> `305937c24f6fc3f5cde35045da6715b0d7616dbb`