Fix getOpcodesForHF for when HF > istanbul#647
Conversation
Codecov Report
@@ Coverage Diff @@
## master #647 +/- ##
======================================
Coverage 91.4% 91.4%
======================================
Files 31 31
Lines 1978 1978
Branches 326 326
======================================
Hits 1808 1808
Misses 90 90
Partials 80 80
Continue to review full report at Codecov.
|
holgerd77
left a comment
holgerd77
left a comment
There was a problem hiding this comment.
Looks good, I would tend to only release this as a patch release if it it changes the method signature here. Think this is not yet very much in use yet. Would you go along with this?
| export function getOpcodesForHF(hf: string) { | ||
| if (hf === 'istanbul') { | ||
| export function getOpcodesForHF(common: Common) { | ||
| if (common.gteHardfork('istanbul')) { |
|
Update: this isn't even part of the official API, is it? |
|
Yeah I think a patch release is fine. This is not part of the API as you said |
| // Set list of opcodes based on HF | ||
| this._opcodes = getOpcodesForHF(this._common.hardfork()!) | ||
| this._opcodes = getOpcodesForHF(this._common) | ||
|
|
|
Just for reference, how did you detect this bug, @s1na? |
|
@evertonfraga I was running mainnet blocks with ethereumjs-vm and the gas usage was off. I have a setup to fetch blocks from geth's rpc endpoint and run them. I'll let you know when I push it to git. It might be useful for testing the VM with realistic blocks/txes. |
|
Sure, thanks! Will be glad to do that kind of sanity checks as well. we can even use one of the live instances from EF to have an online monitor for the VM. |
3 participants
When adding support for Istanbul opcode changes in #582, I had taken an approach that was prone to errors when adding new hardforks, and this has happened. Muir Glacier doesn't inherit the opcode changes from istanbul.
I think it's still open how to approach opcode info for HFs, but for now I've changed the condition in
getOpcodesForHFto apply istanbul changes to every HF gte Istanbul. Also added tests.This needs to be released soon as it affects evm execution in Muir Glacier.