paritytech / wasm-utils

Apache License 2.0
82 stars 46 forks source link

Gas metering does not handle branches and external calls #120

Open jimpo opened 5 years ago

jimpo commented 5 years ago

The gas metering code currently inserts metering instructions at the beginning of block, if, loop statements but does not correctly handle code that uses br_* to jump to a label. For example, in the following program, gas is charged for instructions after the br_if that are skipped.

(module
    (func $fibonacci_with_break (result i32)
        (local $x i32) (local $y i32)

        (block $unrolled_loop
            (set_local $x (i32.const 0))
            (set_local $y (i32.const 1))

            get_local $x
            get_local $y
            tee_local $x
            i32.add
            set_local $y

            i32.const 1
            br_if $unrolled_loop

            get_local $x
            get_local $y
            tee_local $x
            i32.add
            set_local $y
        )

        get_local $y
    )
)

There may also be imported functions that affect control flow or have more strict requirements on the state of the gas meter. In the Substrate srml-contracts module for example, the imported functions such ext_return and ext_gas_left need more delicate handling. ext_return affects control flow and currently gas is over-deducted if there are any unreachable instructions after the call. In the case of ext_gas_left the caller may see a lower result than expected (though this arguably is not a bug).

lexfrl commented 5 years ago

I don't see how to (statically) know in advance the condition if br_if $unrolled_loop is active in the example..

jimpo commented 5 years ago

That's kind of the point. In general that will be impossible to determine, so the correct behavior should be that gas is charged for the instructions between the top of the block and the br_if, then there should be separate gas metering instructions immediately after the br_if accounting for the instructions until the end of the block.

lexfrl commented 5 years ago

yeah, makes sense