-
Notifications
You must be signed in to change notification settings - Fork 765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework memory expansion/access in opFns #174
Conversation
c67eaf0
to
17c7272
Compare
This should work, but the reason it fails in some tests I think is because the VM is actually broken in those aspects, but the exception of |
a867cb3
to
8ad6d0f
Compare
lib/opFns.js
Outdated
const newMemoryWordCount = Math.ceil((offset + length) / 32) | ||
|
||
if (newMemoryWordCount <= runState.memoryWordCount) return | ||
const newMemoryWordCount = offset.add(length).divRound(new BN(32)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the use of divRound
is a problem here because we want to be rounding up to the next full word (i.e. ceil
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review, but divRound
is supposed to round up and also had an assertion for comparing the two versions. It might still be wrong, but most of the failures I've found weren't gas related, but plain crashes.
This PR is not finished and have found a lot of other bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var BN = require('ethereumjs-util').BN
var x = new BN(20)
var y = new BN(15)
x = x.divRound(y)
console.log(x.toString())
gives
1
So it is clearly rounding down in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, in that case the comparison was wrong, however if you look at the test results, there are plenty of other failures (number too big). Those I am in progress fixing.
8ae1477
to
384e5fb
Compare
7a7f163
to
0a51469
Compare
Down to these failures:
|
lib/opFns.js
Outdated
* @return {String} | ||
*/ | ||
function subMemUsage (runState, offset, length) { | ||
// abort if no usage | ||
if (!length) return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So apparently this one is the correct behaviour as per the sidenote in the YP in section H.1.
Working on this. I was able to bring failures down from 569->526. |
Down to 2 failures! |
a0f8a39
to
3ab1bee
Compare
@jwasinger can you let me know when this is near completion? I'd like to squash done some commits (mostly mine). |
@axic Sure. I'm working on the remaining failures now. |
@axic I think this is ready (other than linting travis failure). |
@axic can you squash your commits? then I can fix linting and we can get this merged. |
Yep will do. |
Finally this works now, just need to fix lint & squash more things. |
@jwasinger can you do a review and possibly merge? |
@jwasinger the funny thing is we had the zeroing out, but the state tests pass with and without so who knows.. |
That's a bit unsettling.. I got hung up on how to remove that hack but I think I've got it now. |
419c505
to
ba0dc82
Compare
ba0dc82
to
dd83b2c
Compare
Removing path traversal to run script
Fixes #167. Depends on #175, #179, #186 and #188.