-
Notifications
You must be signed in to change notification settings - Fork 782
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
Homestead: ShanghaiLove_Homestead failing #2406
Comments
Checked on 18.0.0 and 16.0.0, both crash, the CI used a node version of node 16 so that's weird. 16.18 crashes as well. |
Cannot reproduce at any code where the test does not fail. My ts-node is also at the right version per package.json. |
Can confirm I was able to reproduce too. |
I haven't solved it yet but this article was immensely helpful for getting me started with the nodejs debugging interface for Chrome Devtools - https://medium.com/@paul_irish/debugging-node-js-nightlies-with-chrome-devtools-7c4a1b95ae27#.pmqejrn8q Now I just have to figure out why the statemanager is causing us to run out of memory :-) |
What's weird is if I run this test with |
What could be possible is that this DoS vector |
So basically we're now susceptible to the same Shanghai DoS attack that geth was. The client finally made it to 2016! :-) |
Just to put things in perspective a bit, we have realized around our dealing with the Shanghai dDoS attacks here #1536 that we are susceptible to various different attack vectors regarding performance/out-of-memory for quite some time, so not sure if this really deserves a "very important" label. But it would be for sure nice/good if we fix/improve on those things over time. |
(but definitely no "we need to sove it now" thing) |
This PR causes the problem: #2285 |
Interesting. I wonder what about the inner workings of those two libraries is different and causes this. If we decide to go this way, it shouldn't be too too hard to just revert to the older library. I'll have to look at the API but it should be pretty similar. |
I find the reason. The space complexity of
Sadly, |
Thanks for digging into this so quickly, super great!!! 💯 We'll give this some time to play around with this a bit more and then see and decide how best to handle this (switching back to the old package would be unlucky, but we'll see. Maybe there are alternative ways here, adopting the caching mechism or something like that.). |
I think the following data structure might be helpful in this case, but it's not suitable to add to js-sdsl, maybe create a new class OrderedMapWithRollback<K, V> extends OrderedMap<K, V> {
private isRecording = false;
private recordNow: ([K, V] | K)[] = [];
private recordAll: ([K, V] | K)[][] = [];
setElement(key: K, value: V) {
this.recordNow.push(key);
super.setElement(key, value);
}
eraseElementByKey(key: K) {
const value = super.getElementByKey(key);
if (value === undefined) return;
this.recordNow.push([key, value]);
super.eraseElementByKey(key);
}
startRecord() {
if (this.isRecording) return;
this.isRecording = true;
this.recordNow = [];
}
endRecord() {
if (!this.isRecording) return;
this.isRecording = false;
this.recordAll.push(this.recordNow);
}
rollback(depth = 1) {
if (depth <= 0 || this.recordAll.length < depth) {
throw new RangeError();
}
while (depth--) {
const record = this.recordAll.pop()!;
for (const op of record) {
if (Array.isArray(op)) {
super.setElement(op[0], op[1]);
} else super.eraseElementByKey(op);
}
}
}
} |
I created a PR. js-sdsl/js-sdsl#127 |
Pretty cool! 💯 We'll try soon. 🙂 |
Solved by #2630, will close. |
Test is skipped but just checked, it passes. Will push to #2634 |
When running tests with
test-all-hardforks
label, CI now always fails sinceShanghaiLove_Homestead
makes node run out of memory.When I go back into time, for instance: c70a158, which is #2244 (this did test all hardforks, including
ShanghaiLove_Homestead
which passed), it now locally also fails.Starting to think this is not our problem, but the latest node versions might have a problem?
Note, to test;
The problem in this test is that in ShanghaiLove, the contract keeps
DELEGATECALL
ing itself, this copies a ton of code. In TangerineWhistle this is solved because now the cost ofDELEGATECALL
goes from 40 to 700, so you cannot reach a super deep stack.Node sample err;
Note;
node --version
:v18.9.0
The text was updated successfully, but these errors were encountered: