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

Fixing up BufLength, the use of nubOrd, and adding one more PLT rule #424

Merged
merged 2 commits into from
Nov 9, 2023

Conversation

msooseth
Copy link
Collaborator

@msooseth msooseth commented Nov 8, 2023

Description

This is a fix to Prop ordering and 2 improvements.

Prop Ordering and nubOrd

remRedundantProps p = nubOrd $ collapseFalse . filter (\x -> x /= PBool True) . nubOrd $ p

has been changed to have nubOrd at the front. More importantly, Prop was not correctly ordered(!). It missed ordering by Prop type AND it had an issue with using && instead of ||. We have now fixed this and I have added new tests to catch this kind of error.

BufLength issue

Expr.simplify $ (Max (Lit 0) (BufLength (AbstractBuf "txdata")))

became:

(Max (Lit 0) (BufLength (AbstractBuf "txdata")))

Effectively "ignoring" the rule:

    go (Max a b) = case (a, b) of
                    (Lit 0, _) -> b
                    _ -> EVM.Expr.max a b

But in fact it was NOT ignoring that rule. Instead,

    go (BufLength buf) = bufLength buf

Was in fact creating an infinite loop(!) because of:

 bufLengthEnv :: Map.Map Int (Expr Buf) -> Bool -> Expr Buf -> Expr EWord
 bufLengthEnv env useEnv buf = go (Lit 0) buf
   where
     go :: Expr EWord -> Expr Buf -> Expr EWord
     [...]
    go l (AbstractBuf b) = Max l (BufLength (AbstractBuf b))

Which added one Max every time -- which was then stripped by the Max rule. This lead to a "fixedpoint" and so things didn't get simplified. This has now been fixed.

We also added 2 more small rules:

    go (PLT (Max (Lit a) b) (Lit c)) | a < c = PLT b (Lit c)

Checklist

  • tested locally
  • added automated tests
  • updated the docs
  • updated the changelog

@msooseth msooseth requested a review from zoep November 8, 2023 15:15
Copy link
Collaborator

@zoep zoep left a comment

Choose a reason for hiding this comment

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

Nice catch!

@msooseth msooseth force-pushed the better-simplifications branch from 62cc356 to 2a8c2f4 Compare November 9, 2023 11:40
@msooseth msooseth merged commit 5e389fc into main Nov 9, 2023
7 checks passed
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.

3 participants