fix concurrency issue in governed gas pool

[seanyoung]

Description

The governed gas pool implementation does not scale well. The issue was that the gas pool did not use a concurrent fungible asset store.

How Has This Been Tested?

cargo test -p smoke-test test_multivalidator_staking_reward -- --show-output

(This PR also fixes a few races in test_multivalidator_staking_reward)

 ./testsuite/performance_benchmark.sh --short

Add the line ``--disable-feature GOVERNED_GAS_POOL` to see the performance without governed gas pool, which was much better that with governed gas pools enabled. Now with this change, there is no difference in performance between having ggp enabled or disabled. The difference disappears in the noise. Here is a random sample:

With gas pools enabled:

transaction_type            module_working_set  executor      block_size    expected t/s    t/s    sigver/total    exe/total    block_exe/exe    commit/total     g/s    eff g/s    io g/s    exe g/s    g/t    fee/t    out B/s
------------------------  --------------------  ----------  ------------  --------------  -----  --------------  -----------  ---------------  --------------  ------  ---------  --------  ---------  -----  -------  ---------
warmup                                       1  VM                 10000             0     8811           0            0                    0           0           0          0         0          0      0        0          0
apt-fa-transfer                              1  VM                  5558         22232.7  13802           0.161        0.992                1           0.101  110418     110422     55209      55209      8        0   26555656
account-generation                           1  VM                  4512         18049.1  11524           0.131        0.993                1           0.084  103716     103716     57620      46096      9    53240   19878989
publish-package                              1  VM                   262          1051.9    407           0.017        0.999                1           0.015   49455      49455     34733      14722    122   955660   11618392
token-v2-ambassador-mint                     1  VM                  2911         11645.3   7478           0.15         0.994                1           0.067   82253      82253     44865      37388     11    78920   24032959

without gas pools:

transaction_type            module_working_set  executor      block_size    expected t/s    t/s    sigver/total    exe/total    block_exe/exe    commit/total     g/s    eff g/s    io g/s    exe g/s    g/t    fee/t    out B/s
------------------------  --------------------  ----------  ------------  --------------  -----  --------------  -----------  ---------------  --------------  ------  ---------  --------  ---------  -----  -------  ---------
warmup                                       1  VM                 10000             0     9123           0            0                    0           0           0          0         0          0      0        0          0
apt-fa-transfer                              1  VM                  5558         22232.7  13699           0.156        0.99                 1           0.104  109589     109592     54794      54794      8        0   27986287
account-generation                           1  VM                  4512         18049.1  11693           0.136        0.993                1           0.086  105233     105233     58463      46770      9    53240   21561049
publish-package                              1  VM                   262          1051.9    400           0.017        0.999                1           0.015   48745      48745     34179      14566    122   953055   11541278
token-v2-ambassador-mint                     1  VM                  2911         11645.3   7745           0.153        0.994                1           0.067   85190      85190     46467      38723     11    78920   25812686

Key Areas to Review

• Ensure the spec for governed gas pool and

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
• Move Compiler
• Other (specify)

[ganymedio]

movement move prove --filter governed_gas_pool fails on this branch for me... I added some fixes locally in governed_gas_pool.spec.move to make it pass. Sharing in case you want to integrate any of these into this PR:

1. deposit / deposit_gas_fee_v2: signer::address_of(governed_gas_signer()) fails boogie compilation (undeclared function create_signer) — the resource-account signer can't be modeled. Read the address from the capability instead:

   let pool = global<GovernedGasPool>(@aptos_framework).signer_capability.account;

2. deposit_gas_fee_v2: the postconditions only hold on the coin path (with OPERATIONS_DEFAULT_TO_FA_APT_STORE the fee moves through the fungible store, and coin::withdraw_from draws from the paired FA store under COIN_TO_FUNGIBLE_ASSET_MIGRATION). Add:

   requires !features::spec_is_enabled(features::OPERATIONS_DEFAULT_TO_FA_APT_STORE);
   requires global<coin::CoinStore<AptosCoin>>(gas_payer).coin.value >= gas_fee;

3. init_module: add a spec so initialize's framework-signer precondition can be discharged:

   spec init_module(aptos_framework: &signer) {
       requires system_addresses::is_aptos_framework_address(signer::address_of(aptos_framework));
   }

4. deposit_from / deposit_treasury: the pool only has a CoinStore when NEW_ACCOUNTS_DEFAULT_TO_FA_APT_STORE is off (register_fa_and_apt), so deposit's requires exists<coin::CoinStore>(pool) can't be discharged at the call sites. Propagate it to both callers:

   spec deposit_from<CoinType>(account: address, amount: u64) {
       pragma aborts_if_is_partial = true;
       let pool = global<GovernedGasPool>(@aptos_framework).signer_capability.account;
       requires exists<coin::CoinStore<CoinType>>(pool);
   }
   spec deposit_treasury(treasury_account: &signer, amount: u64) {
       pragma aborts_if_is_partial = true;
       let pool = global<GovernedGasPool>(@aptos_framework).signer_capability.account;
       requires exists<coin::CoinStore<AptosCoin>>(pool);
       requires exists<GovernedGasPoolExtension>(@aptos_framework);
   }

5. fund: drop the aborts_with line — its listed codes don't cover the FA-migration overflow path in coin::withdraw. (Pre-existing; unrelated to this PR's changes, only surfaces once the rest passes.)

With these, movement move prove --filter governed_gas_pool returns Success.

[seanyoung]

Thank you very much @ganymedio . I've added this in a commit.

[areshand]

One thing I want to clarify before this merges: governed_gas_pool::initialize now calls upgrade_pool_store_to_concurrent(...) unconditionally in both the existing-pool and newly-created-pool branches.

From this module, there does not appear to be an explicit features::concurrent_fungible_balance_enabled() guard before the upgrade. The local helper just resolves the primary store and calls fungible_asset::upgrade_store_to_concurrent(...), so the feature check/abort is delegated to the lower-level fungible-asset helper.

If CONCURRENT_FUNGIBLE_BALANCE is disabled when this initializer runs, this looks like it can abort rather than skip or fall back. Can you either add an explicit guard/fallback here, or document/prove the rollout ordering that guarantees CONCURRENT_FUNGIBLE_BALANCE is enabled before this path can execute?

[seanyoung]

If CONCURRENT_FUNGIBLE_BALANCE is disabled when this initializer runs, this looks like it can abort rather than skip or fall back. Can you either add an explicit guard/fallback here, or document/prove the rollout ordering that guarantees CONCURRENT_FUNGIBLE_BALANCE is enabled before this path can execute?

Good point. Working on it.

[seanyoung]

One thing I want to clarify before this merges: governed_gas_pool::initialize now calls upgrade_pool_store_to_concurrent(...) unconditionally in both the existing-pool and newly-created-pool branches.

From this module, there does not appear to be an explicit features::concurrent_fungible_balance_enabled() guard before the upgrade. The local helper just resolves the primary store and calls fungible_asset::upgrade_store_to_concurrent(...), so the feature check/abort is delegated to the lower-level fungible-asset helper.

If CONCURRENT_FUNGIBLE_BALANCE is disabled when this initializer runs, this looks like it can abort rather than skip or fall back. Can you either add an explicit guard/fallback here, or document/prove the rollout ordering that guarantees CONCURRENT_FUNGIBLE_BALANCE is enabled before this path can execute?

I don't think we want to enable governed gas pools until CONCURRENT_FUNGIBLE_BALANCE is enabled, due to the performance issue. So, it might be better to make this a documentation issue.

What would be the best place to record this?

[areshand]

til CONCURRENT_FUNGIBLE_BALANCE is enabled, due to the performance issue. So, it might be better to make this a documentation issue.

What would be the best place to record this?

we already have govern gas pool enabled both on testnet and mainnet. The usual workflow is feature gate this change, deploy to testnet and mainnet and later once coin2FA migration is done, we turn on the feature flag

[seanyoung]

we already have govern gas pool enabled both on testnet and mainnet. The usual workflow is feature gate this change, deploy to testnet and mainnet and later once coin2FA migration is done, we turn on the feature flag

Ah I did not realise governed gas pool is already enabled. I can see that now. Yes, this needs some rework.

[0xIcarus]

Security / Correctness Review

Focused on changes introduced in this PR only. Found one confirmed high-severity logic bug, one medium-severity defensive gap, and a few nits.

---

HIGH — Storage-refund minting is no longer gated by governed_gas_pool_enabled()

See inline comment below for the exact diff line. This is the most important finding.

---

MEDIUM — on_reconfig() has no existence guard for GovernedGasPool

// reconfiguration.move:137
governed_gas_pool::on_reconfig();

on_reconfig() calls governed_gas_signer() which does borrow_global<GovernedGasPool>(@aptos_framework). If for any reason GovernedGasPool doesn't exist (e.g. fresh chain before init_module completes, or any test environment with partial setup) this aborts inside reconfiguration::reconfigure(), stalling epoch changes. On live networks where GGP is already deployed this is safe today, but the missing defensive guard is a landmine.

Fix:

public(friend) fun on_reconfig() acquires GovernedGasPool {
    if (exists<GovernedGasPool>(@aptos_framework)) {
        upgrade_pool_store_to_concurrent(&governed_gas_signer());
    }
}

---

LOW — upgrade_store_to_concurrent called on every epoch change

upgrade_pool_store_to_concurrent does a storage read (exists<ConcurrentFungibleBalance>) on every epoch boundary once concurrent_fungible_balance_enabled() is true. The function is idempotent so it never aborts, but the read is wasted after the first upgrade. Minor inefficiency in a hot path.

---

NIT — Misleading comment on withdrawal reordering in withdraw_staking_reward

The comment says the reordering prevents an insufficient-balance abort, but the assert!(balance >= amount, ...) check immediately before already handles that. The real value is reducing retry overhead in concurrent speculative execution. Worth saying that instead.

---

CONFIRMED CORRECT

• deposit_gas_fee_v2 zero-check: correct, the coin path previously had no 0 guard.
• upgrade_store_to_concurrent idempotency: confirmed via fungible_asset.move:1374 — returns early if ConcurrentFungibleBalance already exists.
• initialize_governed_gas_pool_extension refactor: semantically identical, just cleaner.

[0xIcarus]

BUG (HIGH): Storage-refund minting no longer gated by governed_gas_pool_enabled()

The original epilogue_gas_payer path had an explicit guard:

// line ~633 (old code, still present in epilogue_gas_payer)
if (!features::governed_gas_pool_enabled()){
    transaction_fee::mint_and_refund(gas_payer, mint_amount);
}

The TODO comment beside it explained why: "we cannot mint to do storage refund. We need to have a storage refund pool."

unified_epilogue_v2 replaces that guard with:

if (mint_amount > 0) {
    transaction_fee::mint_and_refund(gas_payer_address, mint_amount);
}

Impact: On mainnet and testnet — where GOVERNED_GAS_POOL is already enabled — every transaction where storage_fee_refunded > transaction_fee_amount now mints APT out of thin air. There is no corresponding debit from the governed gas pool or any other source. This is an unbounded inflationary write that bypasses the storage-refund pool design that was explicitly left as a TODO.

Verified with a Move unit test (added to transaction_validation.move for review, not intended to merge as-is):

#[test(aptos_framework = @aptos_framework)]
fun test_unified_epilogue_v2_mints_apt_when_ggp_enabled_bug(aptos_framework: &signer) {
    account::create_account_for_test(@aptos_framework);
    let (burn_cap, mint_cap) = aptos_coin::initialize_for_test(aptos_framework);
    transaction_fee::store_aptos_coin_mint_cap(aptos_framework, mint_cap);

    // Enable GGP — live state on mainnet and testnet
    features::change_feature_flags_for_testing(
        aptos_framework,
        vector[features::get_governed_gas_pool_feature()],
        vector[],
    );

    let gas_payer_addr = @0xbeefdead;
    account::create_account_for_test(gas_payer_addr);
    coin::register<AptosCoin>(&account::create_signer_for_test(gas_payer_addr));

    let balance_before = coin::balance<AptosCoin>(gas_payer_addr);

    // transaction_fee_amount = 0 * 0 = 0, mint_amount = 500
    unified_epilogue_v2(
        account::create_signer_for_test(gas_payer_addr),
        account::create_signer_for_test(gas_payer_addr),
        500, 0, 0, 0, false, true,
    );

    // PASSES — confirms the bug: 500 octas minted despite GGP being enabled
    assert!(coin::balance<AptosCoin>(gas_payer_addr) == balance_before + 500, 1);
    coin::destroy_burn_cap(burn_cap);
}

Test output:

[ PASS ] 0x1::transaction_validation::test_unified_epilogue_v2_mints_apt_when_ggp_enabled_bug

Fix: Restore the guard, or implement the storage-refund pool and debit it here. The easiest safe fix is:

// Only mint the storage refund when GGP is not active.
// When GGP is enabled there is no storage-refund pool yet (see TODO above),
// so minting here would inflate supply without a corresponding pool debit.
if (mint_amount > 0 && !features::governed_gas_pool_enabled()) {
    transaction_fee::mint_and_refund(gas_payer_address, mint_amount);
}

[seanyoung]

On storage refunds:

if (mint_amount > 0) {
    transaction_fee::mint_and_refund(gas_payer_address, mint_amount);
}

1. Storage refunds are not enabled on mainnet, feature STORAGE_DELETION_REFUND so this code path never executed

2. The original code was this:

if (!features::governed_gas_pool_enabled()){
    transaction_fee::mint_and_refund(gas_payer, mint_amount);
}

that is not correct either because with gas pools enabled, the refund simply does not occur.

Should the refund go to the gas pool instead? I don't know what the right answer is here, but this code is not executed so the decisions does not matter.