Skip to content

Commit

Permalink
fix(avm): Do not scale CALLDATACOPY base cost with size (#5879)
Browse files Browse the repository at this point in the history
As discussed with @fcarreiro, base gas cost should only scale if we do
_something_ with the data. If we're just copying it, then the additional
memory accesses take care of reflecting the additional effort.
  • Loading branch information
spalladino authored Apr 25, 2024
1 parent 005c71c commit 99e12b1
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 7 deletions.
8 changes: 5 additions & 3 deletions docs/docs/protocol-specs/public-vm/execution.md
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,13 @@ machineState.daGasLeft = 0
### Gas cost notes and examples

An instruction's gas cost is meant to reflect the computational cost of generating a proof of its correct execution. For some instructions, this computational cost changes based on inputs. Here are some examples and important notes:
- [`JUMP`](./instruction-set/#isa-section-jump) is an example of an instruction with constant gas cost. Regardless of its inputs, the instruction always incurs the same `l1GasCost`, `l2GasCost`, and `daGasCost`.

- All instructions have a base cost. [`JUMP`](./instruction-set/#isa-section-jump) is an example of an instruction with constant gas cost. Regardless of its inputs, the instruction always incurs the same `l1GasCost`, `l2GasCost`, and `daGasCost`.
- The [`SET`](./instruction-set/#isa-section-set) instruction operates on a different sized constant (based on its `dstTag`). Therefore, this instruction's gas cost increases with the size of its input.
- Instructions that operate on a data range of a specified "size" scale in cost with that size. An example of this is the [`CALLDATACOPY`](./instruction-set/#isa-section-calldatacopy) argument which copies `copySize` words from `environment.calldata` to `machineState.memory`.
- In addition to the base cost, the cost of an instruction increases with the number of reads and writes to memory. This is affected by the total number of input and outputs: the gas cost for [`AND`](./instruction-set/#isa-section-and) should be greater than that of [`NOT`](./instruction-set/#isa-section-not) since it takes one more input.
- Input parameters flagged as "indirect" as they require an extra memory access, so these should further increase the gas cost of the instruction.
- The base cost for instructions that operate on a data range of a specified "size" scale in cost with that size, but only if they perform an operation on the data other than copying. For example, [`CALLDATACOPY`](./instruction-set/#isa-section-calldatacopy) copies `copySize` words from `environment.calldata` to `machineState.memory`, so its increased cost is captured by the extra memory accesses. On the other hand, [`SSTORE`](./instruction-set#isa-section-sstore) requires accesses to persistent storage proportional to `srcSize`, so its base cost should also increase.
- The [`CALL`](./instruction-set#isa-section-call)/[`STATICCALL`](./instruction-set#isa-section-staticcall)/[`DELEGATECALL`](./instruction-set#isa-section-delegatecall) instruction's gas cost is determined by its `*Gas` arguments, but any gas unused by the nested contract call's execution is refunded after its completion ([more on this later](./nested-calls#updating-the-calling-context-after-nested-call-halts)).
- An instruction with "offset" arguments (like [`ADD`](./instruction-set/#isa-section-add) and many others), has increased cost for each offset argument that is flagged as "indirect".

> An instruction's gas cost will roughly align with the number of rows it corresponds to in the SNARK execution trace including rows in the sub-operation table, memory table, chiplet tables, etc.
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/simulator/src/avm/avm_gas.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ describe('AVM simulator: dynamic gas costs per instruction', () => {
// BASE_GAS(10) * 1 + MEMORY_WRITE(100) = 110
[new CalldataCopy(/*indirect=*/ 0, /*cdOffset=*/ TypeTag.UINT8, /*copySize=*/ 1, /*dstOffset=*/ 0), [110]],
// BASE_GAS(10) * 5 + MEMORY_WRITE(100) * 5 = 550
[new CalldataCopy(/*indirect=*/ 0, /*cdOffset=*/ TypeTag.UINT8, /*copySize=*/ 5, /*dstOffset=*/ 0), [550]],
[new CalldataCopy(/*indirect=*/ 0, /*cdOffset=*/ TypeTag.UINT8, /*copySize=*/ 5, /*dstOffset=*/ 0), [510]],
// BASE_GAS(10) * 1 + MEMORY_READ(10) * 2 + MEMORY_WRITE(100) = 130
[new Add(/*indirect=*/ 0, /*inTag=*/ TypeTag.UINT8, /*aOffset=*/ 1, /*bOffset=*/ 2, /*dstOffset=*/ 3), [130]],
// BASE_GAS(10) * 4 + MEMORY_READ(10) * 2 + MEMORY_WRITE(100) = 160
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/simulator/src/avm/avm_simulator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ describe('AVM simulator: injected bytecode', () => {

expect(results.reverted).toBe(false);
expect(results.output).toEqual([new Fr(3)]);
expect(context.machineState.l2GasLeft).toEqual(initialL2GasLeft - 680);
expect(context.machineState.l2GasLeft).toEqual(initialL2GasLeft - 670);
});

it('Should halt if runs out of gas', async () => {
Expand Down
4 changes: 2 additions & 2 deletions yarn-project/simulator/src/avm/opcodes/memory.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { AvmContext } from '../avm_context.js';
import { getBaseGasCost, getMemoryGasCost, mulGas, sumGas } from '../avm_gas.js';
import { getBaseGasCost, getMemoryGasCost, sumGas } from '../avm_gas.js';
import { Field, type MemoryOperations, TaggedMemory, TypeTag } from '../avm_memory_types.js';
import { InstructionExecutionError } from '../errors.js';
import { BufferCursor } from '../serialization/buffer_cursor.js';
Expand Down Expand Up @@ -218,7 +218,7 @@ export class CalldataCopy extends Instruction {
}

protected override gasCost(memoryOps: Partial<MemoryOperations & { indirect: number }> = {}) {
const baseGasCost = mulGas(getBaseGasCost(this.opcode), this.copySize);
const baseGasCost = getBaseGasCost(this.opcode);
const memoryGasCost = getMemoryGasCost(memoryOps);
return sumGas(baseGasCost, memoryGasCost);
}
Expand Down

0 comments on commit 99e12b1

Please sign in to comment.