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

Concurrent FungibleAsset balance #11183

Merged
merged 4 commits into from
May 29, 2024
Merged

Conversation

igor-aptos
Copy link
Contributor

Description

Test Plan

Copy link

trunk-io bot commented Dec 4, 2023

⏱️ 2h 6m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-unit-tests 1h 10m 🟩🟩
windows-build 20m 🟩
rust-images / rust-all 9m 🟩
rust-lints 8m 🟩
check-dynamic-deps 6m 🟩🟩
run-tests-main-branch 4m 🟩
check 4m 🟩
general-lints 2m 🟩
docs-lint 41s 🟩
semgrep/ci 25s 🟩
file_change_determinator 14s 🟩
file_change_determinator 11s 🟩
file_change_determinator 10s 🟩
permission-check 4s 🟩
permission-check 3s 🟩
permission-check 2s 🟩
permission-check 2s 🟩
determine-docker-build-metadata 1s 🟩
permission-check 1s 🟩

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

Job Duration vs 7d avg Delta
rust-lints 8m 7m +20%

settingsfeedbackdocs ⋅ learn more about trunk.io

@igor-aptos
Copy link
Contributor Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@igor-aptos igor-aptos marked this pull request as draft December 4, 2023 19:48
Copy link
Contributor

This issue is stale because it has been open 45 days with no activity. Remove the stale label, comment or push a commit - otherwise this will be closed in 15 days.

Comment on lines 153 to 190
struct DepositEventV2 has drop, store {
metadata_address: address,
amount: u64,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
struct DepositEventV2 has drop, store {
metadata_address: address,
amount: u64,
}
struct Deposit has drop, store {
metadata: address,
amount: u64,
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's agree on the naming convention.hah

@igor-aptos igor-aptos force-pushed the igor/concurrent_fungible_balance branch from 642b937 to 21cdef5 Compare February 22, 2024 23:27
@igor-aptos igor-aptos changed the title [draft] Concurrent FungibleAsset balance Concurrent FungibleAsset balance Feb 23, 2024
@igor-aptos igor-aptos marked this pull request as ready for review February 23, 2024 00:01
@@ -111,6 +111,13 @@ module aptos_framework::fungible_asset {
frozen: bool,
}

#[resource_group_member(group = aptos_framework::object::ObjectGroup)]
/// The store object that holds concurrent fungible asset balance
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: . is missing

btw, in Move, can we place annotation after the docstring?

@@ -155,6 +162,14 @@ module aptos_framework::fungible_asset {
frozen: bool,
}

fun default_to_concurrent_fungible_supply(): bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why add these functions? To me features::concurrent_fungible_assets_enabled() seems easier to parse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in case we want to add multiple flags, to separately decide which path to take

@@ -289,6 +304,12 @@ module aptos_framework::fungible_asset {
exists<FungibleStore>(store)
}

#[view]
/// Return whether the provided address has a store initialized.
Copy link
Contributor

Choose a reason for hiding this comment

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

the comment is wrong, also what is store?

public fun balance<T: key>(store: Object<T>): u64 acquires FungibleStore, ConcurrentFungibleBalance {
let store_addr = object::object_address(&store);
if (concurrent_fungible_balance_exists(store_addr)) {
let balance = borrow_global<ConcurrentFungibleBalance>(store_addr);
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually prefer

let balance = &borrow_global<ConcurrentFungibleBalance>(store_addr).balance;
aggregator_v2::read(balance)

because balance.balance looks super weird. Either things need to be called differently, or we can access the field directly when we borrow global

if (store_exists(object::object_address(&store))) {
public fun balance<T: key>(store: Object<T>): u64 acquires FungibleStore, ConcurrentFungibleBalance {
let store_addr = object::object_address(&store);
if (concurrent_fungible_balance_exists(store_addr)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check here that && store_exists(store_addr) as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be part of concurrent_fungible_balance_exists function?

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 should be a general invariant, but not sure we want to/need to explicitly check it here?

@@ -386,22 +424,23 @@ module aptos_framework::fungible_asset {
event::destroy_handle(deposit_events);
event::destroy_handle(withdraw_events);
event::destroy_handle(frozen_events);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove this newline?

aggregator_v2::add(&mut balance.balance, amount);
} else {
let store = borrow_global_mut<FungibleStore>(store_addr);
store.balance = store.balance + amount;
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't care about overflows? Interesting, why?

Copy link
Contributor

Choose a reason for hiding this comment

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

let store = borrow_global_mut<FungibleStore>(store_addr);
store.balance = store.balance + amount;

if (concurrent_fungible_balance_exists(store_addr)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto about checking store exists and avoiding balance.balance

let balance = borrow_global_mut<ConcurrentFungibleBalance>(store_addr);
assert!(aggregator_v2::try_sub(&mut balance.balance, amount), error::invalid_argument(EINSUFFICIENT_BALANCE));
} else {
let store = borrow_global_mut<FungibleStore>(store_addr);
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't check store exists here, is the check somewhere else?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes. delegate to caller

let store_object_address = object::address_from_extend_ref(ref);
let store_object_signer = object::generate_signer_for_extending(ref);
assert!(features::concurrent_fungible_assets_enabled(), error::invalid_argument(ECONCURRENT_SUPPLY_NOT_ENABLED));
assert!(exists<FungibleStore>(store_object_address), error::not_found(ESTORE_NOT_FOUND));
Copy link
Contributor

Choose a reason for hiding this comment

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

reuse store_exists?

@@ -656,6 +708,25 @@ module aptos_framework::fungible_asset {
move_to(&metadata_object_signer, supply);
}

public fun upgrade_store_to_concurrent(
ref: &ExtendRef,

This comment was marked as resolved.


<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

?

This comment was marked as resolved.

@@ -11,18 +11,32 @@ metadata object can be any object that equipped with <code><a href="fungible_ass
- [Resource `ConcurrentSupply`](#0x1_fungible_asset_ConcurrentSupply)
- [Resource `Metadata`](#0x1_fungible_asset_Metadata)
- [Resource `FungibleStore`](#0x1_fungible_asset_FungibleStore)
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

rerun>?

This comment was marked as resolved.

aggregator_v2::add(&mut balance.balance, amount);
} else {
let store = borrow_global_mut<FungibleStore>(store_addr);
store.balance = store.balance + amount;
Copy link
Contributor

Choose a reason for hiding this comment

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

let balance = borrow_global_mut<ConcurrentFungibleBalance>(store_addr);
assert!(aggregator_v2::try_sub(&mut balance.balance, amount), error::invalid_argument(EINSUFFICIENT_BALANCE));
} else {
let store = borrow_global_mut<FungibleStore>(store_addr);
Copy link
Contributor

Choose a reason for hiding this comment

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

yes. delegate to caller

Copy link

codecov bot commented Mar 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 33.1%. Comparing base (d3bb8f6) to head (c6b9ae5).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##             main   #11183     +/-   ##
=========================================
- Coverage    33.1%    33.1%   -0.1%     
=========================================
  Files        1753     1754      +1     
  Lines      337871   337934     +63     
=========================================
- Hits       112000   111959     -41     
- Misses     225871   225975    +104     

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

Copy link
Contributor

@alnoki alnoki left a comment

Choose a reason for hiding this comment

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

How about strategic inlining to reduce gas bloat?

A lot of these helper functions can easily be inlined, and failing to optimize them here will have adverse consequences downstream because they are inner calls for lots of call flows across the framework

(Each function call comes with a fixed gas cost due to stack allocation, but inline functions are effectively free because they are essentially copy-pasted at compile time)

Comment on lines 165 to 216

fun default_to_concurrent_fungible_supply(): bool {
features::concurrent_fungible_assets_enabled()
}

fun default_to_concurrent_fungible_balance(): bool {
features::concurrent_fungible_assets_enabled()
}

fun auto_upgrade_to_concurrent_fungible_balance(): bool {
features::concurrent_fungible_assets_enabled()
}

This comment was marked as resolved.

Comment on lines 312 to 473
#[view]
/// Return whether the provided address has a concurrent fungible balance initialized,
/// at the fungible store address.
public fun concurrent_fungible_balance_exists(store: address): bool {
exists<ConcurrentFungibleBalance>(store)
}
Copy link
Contributor

@alnoki alnoki Mar 5, 2024

Choose a reason for hiding this comment

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

Suggested change
#[view]
/// Return whether the provided address has a concurrent fungible balance initialized,
/// at the fungible store address.
public fun concurrent_fungible_balance_exists(store: address): bool {
exists<ConcurrentFungibleBalance>(store)
}
#[view]
/// Return whether the provided address has a concurrent fungible balance initialized,
/// at the fungible store address.
public fun concurrent_fungible_balance_exists(store: address): bool {
exists<ConcurrentFungibleBalance>(store)
}
inline fun concurrent_fungible_balance_exists_inline(store: address): bool {
exists<ConcurrentFungibleBalance>(store)
}
inline fun store_exists_inline(store: address): bool {
exists<FungibleStore>(store)
}

How about adding inline versions here as well, which can be used in control flow statements?

Comment on lines 339 to 348
if (concurrent_fungible_balance_exists(store_addr)) {
let balance_resource = borrow_global<ConcurrentFungibleBalance>(store_addr);
aggregator_v2::read(&balance_resource.balance)
} else if (store_exists(store_addr)) {
Copy link
Contributor

@alnoki alnoki Mar 5, 2024

Choose a reason for hiding this comment

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

Suggested change
if (concurrent_fungible_balance_exists(store_addr)) {
let balance_resource = borrow_global<ConcurrentFungibleBalance>(store_addr);
aggregator_v2::read(&balance_resource.balance)
} else if (store_exists(store_addr)) {
if (concurrent_fungible_balance_exists_inline(store_addr)) {
let balance_resource = borrow_global<ConcurrentFungibleBalance>(store_addr);
aggregator_v2::read(&balance_resource.balance)
} else if (store_exists_inline(store_addr)) {

To reduce gas bloat

Copy link
Contributor

Choose a reason for hiding this comment

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

you are so paranoid, hah

Copy link
Contributor

Choose a reason for hiding this comment

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

@alnoki has a point, we have many single-block functions, which are extra costs and extra performance losses. Why not inline if we could

Comment on lines 417 to 663

if (concurrent_fungible_balance_exists(addr)) {
let ConcurrentFungibleBalance { balance } = move_from<ConcurrentFungibleBalance>(addr);
Copy link
Contributor

@alnoki alnoki Mar 5, 2024

Choose a reason for hiding this comment

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

Suggested change
if (concurrent_fungible_balance_exists(addr)) {
let ConcurrentFungibleBalance { balance } = move_from<ConcurrentFungibleBalance>(addr);
if (concurrent_fungible_balance_exists_inline(addr)) {
let ConcurrentFungibleBalance { balance } = move_from<ConcurrentFungibleBalance>(addr);

To reduce gas bloat?

let store = borrow_global_mut<FungibleStore>(store_addr);
store.balance = store.balance + amount;

if (auto_upgrade_to_concurrent_fungible_balance() || concurrent_fungible_balance_exists(store_addr)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (auto_upgrade_to_concurrent_fungible_balance() || concurrent_fungible_balance_exists(store_addr)) {
if (auto_upgrade_to_concurrent_fungible_balance() || concurrent_fungible_balance_exists_inline(store_addr)) {

To reduce gas bloat


if (auto_upgrade_to_concurrent_fungible_balance() || concurrent_fungible_balance_exists(store_addr)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (auto_upgrade_to_concurrent_fungible_balance() || concurrent_fungible_balance_exists(store_addr)) {
if (auto_upgrade_to_concurrent_fungible_balance() || concurrent_fungible_balance_exists_inline(store_addr)) {

To reduce gas bloat

@igor-aptos
Copy link
Contributor Author

@alnoki I'll include inlines to individual functions, but I'll skip creating duplicate functions for that.

once @lightmark changes land as well, we will stress test this, and see if it makes any difference, seems probably minor.

@igor-aptos igor-aptos force-pushed the igor/concurrent_fungible_balance branch from b1f5998 to 4b7ae39 Compare March 8, 2024 18:47
@igor-aptos
Copy link
Contributor Author

igor-aptos commented Mar 8, 2024

I'll add more context on one design choice, and anyone has feedback/thoughts, let me know.

With the current code, once this flag is enabled, and stores get migrated, individual store will contain two resources in the resource group:
FungibleStore and ConcurrentFungibleBalance.

we cannot remove balance field from FungibleStore - so it will still be present there, just deprecated, and always 0.

Alternative would be to have create ConcurrentFungibleStore, to replace FungibleStore, with all the same fields, just a different type for the balance. That way, in the future FungibleStore is deprecated and doesn't exist onchain, and store only contains clean ConcurrentFungibleStore resource.

That is in general doable, except for the create_store function, which currently returns Object<FungibleStore>. Object is just a wrapped address, but it asserts that object exists at that address, and so:

  • if someone has stored this reference in their code, doing migration internally would break it (I think?)
  • we cannot change signature of create_store, so we would need to permanently support creating deprecated store, and then migrating it on next call, and having separate create_concurrent_store to be used instead.

Any thoughts?

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Base automatically changed from igor/is_at_least_api to main May 28, 2024 23:12
@igor-aptos igor-aptos force-pushed the igor/concurrent_fungible_balance branch from 7b8045d to 04873c5 Compare May 28, 2024 23:44
@igor-aptos igor-aptos enabled auto-merge (squash) May 28, 2024 23:45

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.

@igor-aptos igor-aptos force-pushed the igor/concurrent_fungible_balance branch from b48a610 to c6b9ae5 Compare May 29, 2024 02:20

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite compat success on 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> c6b9ae57cab7e9863aa17633a64aa81c7a6ed7d6

Compatibility test results for 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> c6b9ae57cab7e9863aa17633a64aa81c7a6ed7d6 (PR)
1. Check liveness of validators at old version: 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411
compatibility::simple-validator-upgrade::liveness-check : committed: 6856.570716880554 txn/s, latency: 4811.060124443712 ms, (p50: 4800 ms, p90: 8000 ms, p99: 8500 ms), latency samples: 242680
2. Upgrading first Validator to new version: c6b9ae57cab7e9863aa17633a64aa81c7a6ed7d6
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 3401.803287405923 txn/s, latency: 9155.552263581489 ms, (p50: 9600 ms, p90: 13800 ms, p99: 14200 ms), latency samples: 139160
3. Upgrading rest of first batch to new version: c6b9ae57cab7e9863aa17633a64aa81c7a6ed7d6
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 2662.575701144134 txn/s, latency: 11135.824768188302 ms, (p50: 12600 ms, p90: 15000 ms, p99: 15700 ms), latency samples: 112160
4. upgrading second batch to new version: c6b9ae57cab7e9863aa17633a64aa81c7a6ed7d6
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 6321.084312654622 txn/s, latency: 5152.101468950749 ms, (p50: 4800 ms, p90: 8800 ms, p99: 10100 ms), latency samples: 233500
5. check swarm health
Compatibility test for 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> c6b9ae57cab7e9863aa17633a64aa81c7a6ed7d6 passed
Test Ok

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on c6b9ae57cab7e9863aa17633a64aa81c7a6ed7d6

two traffics test: inner traffic : committed: 8075.8350789713395 txn/s, latency: 4841.267981576979 ms, (p50: 4500 ms, p90: 6600 ms, p99: 10500 ms), latency samples: 3502140
two traffics test : committed: 99.96212672063903 txn/s, latency: 1899.8264367816091 ms, (p50: 1900 ms, p90: 2100 ms, p99: 4800 ms), latency samples: 1740
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.217, avg: 0.207", "QsPosToProposal: max: 0.313, avg: 0.232", "ConsensusProposalToOrdered: max: 0.453, avg: 0.408", "ConsensusOrderedToCommit: max: 0.377, avg: 0.352", "ConsensusProposalToCommit: max: 0.790, avg: 0.760"]
Max round gap was 1 [limit 4] at version 1761529. Max no progress secs was 4.67488 [limit 15] at version 1761529.
Test Ok

Copy link
Contributor

✅ Forge suite framework_upgrade success on 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> c6b9ae57cab7e9863aa17633a64aa81c7a6ed7d6

Compatibility test results for 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> c6b9ae57cab7e9863aa17633a64aa81c7a6ed7d6 (PR)
Upgrade the nodes to version: c6b9ae57cab7e9863aa17633a64aa81c7a6ed7d6
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1290.4309344846658 txn/s, submitted: 1292.1034056087897 txn/s, failed submission: 1.6724711241238033 txn/s, expired: 1.6724711241238033 txn/s, latency: 2535.329355674875 ms, (p50: 1800 ms, p90: 4500 ms, p99: 7900 ms), latency samples: 108020
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1089.4420297440763 txn/s, submitted: 1091.6563428126617 txn/s, failed submission: 2.2143130685855206 txn/s, expired: 2.2143130685855206 txn/s, latency: 2764.316463414634 ms, (p50: 2100 ms, p90: 4800 ms, p99: 9700 ms), latency samples: 98400
5. check swarm health
Compatibility test for 3ffe0986b5fe4acb76544ae7ae85d73b91a6a411 ==> c6b9ae57cab7e9863aa17633a64aa81c7a6ed7d6 passed
Upgrade the remaining nodes to version: c6b9ae57cab7e9863aa17633a64aa81c7a6ed7d6
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1146.117462852096 txn/s, submitted: 1148.107250113992 txn/s, failed submission: 1.989787261896 txn/s, expired: 1.989787261896 txn/s, latency: 2582.8349054783953 ms, (p50: 2000 ms, p90: 4500 ms, p99: 7700 ms), latency samples: 103680
Test Ok

@igor-aptos igor-aptos merged commit 6ba90fc into main May 29, 2024
47 of 48 checks passed
@igor-aptos igor-aptos deleted the igor/concurrent_fungible_balance branch May 29, 2024 02:54
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants