[movevm] improve automatic creation of account for sponsored txn #11076

davidiw · 2023-11-25T00:08:24Z

If an account is created during sponsored transaction and the txn aborts.
The transaction is now kept but aborted.
The state associated with account creation is wiped and The account is no longer created.
During epilogue, we then hit a invariant violation, because the sequence number for the account cannot be incremented.

This ensures that we create an account even on transaction failure in the epilogue. We then charge as much as possible by recording the error on processing the change set. An error indicates we have run out of gas.

Note, the min gas amount is effectively 2, where we need roughly 4 for this operation, so more needs to be figured out. We could consider updating the simulation, but it is probably easier to just mention in the AIP this specific scenario as that minimizes further code hacking.

There are tests that validate that this works even with enough gas to execute and that it fails to execute without enough gas.

codecov · 2023-11-25T00:52:29Z

Codecov Report

Attention: 33 lines in your changes are missing coverage. Please review.

Comparison is base (0e91537) 68.7% compared to head (b003b9f) 68.7%.
Report is 3 commits behind head on main.

Files	Patch %	Lines
aptos-move/aptos-vm/src/aptos_vm.rs	86.3%	23 Missing ⚠️
aptos-move/aptos-vm/src/gas.rs	81.4%	5 Missing ⚠️
aptos-move/aptos-vm/src/transaction_validation.rs	37.5%	5 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #11076    +/-   ##
========================================
  Coverage    68.7%    68.7%            
========================================
  Files         764      764            
  Lines      176995   177145   +150     
========================================
+ Hits       121630   121819   +189     
+ Misses      55365    55326    -39

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

aptos-move/aptos-vm/src/aptos_vm.rs

zekun000 · 2023-11-28T06:29:41Z

aptos-move/aptos-vm/src/aptos_vm.rs

-            self.vm_impl
+        if matches!(transaction_status, TransactionStatus::Keep(_))
+            && txn_data.fee_payer().is_some()
+            && txn_data.sequence_number == 0


I think account sequence number starts from 0 right? so there could be a valid account with sequence number 0, in that case if the txn fails, I guess it just needs to pay the extra gas for calling create_account_if_does_not_exist

it would have to pay that in the happy path anyway...

aptos-move/aptos-vm/src/aptos_vm.rs

aptos-move/aptos-vm/src/aptos_vm_impl.rs

georgemitenkov · 2023-12-01T16:22:16Z

aptos-move/aptos-vm/src/aptos_vm.rs

+
+        // Need to zero this out, because that's the legacy behavior. Otherwise we charge for txn
+        // size where we used to not.
+        let mut null_txn_data = txn_data.clone();


Why not pass a size to charge_change_set_and_respawn_session? seems better than cloning the whole struct

hmm... because this isn't common case and it will be removed, so I like to leave the minimal foot print... I'm fine either way.

aptos-move/aptos-vm/src/aptos_vm.rs

msmouse · 2023-12-05T19:43:52Z

aptos-move/aptos-vm/src/aptos_vm.rs

+                })?;
+
+            let mut change_set = session.finish(change_set_configs)?;
+            if let Err(err) = self.charge_change_set(&mut change_set, gas_meter, txn_data) {


Shall we add a flag to not record deposits on the new slots created (so no refund for these slots)? Makes me feel safer.

why do that? if we can give them, we should

So we will never create a bug where you can get more refund then you've paid. I know you defend against that by requiring more that two slots worth of it upfront, but I'm not feeling safe -- one can accidentally remove the requirement.

thinking outloud - can we somehow make this more robust, either:

make sure that charge we are missing here is below what we already charged, or

refund what we already charged, charge here expecting no failures, and then try to charge back how much we refunded?

in the current form, it's also a bit odd we are checking whether we have enough only for the storage deposit, but not for the other costs. they might not be high by default, but might be quite high if transaction has set a high gas price for example.

either of the above should account for the extras.

if we don't have enough funds for all the costs of account creation alone, we should probably be discarding the whole transaction, instead of forcing account creation.

I am wondering if we can at least capture these cases in tests, e.g. have a test to check the refund is < what was paid? In case some checks are removed we will see something is off

To be clear, this is not just a test issue.
The cheapest way now to create an account is to have account with 2x storage slot fee, and submit a fee payer txn.
In idle state difference is like 700 octas, but if network is under load - you can freely submit txn with large gas price, get the priority, and get it executed without paying high gas price.

If nothing else, we should check balance for 2* storage + 10 * gas_price , and have test to check its enough

Assuming you landed on the idea that the validation function should test (2* storage + 10*gas_price) which I think is much, much more expensive then it is to create an account. I think that is acceptable... we should be willing to charge more for aborted transactions.

@georgemitenkov , we cannot really test this because we don't have the ability to delete an account. I might propose that we only allow those that upgrade from v1 to v2 to actual delete. In which case, there's conservation here, because we'll need to create new storage anyway.

msmouse · 2023-12-06T19:54:59Z

aptos-move/aptos-vm/src/aptos_vm_impl.rs

+            && self
+                .get_features()
+                .is_enabled(FeatureFlag::SPONSORED_AUTOMATIC_ACCOUNT_CREATION)
+            && max_gas_amount < 2 * storage_slot_cost


You are comparing gas units with octas -- see how it's dangerous?

georgemitenkov · 2023-12-12T07:50:54Z