Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(avm): Do not scale CALLDATACOPY base cost with size #5879

Merged
merged 1 commit into from
Apr 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think remove "as they":

Suggested change
- Input parameters flagged as "indirect" as they require an extra memory access, so these should further increase the gas cost of the instruction.
- Input parameters flagged as "indirect" require an extra memory access, so these should further increase the gas cost of the instruction.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in dfb8704

- 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this the only instance of scaling base cost?

Copy link
Collaborator Author

@spalladino spalladino Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SSTORE/SLOAD are the other ones I found

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Those will get solved once we do this: #5153

const baseGasCost = getBaseGasCost(this.opcode);
const memoryGasCost = getMemoryGasCost(memoryOps);
return sumGas(baseGasCost, memoryGasCost);
}
Expand Down
Loading