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

mathSqrt summary #51

Merged
merged 6 commits into from
Oct 3, 2024
Merged

mathSqrt summary #51

merged 6 commits into from
Oct 3, 2024

Conversation

mariaKt
Copy link
Contributor

@mariaKt mariaKt commented Oct 3, 2024

This PR introduces summarized rules for the mathSqrt function.

These rules are saving 818 steps.

I would like your opinion specifically on this rule here:

  // Skip environment updates when not needed.
  // These occur due to the multiple block statements, introduced by the if
  // statement that the while rewrites to.
  rule <k> restoreEnv( _:Map ) ~> .Statements ~> restoreEnv( E:Map ) => restoreEnv(E) ...</k>
       <summarize> true </summarize>
       <current-function> mathSqrt </current-function> [priority(40)]

Due to the while statement rewriting to an if statement, as the function is executed, there are a lot of block statements that eventually lead to a sequence of

restoreEnv(E) ~> .Statements ~> restoreEnv(E) ~> .Statements ~> ...

I added this rule to skip the unneeded updates. Due to the number of while loop body executions being input dependent, I cannot specify the exact number that this is repeated, so this is a step per loop iteration.

My question is: is this rule OK to include in a summary, or is it considered a general semantic rule?

Copy link
Contributor

@Robertorosmaninho Robertorosmaninho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just a few clarification questions before approving it:

src/uniswap-summaries.md Show resolved Hide resolved
<call-stack>... .List => ListItem(frame(K, E, FUNC)) </call-stack>
requires V >uMInt 3p256 [priority(40)]

rule <k> mathSqrt:Id ( v(V:MInt{256}, uint256), .TypedVals ) => v (1p256, uint256) ...</k>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why the cells: <this-type>, <contract-id>, <contract-fn-id>, etc. wouldn't be necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This rule replaces the entire call. So, we do not need to try and find the code of the function, we simply return a TypedVal. We know that a <contract-fn> with <contract-fn-id> mathSqrt </contract-fn-id> exists, but we do not need to find any information from the cells in that <contract-fn>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can think of it just like any other summary of en entire call (which it is with these conditions), e.g.

rule <k> uniswapV2LibrarySortTokens:Id ( v(V1:MInt{160}, address #as T), v(V2:MInt{160}, T), .TypedVals ) => v(ListItem(V1) ListItem(V2), T[]) ...</k>

@@ -3280,6 +3280,13 @@ module SOLIDITY-MATHSQRT-SUMMARY
<call-stack>... .List => ListItem(frame(K, E, FUNC)) </call-stack>
requires V >uMInt 3p256 [priority(40)]

// While loop rewrite and condition evaluation
rule <k> while (( x < z ) #as Cond) Body:Statement Ss:Statements => if (v(Vx <uMInt Vz, bool)) {Body while(Cond) Body} else {.Statements} ~> Ss ...</k>
<summarize> true </summarize>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question from the case y <= 3 && y != 0 about the cell variables

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we are already within the body of the function mathSqrt, i.e. to get to this rule the one doing the matching and changes to the configuration related to the call itself has already matched and done its rewriting. So, we don't need to match any of those cells.

Copy link
Contributor

@Robertorosmaninho Robertorosmaninho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mariaKt mariaKt merged commit 9b43aa7 into main Oct 3, 2024
1 check passed
@mariaKt mariaKt deleted the mathsqrt-summary branch October 3, 2024 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants