[FVM] Fail fast when payer cannot pay #3473
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3473 +/- ##
==========================================
+ Coverage 55.32% 59.83% +4.51%
==========================================
Files 768 152 -616
Lines 69922 17223 -52699
==========================================
- Hits 38683 10306 -28377
+ Misses 28055 6065 -21990
+ Partials 3184 852 -2332
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
FVM Benchstat comparisonThis branch with compared with the base branch onflow:master commit 0459d16 The command Collapsed results for better readability
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I am late to the party, but why do we need Considering when I have <MAB, any transaction will fail, and I will end up with <MAB; isn't it enough check to filter transactions and reject? Why do we need to cast the net wider here? |
janezpodhostnik
force-pushed
the
janez/fail-fast-payer-cannot-pay
branch
from
97f40a0
to
37c3546
Compare
janezpodhostnik
force-pushed
the
janez/fail-fast-payer-cannot-pay
branch
from
37c3546
to
c24c279
Compare
|
thanks @janezpodhostnik, but I fail to see the benefit here; probably I am missing something: Now; in the case of MAB, limit is constant. But when we introduce limit as a function of gas_limit ( MAB + cost(msx_gas_limit) ) we only cover very limited cases ( which are not important ) but generate extra complication. Considering my account has MAB + delta ( where delta is a small amount ) of balance, It is in a state of unknown, isn't it easier to cache without this delta and calculation? If we have an account that has < MAB, we can cache the result, and even skip the MAB check ( until balance > MAB ), but when we have MAB + delta calculation, we are losing that possibility. |
|
That is a good argument! I will bring that up with the team. This change would go into the smart contract anyway, so it doesn't affect this PR in particular, but the feature in general. |
janezpodhostnik
force-pushed
the
janez/fail-fast-payer-cannot-pay
branch
2 times, most recently
from
58d3e8e
to
fc1eb83
Compare
| inclusionEffort uint64, | ||
| maxExecutionEffort uint64, | ||
| ) (cadence.Value, error) { | ||
| return sys.Invoke( |
There was a problem hiding this comment.
do you have profiling info about how much this extra overhead will be? wondering if it can be less than deductFees & checkStorage cadence calls.
|
i'm thinking out loud: would it be helpful if Cadence can provide FVM more info like, if there's any withdraw in the contract, and pass it to FVM as a flag, so that FVM will have more info to make decisions like this. |
It is very good idea, but this being a good idea makes implementing it a bad idea ( in a way ) If this is a need ( and I agree it is ) solution is not for cadence to provide FVM information but instead FVM should take responsibility for storage (atree and friends ) ( as we are resource-aware blockchain ) and it should provide storage functions to Cadence. ( I know it is easier said than done, e.g. It is not so easy with metering etc ) |
janezpodhostnik
force-pushed
the
janez/fail-fast-payer-cannot-pay
branch
4 times, most recently
from
383a5f7
to
55d69c8
Compare
janezpodhostnik
requested review from
gomisha,
yhassanzadeh13,
ramtinms,
pattyshack and
m4ksio
as code owners
| uint64(txnState.TotalComputationLimit()), | ||
| ) | ||
| if err != nil { | ||
| return 0, errors.NewPayerBalanceCheckError(proc.Transaction.Payer, err) |
There was a problem hiding this comment.
NewPayerBalanceCheckError should be a failure instead of error?
There was a problem hiding this comment.
I'm not sure This should be a failure, but I don't have a good argument against it.
Let me create a separate PR for making this a failure, so I can move this one forward.
fvm/transactionInvoker.go
Outdated
| @@ -275,6 +276,21 @@ func (executor *transactionExecutor) normalExecution() ( | |||
| modifiedSets programsCache.ModifiedSetsInvalidator, | |||
| err error, | |||
| ) { | |||
| executor.txnState.RunWithAllLimitsDisabled(func() { | |||
There was a problem hiding this comment.
btw, should this check be done in ExecuteTransactionBody, before getBodyMeterParameters?
There was a problem hiding this comment.
o ... nevermind. i need to move getBodyMeterParameters into preprocessing, which would defeat this point.
janezpodhostnik
force-pushed
the
janez/fail-fast-payer-cannot-pay
branch
from
55d69c8
to
5355125
Compare
janezpodhostnik
force-pushed
the
janez/fail-fast-payer-cannot-pay
branch
from
5355125
to
c488364
Compare
|
bors merge |
This is the first part of #2767
Payer must have sufficient balance for the transaction execution to happen.
The required balance is defined by the smart contract.
related to: onflow/flow-emulator#231 and onflow/flow-core-contracts#318