Skip to content
This repository has been archived by the owner on Apr 6, 2020. It is now read-only.

Add missing Istanbul gas costs #58

Merged
merged 1 commit into from
Aug 7, 2019
Merged

Conversation

holgerd77
Copy link
Member

Added missing Istanbul gas costs for:

  • ChainID opcode (EIP-1344, as base param …in chainstart)
  • Blake2b precompile (EIP-2129/152)
  • Calldata gas cost reduction (EIP-2028)

Wasn't sure about this referenced G_BASE parameter for the ChainID opcode addition mentioned in the EIP. Py-EVM is just integrating it as a generic constant. I did a parameter search in Common and the VM and couldn't find a reference to that (maybe I missed), so I decided to add this here in a generic way to chainstart.

Maybe @rmeissner - who worked out the EIP and also did some work on the VM - could help with a comment on that.

…in chainstart), Blake2b precompile (EIP-2129/152), calldata gas cost reduction (EIP-2028)
@rmeissner
Copy link

rmeissner commented Aug 3, 2019

I took the G_BASE from the yellow paper.
Quote: G base - 2 - Amount of gas to pay for operations of the set W base.

Edit: maybe this is reflected here: https://github.com/ethereumjs/ethereumjs-common/pull/58/files#diff-17e7800cbf4bde014712cf3b079a4f45R25

@s1na
Copy link
Contributor

s1na commented Aug 5, 2019

The base gas cost for opcodes are defined in the VM. Example for an opcode that has G_BASE is ADDRESS. Snippets from ethereumjs-vm and py-evm

  0x30: ['ADDRESS', 2, true],
    opcode_values.ADDRESS: as_opcode(
        logic_fn=context.address,
        mnemonic=mnemonics.ADDRESS,
        gas_cost=constants.GAS_BASE,
    ),

@s1na
Copy link
Contributor

s1na commented Aug 5, 2019

The other two changes look good. I can approve if you'd rather keep G_BASE here, rather than hardcoded in the VM.

@holgerd77
Copy link
Member Author

@s1na I would tend to leave it here, we might want to also update on the VM side or - if for the moment too distracting - at least add some TODO note on this.

But feel free to point out some objection if you don't like this for some reason.

Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

Looks good

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants