-
Notifications
You must be signed in to change notification settings - Fork 151
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
Optimize summary rules #2732
Optimize summary rules #2732
Conversation
- remove unneccessary summaries with `statusCode` changes, like `EVM_STACK_OVERFLOW` - remove unneccessary constraints for the preconditions - remove ensures clauses that might not be satified by the preconditions.
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.
LGTM. I'll let @JuanCoRo approve the PR though, as he has a better understanding of the implications of this change on the proving process.
kevm-pyk/src/kevm_pyk/summarizer.py
Outdated
@@ -289,6 +289,35 @@ def get_todo_list() -> list[str]: | |||
return todo_list | |||
|
|||
|
|||
def stack_added(opcode_id: str) -> int: |
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.
def stack_added(opcode_id: str) -> int: | |
def stack_added(opcode: str) -> int: |
(Similarly for the rest.)
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'm working on the better rule id and will fix it latter!
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.
After that, I hope that @JuanCoRo can just use <opcode name>-SUMMARY-USEGAS
to generate the lean code.
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.
Done!
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.
Hey @Stevengre! Should the -SUMMARY-USEGAS
labeling be in place already? For what I see in the last commit it's still using the old one (e.g., rule [ADD-SUMMARY-0]:
)
Is this meant for a follow up PR? Thank you!
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.
Sure, I'll provide the better names in the next PR. Just after merging this one.
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.
For this one, I make the require clause more feasible for some opcode and reduce the rules to make it easier to generate better names.
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.
There are still some problems for some opcodes, like balance
. But I think that I can resolve it when you need.
- Updated the `stack_added`, `stack_needed`, and `stack_delta` functions to accept `opcode` as a parameter instead of `opcode_id`, improving clarity and consistency in the codebase.
andBool ( ( notBool #sizeWordStack ( WS:WordStack , 0 ) <Int 0 ) | ||
andBool ( ( notBool 1023 <Int #sizeWordStack ( WS:WordStack , 0 ) ) | ||
andBool ( #sizeWordStack ( WS:WordStack , 0 ) <=Int 1023 |
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.
These conditions here are not exactly equivalent, right? I think we would still need something like
0 <=Int #sizeWordStack ( WS:WordStack , 0 )
Am I missing something?
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.
This is generated automatically, since I don't provide any not underflow condition for the opcodes. This follows the same principle as the not overflow before opcode execution that you might need to add this assumption in the theorems.
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.
Do you know why there's an overflow requirement but not an underflow? I'm just trying to understand where this condition get s dropped.
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.
Wait a minute... Let me check the semantics...
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.
Reason for this:
The following new-introduced code provides not overflow
precondition for the opcode summaries.
init_constraints.append(_le(_ws_size(stack_needed(op)), KToken(str(1024 - delta), 'Int'))) |
As a result, the following rule applies without a split according to the conditions.
evm-semantics/kevm-pyk/src/kevm_pyk/kproj/evm-semantics/evm.md
Lines 347 to 356 in e7498c3
rule <k> #next [ OP:OpCode ] | |
=> #addr [ OP ] | |
~> #exec [ OP ] | |
~> #pc [ OP ] | |
... | |
</k> | |
<wordStack> WS </wordStack> | |
<static> STATIC:Bool </static> | |
requires notBool ( #stackUnderflow(WS, OP) orBool #stackOverflow(WS, OP) ) | |
andBool notBool ( STATIC andBool #changesState(OP, WS) ) |
However, without this new precondition, there will be a split for
notBool ( #stackUnderflow(WS, OP) orBool #stackOverflow(WS, OP) )
andBool notBool ( STATIC andBool #changesState(OP, WS) )
And it seems like the haskell backend doesn't reduce notBool ( #stackUnderflow(WS, OP) orBool #stackOverflow(WS, OP) )
to its simplest conjunctive normal form (CNF) -- notBool #stackUnderflow(WS, OP) andBool notBool #stackOverflow(WS, OP)
.
Therefore, when notBool ( #stackUnderflow(WS, OP) orBool #stackOverflow(WS, OP) )
is false, since #stackUnderflow(WS, OP)
is always false for pushzero, then this constraint is reduced to #stackOverflow(WS, OP)
and evaluate to 1023 <Int #sizeWordStack ( _WS:WordStack , 0 )
.
When notBool ( #stackUnderflow(WS, OP) orBool #stackOverflow(WS, OP) )
is true, ???
Sorry, forget it... It looks strange to me. It might also relate to the existing simplifcation rules like:
rule N <=Int #sizeWordStack ( _ , N ) => true requires 0 <=Int N [simplification, smt-lemma] |
evm-semantics/kevm-pyk/src/kevm_pyk/kproj/evm-semantics/lemmas/int-simplification.k
Lines 218 to 219 in e7498c3
rule notBool (A <Int B) => B <=Int A [simplification] | |
rule notBool (A <=Int B) => B <Int A [simplification] |
And it might also relate to the way that the backend tackle the side conditions and simplication rules.
I'm sorry, Juan. I'm actually not quite sure why such a strange condition was generated previously. This is indeed an interesting issue. I think @ehildenb and @jberthold might have more thoughts on what could be causing this result.
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.
These conditions here are not exactly equivalent, right? I think we would still need something like
0 <=Int #sizeWordStack ( WS:WordStack , 0 )
Am I missing something?
Reason for this:
...
rule N <=Int #sizeWordStack ( _ , N ) => true requires 0 <=Int N [simplification, smt-lemma]
I think the simplification that @Stevengre points out makes the 0 <= ...
condition redundant (specialise N == 0
in the lemma).
And just to clarify:
And it seems like the haskell backend doesn't reduce
notBool ( #stackUnderflow(WS, OP) orBool #stackOverflow(WS, OP) )
to its simplest conjunctive normal form (CNF) --notBool #stackUnderflow(WS, OP) andBool notBool #stackOverflow(WS, OP)
.
Correct for the booster
backend, we are trying to not transform predicates to CNF (the only transformation we do is to turn ML predicates into 1st-order ones). The legacy backend might have logic to transform to CNF but this is actually not a desired thing - we want all simplifications to be readable K code rather than backend magic.
In this case, we could maybe just write the CNF into the rule in the first place?
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.
Thank you @Stevengre and @jberthold!!
It makes sense to me now, so I'll approve the PR!
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.
Thank you @jberthold! It makes a lot of sence! But I think that we can also make this CNF during the frontend process. I'll make an issue to record this.
statusCode
changes, likeEVM_STACK_OVERFLOW