fix: expand memory on reading prev. untouched location

[nvnx7]

Memory is expanded by word when accessing previously untouched memory word (relevant docs). That applies to read operation on memory too.

[holgerd77]

Hi there,
thanks a lot for the PR, that's obviously a very core change/fix.

Did this have some practical implications for you, I am rather curious, how did you stumble upon this?

Also: am I correct that the main implication of this that this would/will change gas costs in some certain circumstances? If this is the case I am a bit confused why this bug/misbehavior has not been detected by the official ethereum/tests test suite? There should likely be some test for this? 🤔 If not we might want to reach out there and/or write/add such a test ourselves.

//cc @jochem-brouwer (other)

[codecov]

Codecov Report

Merging #1887 (f596ecd) into master (bdfbe37) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Flag	Coverage Δ
block	85.57% <ø> (ø)
blockchain	83.82% <ø> (ø)
client	76.81% <ø> (ø)
common	94.19% <ø> (ø)
devp2p	82.62% <ø> (+0.13%)	⬆️
ethash	90.76% <ø> (ø)
trie	80.02% <ø> (ø)
tx	88.22% <ø> (ø)
util	92.62% <ø> (ø)
vm	81.00% <100.00%> (-0.15%)	⬇️

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

[nvnx7]

Hey, I was actually coding up a mini - evm in rust and was visually cross-checking evm's behavior on evm.codes after digging deep into docs/papers for my project.
evm.codes (which depends on this project) gave odd results when doing a MLOAD at previously untouched memory area. For example:

PUSH1 0x04
MLOAD
MSIZE

Running the above code should increase memory size to 64 (0x40) - which the output of last MSIZE can confirm. But reading the memory buffer yields 0 length array instead of an array of length 64 filled with 0s:

vm.on('step', function (data) {
  // data.memory.length is 0 here, instead of 64!
})

Regarding the gas costs - yes, expansion - if any - does cost a bit of extra gas during read/write to memory.

I couldn't find any tests that specifically tested the expansion behavior of memory during read/write operations. This case must've been missed most probably because expansion is not actually a part of read/write methods in source code.

Unless there's some specific requirement I'm unaware of, I think extend should be included in the read and write methods itself like:

  write(offset: number, size: number, value: Buffer) {
    this.extend(offset, size); // extend memory automatically
    if (size === 0) {
      return
    }

    assert(value.length === size, 'Invalid value size')
    assert(offset + size <= this._store.length, 'Value exceeds memory capacity')
    assert(Buffer.isBuffer(value), 'Invalid value type')

    for (let i = 0; i < size; i++) {
      this._store[offset + i] = value[i]
    }
  }
  
 /**
  * Similarly for `read`  method
  */

...instead of doing an extend manually at the opcode function level (e.g. at CALLDATACOPY, CODESIZE, WRITE in functions.ts). Because every extend is accompanied by a read/write anyway (right?). That would also create the required test scenario of proper memory expansion on untouched location access.

[jochem-brouwer]

Thanks for opening this PR!

I will double-check this but this should not change gas costs. This will indeed influence the step output but it will not contain a consensus bug. The reason is that in gas.ts if you try to access memory (for MLOAD, MSTORE, MSTORE8 but also things like CALL) then it calls subMemUsage:

ethereumjs-monorepo/packages/vm/src/evm/opcodes/util.ts

Line 186 in 4217ed6

export function subMemUsage(runState: RunState, offset: BN, length: BN, common: Common) {

This does update the gas-related fields like the highestMemCost and memoryWordCount (this keeps track of the memory length). However, this does not actually expand the internal buffer. This is what causes the length to be incorrect in the step event. We should fix it for the step event but this should not have caused any consensus bug. (I am also fairly certain that this is tested anywhere in ethereum/tests, this does not really seem like an edge case). However I will still double check.

[jochem-brouwer]

Just checked;

  const vm = await VM.create()
  const runCodeArgs = {
    code: Buffer.from('602051', 'hex'),
    gasLimit: BigInt(0xffff),
  }

  const result = await vm.runCode(runCodeArgs)
  console.log(result.gasUsed)

This runs PUSH 20 MLOAD, so this should expand memory to 0x20 (32 bytes).

On both master and this branch this consumes 12 gas. We should merge nevertheless such that step event reports the correct value.

[jochem-brouwer]

I think we should add the memory.extend into memory.ts, specifically the read method. It should check if current memory size is lower than offset + size, if that is the case first extend memory and then return the Buffer. This PR fixes the step event output for MLOAD, but not for opcodes which read memory like CALL, CALLCODE, DELEGATECALL, etc.

[holgerd77]

@jochem-brouwer ah, thanks for the in-depth analysis, that is really interesting, took me a while to grasp why this is not a consensus bug.

So how would you recommend that we proceed here? Should we merge this one in and you would do an on-top-PR? Would you directly do an alternative PR? Other options? 🙂

[holgerd77]

@theNvN ah, and thanks for your extensive explanatory write-up as well! 🙏 ❤️

[nvnx7]

Just checked;

  const vm = await VM.create()
  const runCodeArgs = {
    code: Buffer.from('602051', 'hex'),
    gasLimit: BigInt(0xffff),
  }

  const result = await vm.runCode(runCodeArgs)
  console.log(result.gasUsed)

This runs PUSH 20 MLOAD, so this should expand memory to 0x20 (32 bytes).

On both master and this branch this consumes 12 gas. We should merge nevertheless such that step event reports the correct value.

PUSH 20 MLOAD should actually cause memory to expand to a total size of 0x40 (64 bytes) :) ...because MLOAD would load a word (32 bytes) starting from the given offset.

[nvnx7]

Thanks for opening this PR!

I will double-check this but this should not change gas costs. This will indeed influence the step output but it will not contain a consensus bug. The reason is that in gas.ts if you try to access memory (for MLOAD, MSTORE, MSTORE8 but also things like CALL) then it calls subMemUsage:

ethereumjs-monorepo/packages/vm/src/evm/opcodes/util.ts

Line 186 in 4217ed6

export function subMemUsage(runState: RunState, offset: BN, length: BN, common: Common) {

This does update the gas-related fields like the highestMemCost and memoryWordCount (this keeps track of the memory length). However, this does not actually expand the internal buffer. This is what causes the length to be incorrect in the step event. We should fix it for the step event but this should not have caused any consensus bug. (I am also fairly certain that this is tested anywhere in ethereum/tests, this does not really seem like an edge case). However I will still double check.

Nice. Yes, I think the related gas cost part is behaving correctly. This might be helpful.

[nvnx7]

I think we should add the memory.extend into memory.ts, specifically the read method. It should check if current memory size is lower than offset + size, if that is the case first extend memory and then return the Buffer. This PR fixes the step event output for MLOAD, but not for opcodes which read memory like CALL, CALLCODE, DELEGATECALL, etc.

Yes. That's what I thought too!

Unless there's some specific requirement I'm unaware of, I think extend should be included in the read and write methods itself

And what about the write method? Shouldn't the memory.extend be in write too...?

[jochem-brouwer]

I think the end goal of this PR is that the step event output of the memory matches what is going on in the VM. This means that whenever we read memory it is extended (and possibly also on write, we should probably expand it with 32 bytes whenever we touch a new region). This eventually means that we can also refactor runState, we do not need memoryWordCount anymore since we can read this directly from the memory. @theNvN do you think you could add both these changes to runState.memory.extend and runState.memory.write? I can help refactoring the runState.

[nvnx7]

Sure 👍🏼. I can push additional changes.

Just so we're on the same page - I should make these changes (?):

• (memory.ts) Put the memory.extend in memory.read and memory.write .
• (functions.ts) Remove memory.extends from opcode handler functions.
• (memory.spec.ts) a couple of additional tests for proper memory expansion on read/write.
• (opcode/utils.ts) In subMemUsage(..), should we maybe just set new value for memoryWordCount as follows?:

// const newMemoryWordCount = divCeil(offset.add(length), new BN(32)); // remove
const newMemoryWordCount = new BN(runState.memory._store.length / 32); // add

Anything else..? Let me know.

[jochem-brouwer]

@theNvN Yes that is correct. Thanks a lot! 😄

[nvnx7]

@jochem-brouwer pushed changes except last one (memoryWordCount in subMemUsage(..)) as it causes some tests to fail (gas usage related I think)

[jochem-brouwer]

@theNvN CI does not show any failing tests. Which are failing?

[nvnx7]

Sure 👍🏼. I can push additional changes.

Just so we're on the same page - I should make these changes (?):

• (memory.ts) Put the memory.extend in memory.read and memory.write .
• (functions.ts) Remove memory.extends from opcode handler functions.
• (memory.spec.ts) a couple of additional tests for proper memory expansion on read/write.
• (opcode/utils.ts) In subMemUsage(..), should we maybe just set new value for memoryWordCount as follows?:

// const newMemoryWordCount = divCeil(offset.add(length), new BN(32)); // remove
const newMemoryWordCount = new BN(runState.memory._store.length / 32); // add

Anything else..? Let me know.

I didn't push that change. It is the last one in above (in opcode/utils.ts)

This:

const newMemoryWordCount = divCeil(offset.add(length), new BN(32));

is not always same as this:

const newMemoryWordCount = new BN(runState.memory._store.length / 32); 

[jochem-brouwer]

Huh that's weird, I do recall that I commented a warning about that on here, but apparently I did not.

const newMemoryWordCount =  divCeil(new BN(runState.memory.length), new BN(32))

This should work.

[nvnx7]

Huh that's weird, I do recall that I commented a warning about that on here, but apparently I did not.

const newMemoryWordCount =  divCeil(new BN(runState.memory.length), new BN(32))

This should work.

Nope. Still failing...probably the same tests (incl. runTx.spec.ts, runCall.spec.ts) as before. newMemoryWordCount calculates to a different value as opposed to the original expression. I'm not very sure of the logic of gas costs arithmetic here.

An interesting thing I find is that gas cost is subtracted first:
https://github.com/theNvN/ethereumjs-monorepo/blob/memory-bug-fix/packages/vm/src/evm/interpreter.ts#L184

and then opcode handler is executed later:
https://github.com/theNvN/ethereumjs-monorepo/blob/f596ecd8d8ffcf7576ff951448e42d891b300f38/packages/vm/src/evm/interpreter.ts#L204-L209

[jochem-brouwer]

Ahh yes you are on the right track 👍! You are right that gas first gets calculated and then the actual function is executed. subMemUsage is only called from gas.ts. Since we have not yet extended the memory (in functions.ts) we should thus calculate what the actual length is (and not take it from the memory length which could currently be incorrrect). So, this step is actually not necessary. I will re-run the CI to see if it passes only all hardforks, if that is OK then I will approve it (but I will wait until someone else approves it too before merging!)

Thanks a lot 😄 👍

[nvnx7]

cool!

[jochem-brouwer]

Thanks, LGTM!

[holgerd77]

@theNvN nice, thanks a lot! 👍 🙂

[nvnx7]

my pleasure 😄 👍