Skip to content
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

Default max_depth = 25000, no break-on-calls #2032

Closed
wants to merge 2 commits into from

Conversation

palinatolmach
Copy link
Contributor

@palinatolmach palinatolmach commented Aug 23, 2023

Partially addresses #1958.

This PR is aimed to improve the default observable performance of KEVM by making no-break-on-calls default and increasing default --max-depth to 25000.

@palinatolmach palinatolmach marked this pull request as ready for review August 23, 2023 09:07
@PetarMax
Copy link
Contributor

There seems to be a proof failing because an output obtained from the is different - I am confused why this could be showing up with this change - could it be that the output depends on break-on-calls? @ehildenb @palinatolmach

@ehildenb
Copy link
Member

Hmmmm, is it actually different, or just failing the proof because of flakiness? The booster seems to think it has the same structure as before, so I'd be surprised if it was actually different output. I guess we may need to look more closely at it.

@PetarMax
Copy link
Contributor

It could be flakiness, I just saw it fail twice in the same way, so I flagged.

@PetarMax
Copy link
Contributor

It just failed in the same way right now...

@PetarMax
Copy link
Contributor

I can confirm that there is a problem with the output, here is a snippet:

OBTAINED:

(2819 steps)\n├─ 5\n│   k: CALL 9223372036854765663 645326474426547203313410069153905908525362434349 0 388  ...\n│   pc: 2389\n│   callDepth: 0\n│   statusCode: STATUSCODE:StatusCode\n│\n│  (1 step)\n├─ 6\n│   k: #checkCall 728815563385977040452943777879061427756277306518 0 ~> #call 728815563 ...\n│   pc: 2389\n│   callDepth: 0\n│   statusCode: STATUSCODE:StatusCode\n│\n│

EXPECTED:

(1000 steps)\n├─ 5\n│   k: #gas [ MSTORE , MSTORE 192 46308022326495007027972728677917914892729792999299745 ...\n│   pc: 2271\n│   callDepth: 0\n│   statusCode: STATUSCODE:StatusCode\n│\n│  (1000 steps)\n├─ 6\n│   k: #gasExec ( LONDON , JUMP 2621 ) ~> #deductGas ~> #access [ JUMP , JUMP 2621 ] ~> ...\n│   pc: 2691\n│   callDepth: 0\n│   statusCode: STATUSCODE:StatusCode\n│\n│  (819 steps)\n├─ 7\n│   k: CALL 9223372036854765663 645326474426547203313410069153905908525362434349 0 388  ...\n│   pc: 2389\n│   callDepth: 0\n│   statusCode: STATUSCODE:StatusCode\n│\n│  (1 step)\n├─ 8\n│   k: #checkCall 728815563385977040452943777879061427756277306518 0 ~> #call 728815563 ...\n│   pc: 2389\n│   callDepth: 0\n│   statusCode: STATUSCODE:StatusCode\n│\n│

from which you can see two things:

  1. OBTAINED does 2819 steps immediately, EXPECTED does 1000+1000+819; but also, more worryingly
  2. they both do 1 step afterwards, which means we still break on calls and the PR needs further work, I think.

@palinatolmach @ehildenb

@palinatolmach
Copy link
Contributor Author

palinatolmach commented Aug 25, 2023

@PetarMax thanks for the investigation, I'll look into it! I wanted to check if the test is failing locally too, but had problems running tests on my machine. It's strange that it doesn't fail when the booster is on though.

@ehildenb
Copy link
Member

Hmmmm, some thoughts:

  • Have we confirmed that the booster is running that test?
  • Could it be that the booster is branching earlier anyway, because it's not quite reached its goal of being observationally equivalent to the old backend?

@palinatolmach palinatolmach self-assigned this Oct 19, 2023
@yale-vinson yale-vinson added the enhancement New feature or request label Oct 23, 2023
@palinatolmach palinatolmach reopened this Oct 25, 2023
@ehildenb
Copy link
Member

Out of date.

@ehildenb ehildenb closed this Dec 19, 2023
@ehildenb ehildenb deleted the optimized-defaults branch December 19, 2023 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants