explicitly duplicate bignumbers on stack

[jochem-brouwer]

Solves #732

[codecov]

Codecov Report

Merging #733 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #733      +/-   ##
==========================================
+ Coverage   91.66%   91.71%   +0.05%     
==========================================
  Files          46       46              
  Lines        2998     3006       +8     
  Branches      469      468       -1     
==========================================
+ Hits         2748     2757       +9     
  Misses        151      151              
+ Partials       99       98       -1     

Flag	Coverage Δ
#account	94.11% <ø> (ø)
#block	88.36% <ø> (+0.27%)	⬆️
#blockchain	89.15% <ø> (+0.41%)	⬆️
#common	94.37% <ø> (ø)
#tx	94.02% <ø> (ø)
#vm	92.53% <100.00%> (ø)

Impacted Files	Coverage Δ
packages/vm/lib/evm/stack.ts	94.87% <100.00%> (ø)
packages/blockchain/src/cache.ts	100.00% <0.00%> (+20.00%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65c9620...4488760. Read the comment docs.

[jochem-brouwer]

So what I did is create a minimal EVM code (I put a comment in the test): what it does is to ensure we run into a requirement of the gas stipend (2300 gas) if a nonzero Ether CALL is made. We do this because this internally calls iaddn of bn.js which thus edits the "host" of the number, not a clone of the number.

Before we make this call, we PUSH 00 and DUP this item multiple times - 1 time too many actually, because we want to keep the original PUSHed item on stack for later use, to show that this is internally edited by the VM. After the call, what we expect is that 7 items get popped of the stack (this happens), but the rest of the stack should be unchanged. However, this is not the case: after POPing the 1 pushed on the stack by the CALL, what remains on the stack is 0x8fc, which is 2300. For some reason 2300 got on the stack - it is actually edited by opFns when internally writing to gasLimit which refers to one of the POPed items of the stack prior in that function: however, because we have DUPed the original item on the stack we are thus now writing to a value which is still on the stack.

To show that this happens, the current value of the stack is MSTOREd at 0 and then we return this memory slot (i.e. 32 bytes at offset 0). This item should be 0 (the original PUSHed item), but it is 2300 if we do not implement cloning the BN.js on the stack. However, if we clone it, it returns 0 just fine. If we thus clone this number we are free to edit it internally, as we can at least be sure that it will not edit other DUPed items on the stack.

[jochem-brouwer]

If this impacts the performance a lot then it might also be possible to fix this in opFns directly by cloning gasLimit. However this is more unsafe as other situations could not be captured by this.

[holgerd77]

Thanks, that's great! 😄

Ah, yeah, I see what you mean ("a bit out of place"), lol 😜, but anyhow, doesn't hurt or blow things up either, at least would be my stance here on this.

[jochem-brouwer]

I don't really see how to make it much compacter either. For now, it seems like only this gas stipend thing messes up the stack as I don't see any other potentially stack-editing operations in opFns via BN.js i* arithmetic when glancing over the file.

[holgerd77]

Thanks, LGTM!

[holgerd77]

Side note: compared for a very rough performance check (we really should do better here!) against CI run times of a recently submitted VM PR, nothing odd to discover on first sight.

[jochem-brouwer]

Yes, I don't think the performance will be impacted a lot by it: stack sizes are at most 32 bytes * 1024 per call frame, so allocating memory of that size should not take a lot of time.