deposit in prologue and refund in epilogue to prevent undergasing attack for randomness txns #12695
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
⏱️ 26h 7m total CI duration on this PR🚨 3 jobs on the last run were significantly faster/slower than expected
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12695 +/- ##
=========================================
- Coverage 62.6% 62.6% -0.1%
=========================================
Files 823 820 -3
Lines 184343 184003 -340
=========================================
- Hits 115499 115251 -248
+ Misses 68844 68752 -92 ☔ View full report in Codecov by Sentry. |
msmouse
reviewed
Mar 27, 2024
aptos-move/framework/aptos-framework/sources/transaction_validation.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/aptos-framework/sources/transaction_validation.spec.move
Outdated
Show resolved
Hide resolved
| @@ -294,11 +423,15 @@ module aptos_framework::transaction_validation { | |||
| transaction_fee_amount | |||
| }; | |||
|
|
|||
| if (amount_to_burn > storage_fee_refunded) { | |||
| let burn_amount = amount_to_burn - storage_fee_refunded; | |||
| let refund = storage_fee_refunded; | |||
There was a problem hiding this comment.
assertion on line 407 seems not proper any more
also, line 417 is probably broken
There was a problem hiding this comment.
why? i thought the only change to the epilogue is to mint back?
There was a problem hiding this comment.
We should consider the burnt amount when judging if the sender has enough fund to cover the gas?
There was a problem hiding this comment.
so randomness refund should happen earilier, somewhere before L407?
There was a problem hiding this comment.
I think you should do the math with the refund accounted for and still either mint or refund once.
8f87bfb
to
539b358
Compare
|
So now that I had more time to think about this, why aren't we just making this the status quo for all transactions? |
aptos-move/framework/aptos-framework/sources/transaction_validation.move
Outdated
Show resolved
Hide resolved
aptos-move/framework/aptos-framework/sources/transaction_validation.move
Outdated
Show resolved
Hide resolved
| @@ -36,10 +36,14 @@ pub static APTOS_TRANSACTION_VALIDATION: Lazy<TransactionValidation> = | |||
| module_addr: CORE_CODE_ADDRESS, | |||
| module_name: Identifier::new("transaction_validation").unwrap(), | |||
| fee_payer_prologue_name: Identifier::new("fee_payer_script_prologue").unwrap(), | |||
| fee_payer_prologue_v2_name: Identifier::new("fee_payer_script_prologue_v2").unwrap(), | |||
There was a problem hiding this comment.
We can upgrade private functions iirc, so this code can be avoided
There was a problem hiding this comment.
if we update the function arg list inline, then the rust side needs an additional feature flag to know when to include the required_deposit arg, which i thought is worse than defining a func v2 + using v2 only when required_deposit.is_some (so no additional feature flag is needed)?
cc @zekun000
There was a problem hiding this comment.
I thought we need a flag anyway, we can't use required_deposit to decide if calling v1 or v2 because the framework may not be updated yet
aptos-move/aptos-vm/src/aptos_vm.rs
Outdated
| | TransactionPayload::ModuleBundle(_) | ||
| | TransactionPayload::Multisig(_) => false, | ||
| TransactionPayload::EntryFunction(entry_func) => { | ||
| has_randomness_attribute(resolver, session, entry_func).unwrap_or(false) |
There was a problem hiding this comment.
This should propagate the error, which can happen if module loading failed
There was a problem hiding this comment.
(i'm turning this func into get_required_deposit)
can this function focus on required deposit calculation and let a later step capture the module loading failure?
aptos-move/aptos-vm/src/aptos_vm.rs
Outdated
| @@ -2407,12 +2445,15 @@ impl VMValidator for AptosVM { | |||
| let mut session = self.new_session(&resolver, SessionId::prologue_meta(&txn_data)); | |||
|
|
|||
| // Increment the counter for transactions verified. | |||
| let has_randomness_annotation = | |||
| check_randomness_annotation(&mut session, &resolver, txn.payload()); | |||
There was a problem hiding this comment.
When you do let txn_data = TransactionMetadata::new(&txn); few lines above you can probably also add a field there for has_randomness_annotation? Then no need to pass it around. Also calling more generally makes more sense if we want to switch to this flow at some point anyway?
There was a problem hiding this comment.
we now have required_deposit in TransactionMetadata.
zekun000
reviewed
Mar 29, 2024
| @@ -136,9 +162,36 @@ module aptos_framework::transaction_validation { | |||
| ); | |||
| let balance = coin::balance<AptosCoin>(gas_payer); | |||
| assert!(balance >= max_transaction_fee, error::invalid_argument(PROLOGUE_ECANT_PAY_GAS_DEPOSIT)); | |||
|
|
|||
| if (option::is_some(&required_deposit)) { | |||
| transaction_fee::burn_fee(gas_payer, option::extract(&mut required_deposit)); | |||
There was a problem hiding this comment.
we need a new error code if the max_transaction_fee != required_deposit, otherwise the abort will be Unexpected prologue Move abort https://github.com/aptos-labs/aptos-core/blob/main/aptos-move/aptos-vm/src/errors.rs#L64
There was a problem hiding this comment.
how does the comparison between max_transaction_fee and required_deposit matter?
did you mean a new error code for when there's not enough balance to deposit?
| @@ -104,7 +112,15 @@ pub(crate) fn run_script_prologue( | |||
| MoveValue::U64(txn_expiration_timestamp_secs), | |||
| MoveValue::U8(chain_id.id()), | |||
| ]; | |||
| (&APTOS_TRANSACTION_VALIDATION.fee_payer_prologue_name, args) | |||
| if required_deposit.is_some() { | |||
| args.push(required_deposit.as_move_value()); | |||
There was a problem hiding this comment.
I don't think this works since the binary and framework are not updated at the same time, we need to handle the case that the framework is not updated
There was a problem hiding this comment.
fixed by gating randomness deposit calculation with randomness feature flags (and in the future. feature X deposit calculation with X feature flags)
?
| @@ -189,40 +210,65 @@ fn run_epilogue( | |||
| fee_statement: FeeStatement, | |||
| txn_data: &TransactionMetadata, | |||
| features: &Features, | |||
| required_deposit: Option<u64>, | |||
There was a problem hiding this comment.
nit: maybe we can merge this into TransactionMetadata so we don't need to pass the extra param all over the place
There was a problem hiding this comment.
we now have required_deposit in TransactionMetadata.
zjma
commented
Mar 30, 2024
aptos-move/aptos-vm/src/aptos_vm.rs
Outdated
| let internal_gas_per_gas = u64::from(txn_gas_params.scaling_factor()); | ||
| let max_execution_gas = Gas::from( | ||
| u64::from(txn_gas_params.max_execution_gas).div_ceil(internal_gas_per_gas), | ||
| ); |
There was a problem hiding this comment.
@vgao1996 is there a better way to convert InternalGas to Gas?
There was a problem hiding this comment.
maybe just
let max_execution_gas: Gas = txn_gas_params
.max_execution_gas
.to_unit_round_up_with_params(txn_gas_params);
zekun000
reviewed
Mar 30, 2024
aptos-move/aptos-vm/src/aptos_vm.rs
Outdated
| let internal_gas_per_gas = u64::from(txn_gas_params.scaling_factor()); | ||
| let max_execution_gas = Gas::from( | ||
| u64::from(txn_gas_params.max_execution_gas).div_ceil(internal_gas_per_gas), | ||
| ); |
There was a problem hiding this comment.
maybe just
let max_execution_gas: Gas = txn_gas_params
.max_execution_gas
.to_unit_round_up_with_params(txn_gas_params);
zjma
commented
Apr 1, 2024
aptos-move/aptos-vm/src/aptos_vm.rs
Outdated
| txn.payload(), | ||
| ) | ||
| } else { | ||
| return VMValidatorResult::error(StatusCode::UNKNOWN_VALIDATION_STATUS); |
There was a problem hiding this comment.
is this error code fine?
There was a problem hiding this comment.
zjma
commented
Apr 1, 2024
aptos-move/framework/aptos-framework/sources/transaction_validation.spec.move
Outdated
Show resolved
Hide resolved
a69d8a5
to
cf9ebc5
Compare
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
❌ Forge suite
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
min(A, B), where:A = (max_execution_gas + max_io_gas) * price_per_gas_unit + max_storage_fee, andB = maximum_number_of_gas_units * price_per_gas_unit.Some reference numbers on mainnet.
Therefore, for a txn that directly or indirectly call randomness API, the required deposit is 2_000_000 octas, i.e., 2APT.
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
UTs
Key Areas to Review
function
execute_user_transaction_impl.Checklist