Supplant DIFFICULTY opcode with PREVRANDAO

[ethosdev]

Reference: https://eips.ethereum.org/EIPS/eip-4399

Since the Merge, opcode 0x44 returns the output of the randomness beacon provided by the beacon chain

[holgerd77]

I am a bit cautious here, no total EVM expert, @jochem-brouwer what do you think, can we merge this without side effects? 🤔

[codecov]

Codecov Report

Merging #2321 (40d977e) into master (eb7332e) will increase coverage by 10.27%.
The diff coverage is n/a.

Additional details and impacted files

Flag	Coverage Δ
block	92.86% <ø> (?)
blockchain	90.21% <ø> (?)
client	86.97% <ø> (?)
common	98.37% <ø> (?)
devp2p	92.45% <ø> (?)
evm	?
rlp	∅ <ø> (?)
statemanager	88.43% <ø> (?)
trie	90.36% <ø> (?)
tx	97.98% <ø> (?)
util	88.99% <ø> (?)
vm	85.31% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

[jochem-brouwer]

I have to check but I think we can merge this without side effects. However, I do not think we should merge this as-is. The probem is that if you listen to step, now all DIFFICULTY opcodes are now renamed to PREVRANDAO which is confusing. What we should do is: if EVM is >= Merge then use PREVRANDAO otherwise use DIFFICULTY.

[holgerd77]

I have to check but I think we can merge this without side effects. However, I do not think we should merge this as-is. The probem is that if you listen to step, now all DIFFICULTY opcodes are now renamed to PREVRANDAO which is confusing. What we should do is: if EVM is >= Merge then use PREVRANDAO otherwise use DIFFICULTY.

Ah, ok, yes, that makes sense, thanks.

We do not have Common in this code part available (and very likely don't want to rework towards this). So I guess what we can do here is to add a new section to hardforkOpcodes like this after Istanbul:

{
    hardfork: Hardfork.Merge,
    opcodes: {
      0x44: { name: 'PREVRANDAO', isAsync: true, dynamicGas: false },
    },
  },

And then we would move the change here back to DIFFICULTY.

Right?

This is then getting so complex that I would like to have at least one test case for this.

@ethosdev can you do these additional updates/changes?

[holgerd77]

Update: ah, and interestingly CI is failing for EVM when run with the current change. This should be investigated before we do the proposed update, otherwise it might not be repeatable any more and this might hint and some overall problems with the PR.

Ah, just seeing, it's a ethereum/tests state test called blockInfo:

# file: VMTests/vmTests/blockInfo.json test: blockInfo
ok 498 [ 0.015 secs ] the state roots should match (successful tx run)
not ok 499 [ 0.015 secs ] the state roots should match (successful tx run)
  ---
    operator: ok
    expected: true
    actual:   false
    at: runTestCase (/home/runner/work/ethereumjs-monorepo/ethereumjs-monorepo/packages/vm/tests/tester/runners/GeneralStateTestsRunner.ts:149:5)
    stack: |-
      Error: [ 0.015 secs ] the state roots should match (successful tx run)
          at Test.assert [as _assert] (/home/runner/work/ethereumjs-monorepo/ethereumjs-monorepo/node_modules/tape/lib/test.js:314:54)
          at Test.bound [as _assert] (/home/runner/work/ethereumjs-monorepo/ethereumjs-monorepo/node_modules/tape/lib/test.js:99:32)
          at Test.assert (/home/runner/work/ethereumjs-monorepo/ethereumjs-monorepo/node_modules/tape/lib/test.js:433:10)
          at Test.bound [as ok] (/home/runner/work/ethereumjs-monorepo/ethereumjs-monorepo/node_modules/tape/lib/test.js:99:32)
          at runTestCase (/home/runner/work/ethereumjs-monorepo/ethereumjs-monorepo/packages/vm/tests/tester/runners/GeneralStateTestsRunner.ts:149:5)
          at async runStateTest (/home/runner/work/ethereumjs-monorepo/ethereumjs-monorepo/packages/vm/tests/tester/runners/GeneralStateTestsRunner.ts:174:9)
          at async /home/runner/work/ethereumjs-monorepo/ethereumjs-monorepo/packages/vm/tests/tester/index.ts:234:17
          at async fileCallback (/home/runner/work/ethereumjs-monorepo/ethereumjs-monorepo/packages/vm/tests/tester/testLoader.ts:54:11)
  ...

Then I would very much assume that ethereum/tests is just not prepared yet for this change. 🤔

[jochem-brouwer]

We should definitely investigate this, I do not see how changing the opcode name changes the outcome of the state roots. I thought it was just a semantic thing..?

Anyways, in codes.ts we have access to common in the getOpcodesForHF, (which is also the one which we export since it holds all logic). So it should not be that complex, we have the case if >=Merge replace the difficulty opcode name prevrandao, or else "replace" it again with difficulty.

[holgerd77]

We should definitely investigate this, I do not see how changing the opcode name changes the outcome of the state roots. I thought it was just a semantic thing..?

Anyways, in codes.ts we have access to common in the getOpcodesForHF, (which is also the one which we export since it holds all logic). So it should not be that complex, we have the case if >=Merge replace the difficulty opcode name prevrandao, or else "replace" it again with difficulty.

I actually find the proposed solution a lot cleaner even if we have access to common, why to add various logic parts if this can remain plainly descriptive?

I would very much assume that the blockInfo ethereum/tests test is doing some weird contract stuff to simulate testing the opcode names (so: setting in the contract, comparing with DIFFICULTY in contract call, something like that). So - yes, I would be 99% sure that you are right, it's only semantics - however it is baked into the test suite to have some common groud here (all unproven, but pretty likely I would say).

[jochem-brouwer]

ethereumjs-monorepo/packages/evm/src/opcodes/codes.ts

Line 350 in eb7332e

const baseFee = Number(common.param('gasPrices', opcodeBuilder[key].name.toLowerCase()))

this is the problem

Change sha3 in common to keccak256 and it will/should work :)

[holgerd77]

Have added to #2706, will close here