go-spacemesh icon indicating copy to clipboard operation
go-spacemesh copied to clipboard

vm: transactions that can't cover max_gas

Open dshulyak opened this issue 1 year ago • 7 comments

dshulyak avatar Jul 29 '22 15:07 dshulyak

@lrettig

dshulyak avatar Jul 29 '22 15:07 dshulyak

See discussion thread in #3272

@dshulyak I'll fill in this task on the basis of my discussion with @noamnelke and @tal-m

lrettig avatar Jul 29 '22 19:07 lrettig

We discussed this today and decided that, for this class of transaction, we should:

  1. run verify
  2. (do not even attempt execution)
  3. charge both intrinsic gas and as much of max-gas as possible (up to the full principal account balance), i.e., the tx should pay as much as it can
  4. consume the nonce

lrettig avatar Aug 08 '22 19:08 lrettig

@lrettig without trying to execute it?

dshulyak avatar Aug 09 '22 03:08 dshulyak

@lrettig without trying to execute it?

There's no point in trying since (at genesis, with the fixed-gas regime) we know it will fail. Note that this is an optimization and in future when we enable variable gas, we will attempt execution.

And we are assuming here that max-gas is set to precisely the fixed-gas requirement. To be explicit I think the logic should be

if MIN(fixed-gas-required-by-template, max-gas-provided-by-tx) > principal-remaining-balance then consume(principal-remaining-balance)
else if max-gas-provided-by-tx > fixed-gas-required-by-template then consume(fixed-gas-required-by-template)
else consume(max-gas-provided-by-tx)

lrettig avatar Aug 09 '22 18:08 lrettig

is it true for multisig (self)spawn transactions? N is a variable defined specifically for spawn. as i understand it actual gas will be charged based on actual N (for example 5), but max_gas has to be precomputed based on max N (10)

dshulyak avatar Aug 10 '22 07:08 dshulyak

I think this is part of Wojtek's spec, @selfdual-brain would you mind weighing in here?

lrettig avatar Aug 10 '22 17:08 lrettig

The formula provided by Lane seems to be wrong.

Let me clean-up the notation first:

  • tx is the transaction in question
  • tx.maxGas is the max-gas calculated for this transaction by template-level method parsePayload()
  • tx.principal is the principal address of tx
  • tx.template is the template of tx.principal
  • tx.method is the method selector sealed into tx
  • GlobalState.gasPurseBalanceOf(address) - the GasPurse balance for given account
  • COMPUTATION_COST(tx.template, tx.method) - is referring to a static table of computation cost (this table is a parameter of the protocol)
  • STORAGE_COST_FACTOR: Int - is a parameter of the protocol

Beginning of the pseudo-code is:

val fixedGas = COMPUTATION_COST(tx.template, tx.method)
val maxGas = fixedGas + tx.binarySize * STORAGE_COST_FACTOR 
val balance = GlobalState.gasPurseBalanceOf(tx.principal) 

Now, let me rewrite what Lane wrote above but this time using my new notation:

if (min(fixedGas, maxGas) > balance)
    consume(balance)
else {
    if (maxGas > fixedGas)
        consume(fixedGas)
    else
        consume(maxGas)
}

To understand why this formula is wrong, let me simplify it first. Please observe that because of the way we calculate maxGas, expression maxGas >= fixedGas is always true. Hence: min(fixedGas, maxGas) can be replaced by fixedGas.

The simplified Lane formula looks then:

if (balance < fixedGas)
    consume(balance)
else {
    if (fixedGas != maxGas)
        consume(fixedGas)
    else
        consume(maxGas)
}

This formula is wrong because it checks the actual balance only once. Now, let me derive the correct formula.

In general we have 2 numbers: fixedGas and maxGas and we know that fixedGas <= maxGas. In most general situation these numbers partition the space of non-negative integers into 3 intervals:

  • [0, fixedGas)
  • [fixedGas, maxGas)
  • [maxGas, Integer.MAXVALUE]

Caution: notice how I am using round and square brackets to deal with the ends of intervals.

Required behaviour of gas consumption depends on finding out in which of these intervals the value balance actually landed. There are 3 cases:

  • case 1: balance landed in [0, fixedGas) - this means the balance was not sufficient to cover fixedGas
  • case 2: balance landed in [fixedGas, maxGas) - this means the balance was sufficient to cover fixedGas but insufficient to cover maxGas
  • case 3: balance landed in [maxGas, Integer.MAXVALUE] - this means the balance was sufficient

The agreed behaviour of our system is:

  • case 1: consume the whole balance (and apply the nonce); the execution result of this tx is "insufficient balance"
  • case 2: same as in case 1
  • case 3: consume maxGas (and apply the nonce); this is a successful execution of tx

In pseudo-code it looks like this:

if (balance < maxGas) {
    consume(balance)
    applyNonce(tx.nonce)
    registerResult(tx, ERROR_INSUFFICIENT_BALANCE)
} else {
    consume(maxGas)
    applyNonce(tx.nonce)
    registerResult(tx, SUCCESS)
}

The reason of long discussions around this topic was that there is another possible behaviour, which looks pretty resonable. Namely this one:

  • case 1: consume the whole balance (and apply the nonce); the execution result of this tx is "insufficient balance"
  • case 2: consume only fixedGas (and apply the nonce); the execution result of this tx is "insufficient balance"
  • case 3: consume maxGas (and apply the nonce); this is a successful execution of tx

This variant was finally dropped, mostly because of arguments provided by @tal-m.

selfdual-brain avatar Aug 11 '22 13:08 selfdual-brain

@selfdual-brain, thanks for the clarifications. Just two additional points:

val fixedGas = COMPUTATION_COST(tx.template, tx.method) val maxGas = fixedGas + tx.binarySize * STORAGE_COST_FACTOR because of the way we calculate maxGas, expression maxGas >= fixedGas is always true. Hence: min(fixedGas, maxGas) can be replaced by fixedGas

Your revised spec assumes both a fixed-gas regime, and that we control the behavior of all templates. My original proposed was intended to be more generic and address the future case where we allow user-deployed templates, in which case we cannot guarantee how a template chooses to calculate maxGas (and in particular it could be less than fixedGas or whatever intrinsic gas cost is calculated in a variable-gas regime). Your clarifications are of course perfectly reasonable as an optimization/simplification for genesis.

val fixedGas = COMPUTATION_COST(tx.template, tx.method) val maxGas = fixedGas + tx.binarySize * STORAGE_COST_FACTOR

One further point regarding these values. You're still missing at least one value here, which is a fixed per-tx gas cost which covers not only the "computation cost" and storage cost, but also the cost of verify(), the "consensus cost", the bandwidth cost of transmitting the tx, etc. -- unless you intended for all of this to be included in COMPUTATION_COST (it would be helpful to see pseudocode for this as well).

For reference please see these snippets from go-ethereum:

  • https://github.com/ethereum/go-ethereum/blob/a9ef135e2dd53682d106c6a2aede9187026cc1de/core/state_transition.go#L117-L156
  • https://github.com/ethereum/go-ethereum/blob/a9ef135e2dd53682d106c6a2aede9187026cc1de/core/state_transition.go#L192-L363

lrettig avatar Aug 12 '22 17:08 lrettig

COMPUTATION_COST table is the approach for Genesis only. Values in this table are fixed costs which cover:

  • consensus cost
  • verify() cost
  • method execution

selfdual-brain avatar Aug 17 '22 13:08 selfdual-brain

Gotcha. But I think it's important to have a notion of intrinsic gas for all transactions, and for all time (not just for genesis).

lrettig avatar Aug 17 '22 17:08 lrettig

I agree. But I assumed this will be decided after Genesis - ?

selfdual-brain avatar Aug 18 '22 13:08 selfdual-brain

I think we have enough information to pick a reasonable intrinsic cost function now. Whether this goes into a "table" or is calculated on the fly per tx is a question of semantics.

lrettig avatar Aug 18 '22 15:08 lrettig

i read it as we need to have intrinsic gas check for genesis, otherwise we will consume transactions that can't cover intrinsic gas. they are ineffective based on https://docs.google.com/spreadsheets/d/1PNyKl4DuA17Ku85Ajl5A1lwkE9V7EXaYHJpkRxwyQ_U/edit#gid=0

dshulyak avatar Aug 19 '22 13:08 dshulyak

@lrettig why do we even need to check if tx can cover intrinsic gas? normally transaction won't be included into the block if it can't cover max_gas. however it will be consumed if it was included and balance is insufficient (like discussed above). you say that we should not consume transactions that can't cover intrinsic gas, but when and why does it matter?

also if it matters, is it going to be the same for all transactions (e.g. protocol parameter)?

dshulyak avatar Aug 24 '22 13:08 dshulyak