Fix some broken opcodes due to JS number issues #168
Conversation
|
@cdetrio @jwasinger @holgerd77 dare to review this? :) |
| @@ -3,6 +3,7 @@ const utils = require('ethereumjs-util') | |||
|
|
|||
| module.exports = { | |||
| getBlock: function (n, cb) { | |||
| // FIXME: this will fail on block numbers >53 bits | |||
There was a problem hiding this comment.
Actually this is not breaking for me when invoking with numbers > Number.MAX_SAFE_INTEGER, but refactoring this won't hurt and is definitely cleaner.
There was a problem hiding this comment.
After upgrading ethereumjs-util it likely will throw an exception.
| a = utils.bufferToInt(a) | ||
| return Buffer.from([!a]) | ||
| a = new BN(a) | ||
| return Buffer.from([a.isZero()]) | ||
| }, |
| pos = pos.toNumber() | ||
| word = utils.setLengthLeft(word, 32) | ||
|
|
||
| return utils.intToBuffer(word[pos]) | ||
| }, |
There was a problem hiding this comment.
Looks good as well, went through line-by-line and looked for equivalency of both versions and tested with some real-world parameters.
| pos = pos.toNumber() | ||
| loaded = runState.callData.slice(pos, pos + 32) | ||
| loaded = loaded.length ? loaded : Buffer.from([0]) | ||
| } | ||
|
|
There was a problem hiding this comment.
Look good, this first check of callData length seems to be cleaner for me as well. Wondered if the gtn for the pos > runState.callData.length comparison should be used instead of the simple > operator though it seems that this is working as well.
There was a problem hiding this comment.
Good catch, it shouldn't work with > 😕
Javascript...
There was a problem hiding this comment.
console.log(new BN(0) > 42) => false
console.log(new BN(44) > 42) => true
Ah yeah I remember, Javascript calls parseInt(object.toString()) if it is not a Number, i.e. it will do what we don't want.
There was a problem hiding this comment.
Hmmm, to just change this would have been the cleaner solution in my regard, but ok.
There was a problem hiding this comment.
It was changed, just explained why it did "work" or more likely why the tests don't have a good coverage.
|
|
||
| // block lookups must be within the past 256 blocks | ||
| if (diff > 256 || diff <= 0) { | ||
| if (diff.gtn(256) || diff.lten(0)) { | ||
| cb(null, Buffer.from([0])) | ||
| return | ||
| } |
| } | ||
|
|
||
| runState.programCounter = dest | ||
| } | ||
| }, |
There was a problem hiding this comment.
Why not put the extra code length check here in the jumpIsValid logic?
There was a problem hiding this comment.
Phew, this is so much better readable now. Looks good as well.
There was a problem hiding this comment.
Hm, that is the edge case of having JUMPI as the last instruction in the code. Need to check the YP.
There was a problem hiding this comment.
It seems that results in a STOP and not invalid jump. Will double check.
| trap(ERROR.INVALID_JUMP + ' at ' + describeLocation(runState)) | ||
| } | ||
|
|
||
| dest = dest.toNumber() |
Part of #167. Fixes #169.