-
Notifications
You must be signed in to change notification settings - Fork 20
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
Governed Gas Pool Spec #118
base: l-monninger/governed-gas-pool
Are you sure you want to change the base?
Governed Gas Pool Spec #118
Conversation
spec aptos_framework::governed_gas_pool { | ||
/// <high-level-req> | ||
/// No.: 1 | ||
/// Requirement: The GovernedGasPool resource must exist at the aptos_framework address after initialization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
/// Enforcement: Formally verified via [high-level-req-1](initialize). | ||
/// | ||
/// No.: 2 | ||
/// Requirement: Only the aptos_framework address is allowed to initialize the GovernedGasPool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
/// Enforcement: Formally verified via [high-level-req-2](initialize). | ||
/// | ||
/// No.: 3 | ||
/// Requirement: Deposits into the GovernedGasPool must be reflected in the pool's balance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say you want to prove above that initialization creates a resource account distinct from the aptos_framework
address. Then deposits into the GovernedGasPool
are reflected in that account's balance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I try to to do ensures
with anything resource account related I get the error noted down in the PR description.
/// Enforcement: Formally verified via [high-level-req-3](deposit), [high-level-req-3.1](deposit_from). | ||
/// | ||
/// No.: 4 | ||
/// Requirement: Only the aptos_framework address can fund accounts from the GovernedGasPool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
aptos-move/framework/aptos-framework/sources/governed_gas_pool.spec.move
Outdated
Show resolved
Hide resolved
spec module { | ||
/// [high-level-req-1] | ||
/// The GovernedGasPool resource must exist at aptos_framework after initialization. | ||
invariant exists<GovernedGasPool>(@aptos_framework); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GovernGasPool
exists for the aptos_framework
cannot be an invariant. You would need an exists implies, i.e., ==>
. One thing the GovernedGasPool
existing could imply is that the resource account for the GovernedGasPool
exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's passing though, I think because of
spec initialize(aptos_framework: &signer, delegation_pool_creation_seed: vector<u8>) {
requires system_addresses::is_aptos_framework_address(signer::address_of(aptos_framework));
/// [high-level-req-1]
ensures exists<GovernedGasPool>(@aptos_framework);
}
```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove it if you we prefer. I suppose we would need to be doing an init_module
for it to be an invariant.
@0xmovses do you have any of the |
@l-monninger Yes the changes for |
@@ -285,8 +290,8 @@ spec aptos_framework::transaction_validation { | |||
// let post balance = global<coin::CoinStore<AptosCoin>>(gas_payer).coin.value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the critical bit to validate. But, you'll notice Aptos has not made the effort to prove that herein since the FA migration began and instead would do it in transaction_fee.spec.move
This is what I was commenting on here: #114 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@franck44 Can you comment here on the difference in approaches used in transaction_validation.spec.move
and transaction_fee.spec.move
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I had done this earlier:
public fun coin_to_fungible_asset_migration_feature_enabled(): bool {
false
}
Which if I remember correctly, made this line pass. But with the feature enabled, I am not able to make ensures on coin
, because the migration paths are hit at run time.
I though it better to keep that feature in. We could decide not to enable this feature, then we would have more verification.
@@ -298,8 +303,8 @@ spec aptos_framework::transaction_validation { | |||
// aborts_if pre_balance < transaction_fee_amount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@0xmovses So, you're saying that this all should pass if I hardcode as suggested here?: https://github.com/movementlabsxyz/aptos-core/pull/118/files/8c8ce6c47e48a0cd786f000cdd7714b2671182db#r1907843043
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the feature disabled like this:
public fun coin_to_fungible_asset_migration_feature_enabled(): bool {
false
//is_enabled(COIN_TO_FUNGIBLE_ASSET_MIGRATION)
}
then the L293 & 294 for transaction_validation.spec.move
pass :
let pre_governed_gas_pool_balance = global<coin::CoinStore<AptosCoin>>(governed_gas_pool_address()).coin.value;
let post governed_gas_pool_balance = global<coin::CoinStore<AptosCoin>>(governed_gas_pool_address()).coin.value;
aptos move prove -f aptos move prove -f transaction_validation
Furthermore more can be verified within governed_gas_pool.spec.move
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to add and these two lines, to comment out, these ensures pass
ensures governed_gas_pool_balance == pre_governed_gas_pool_balance + transaction_fee_amount;
ensures account.sequence_number == pre_account.sequence_number + 1;
Description
governed_gas_pool.spec.move
transaction_validation.spec.move
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Yes
aptos move prove -f governed_gas_pool && aptos move prove -f transaction_validation
Key Areas to Review
IMPORTANT
When the feature
COIN_TO_FUNGIBLE_ASSET_MIGRATION
is enabled, this causes thespec fund
ingoverned_gas_pool.spec.move
to fail. So, I disabled it and it passes as expected.I would advise us to disable this feature for mainnet as it seems strange to enable as we are not migrating to FA yet. However, if we do want to go to mainnet with this feature enabled, then I would imagine something is wrong with the spec.
NOTE
When trying to call
governed_gas_pool_address()
within the governed_gas_pool spec I get the following error@franck44 mentions this is a problem to do with versioning and configuration. I tried it with both
aptos move prove
andmovement move prove
, but it yields the same result. From the error message it seems that Boogie doesn't have a declaration for create_signer at runtime. So makingensures
for resource accounts seems tricky.In any case the changes offered here offer some value toward full verification.