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

[DOC-351] Document control flow splitting #164

Merged
merged 20 commits into from
Jan 26, 2024

Conversation

alexandernutz
Copy link
Contributor

@alexandernutz alexandernutz commented Oct 27, 2023

Add a conceptual explanation for the (control flow) splitting feature. This understanding is helpful for mitigating timeouts using the corresponding collection of cli options.

Jira ticket: https://certora.atlassian.net/browse/DOC-351
Link to generated documentation: https://certora-certora-prover-documentation--164.com.readthedocs.build/en/164/docs/prover/techniques/index.html#control-flow-splitting

@alexandernutz alexandernutz added the existing feature new documentation for an existing feature label Nov 7, 2023
Base automatically changed from alex/timeouts-update to master November 21, 2023 13:35
@alexandernutz alexandernutz marked this pull request as ready for review November 21, 2023 16:05
@alexandernutz alexandernutz requested a review from a team as a code owner November 21, 2023 16:05
@alexandernutz alexandernutz changed the title Alex/control flow splitting Document control flow splitting Nov 23, 2023
Copy link

@otakar-trunda otakar-trunda left a comment

Choose a reason for hiding this comment

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

I left two minor comments, otherwise it looks good.

docs/prover/cli/options.md Outdated Show resolved Hide resolved
docs/prover/cli/options.md Show resolved Hide resolved
@mdgeorge4153 mdgeorge4153 changed the title Document control flow splitting [DOC-351] Document control flow splitting Dec 12, 2023
Copy link
Contributor

@mdgeorge4153 mdgeorge4153 left a comment

Choose a reason for hiding this comment

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

Looking good! There's a bit much low-level detail, but I think that's unavoidable for this information, and I think it is well scoped. I have some editorial comments.

docs/prover/techniques/index.md Outdated Show resolved Hide resolved
docs/prover/techniques/index.md Outdated Show resolved Hide resolved
splittings along a fixed sequence of split points up to the maximum splitting
depth. We call the splits at maximum splitting depth *split leafs*. The
exploration stops in either of the following three cases:
- if one split was found that is SAT (reasoning: if one split is SAT, then the
Copy link
Contributor

Choose a reason for hiding this comment

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

I think SAT and UNSAT require some mental gymnastics. I think something like "if a counterexample is found for any split, then the process stops" would be clearer.

Copy link
Contributor Author

@alexandernutz alexandernutz Dec 18, 2023

Choose a reason for hiding this comment

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

the problem is that it's not always a counterexample, sometimes it's a "witness example" ...
-- so I chose SAT because it's the more "stable" concept here ...
I made a glossary entry explaining the background there.

Not needing the mental gymnastics is of course better 🤔

I can try how it looks with "counterexample", and on first mention, or in a footnote, mention witness examples and SAT/UNSAT, pointing to the glossary ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, after revisiting this again, I feel like for this very technical section, SAT and UNSAT might be best, since they're just the most concise thing to put e.g. into the pseudo code ...
-- there is a glossary entry for them, which also relates them to the concepts of counterexample and witness example.

I'll also add a sentence to the top of this section mentioning this relationship
-- let me know if you think this won't do ...

Splitting is best illustrated using the {term}`control flow graph` (CFG) of a given
CVL rule.

A single splitting step proceeds as follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a high-level idea that might get lost in the details. Essentially whenever we find a branch (e.g. an if statement or loop guard), we sometimes look for counterexamples in each branch separately. We could also include a small example, something like:

For example, consider a rule with an `if` statement:
```cvl
rule example {
  ...
  if (owner == spender) {
    assert balance_after == balance_before;
  } else {
    assert balance_after == balance_before + amount;
  }
}
```
To simplify the search for a counterexample, the Prover may internally split this single rule into two rules:
```cvl
rule example_split_1 {
  ...
  require owner == spender;
  assert balance_after == balance_before;
}

rule example_split_2 {
  ...
  require owner != spender;
  assert balance_after == balance_before + amount;
}
```

A counterexample for either of the split rules will also be a counterexample for the original rule, and any counterexample for the original rule must violate one of the two split rules, so this splitting doesn't change the meaning of the rule.  However, in some cases the split rule is easier for the Prover to reason about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, making this into an intro section -- should be nice!


(storage-and-memory-analysis)=
# Storage and Memory Analysis
# Storage and Memory analysis

The Certora Prover works on EVM bytecode as its input. To the bytecode, the
address space of both Storage and Memory are flat number lines. That two
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think Storage and Memory should be capitalized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea of capitalizing was to indicate that I'm referring to the terms as defined by EVM, not to the general concepts (in which case I wouldn't be sure what the difference between the two even is exactly) ...

Maybe I make them glossary references instead of capitalizing (first occurrence at least .. make new glossary entries, too, if necessary)?..
I'll try that for now, open to comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll use the terms "EVM storage" and "EVM memory" instead of the capitalized Storage and Memory ..

the other hand, if something will be split further anyway, this can save the
run time spent on intermediate "TIMEOUT" results. Use
{ref}`-smt_initialSplitDepth` to eliminate that time investment altogether up to
a given depth.
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably doesn't belong right here (but maybe in the main page on splitting), but this reminds me: Is there a way to know how much time was spent on different splits to help with this kind of tuning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is on my list for things to implement in the new "Live Stats" tab. -- once we have that, we should document it here.




% The regular SMT timeout determines how much time is maximally spent on checking
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why this is commented out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed and added an extra sentence to the description of -mediumTimeout right above

is enough that one split is UNSAT.
**What does it do?**

We can tell the Certora Prover to continue even when the first split has had a
Copy link
Contributor

Choose a reason for hiding this comment

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

as above, split should be a glossary link 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.

right, so I'm linking every first occurrence under each option ... (or other "entry point")

**What does it do?**

We can tell the Certora Prover to continue even when the first split has had a
maximum-depth timeout. Note that this is only useful for {term}`SAT result`s,
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, I think SAT / UNSAT is almost always more confusing for users than something like "finds a counterexample" / "successfully proves".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, rewrote for counterexample / witness example ... clunky as well, but perhaps clearer ..

@@ -793,29 +793,117 @@ Advanced options that control control flow splitting

See [here](control-flow-splitting) for an explanation of control flow splitting.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would shorten "Advanced options that control control flow splitting" to "Control flow splitting options". It also looks like the header levels are inconsistent; take a look at the generated table of contents at the top of the page.

Copy link
Contributor Author

@alexandernutz alexandernutz Jan 12, 2024

Choose a reason for hiding this comment

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

header: right, much better ...

hm, the toc looks okay-ish to me (maybe something changed in the meantime ..) -- "Control flow splitting options" is on one level with the other groups ("Options that control the soldiity compiler", "Advanced Options", etc .. though whether Advanced options is a great category might be subject for discussion ..)

@alexandernutz alexandernutz requested review from mdgeorge4153 and shellygr and removed request for mdgeorge4153 January 12, 2024 17:02
Copy link
Contributor

@shellygr shellygr left a comment

Choose a reason for hiding this comment

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

Great! nits

@@ -949,7 +953,7 @@ This option disables the storage splitting optimization.


(--allow_solidity_calls_in_quantifiers)=
### --allow_solidity_calls_in_quantifiers
### `--allow_solidity_calls_in_quantifiers`
Copy link
Contributor

Choose a reason for hiding this comment

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

an aside - if you're faster than me... can you fix to --prover_args instead of --settings in line 938 --settings -smt_groundQuantifiers=false? just noticed it's out of date...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, the accent seems to be on master already; changed --settings (in separate branch, PR upcoming ..)

**When to use it?**

When the deepest {term}`split`s are too heavy to solve, but not too high in
number, increasing this will lead to smaller, but more {term}`split leaves`, which run
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
number, increasing this will lead to smaller, but more {term}`split leaves`, which run
number, increasing this will lead to smaller, but more numerous {term}`split leaves`, which run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rright 💯

number, increasing this will lead to smaller, but more {term}`split leaves`, which run
at the full SMT timeout (as set by {ref}`--smt_timeout`). Conversely, if run
time is too high because there are too many splits, decreasing this number means
that more time is spent on fewer, but bigger split leaves.
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. do we have a tip on when to increase or decrease?
  2. should we state the current default (10)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re 1: well .. this is supposed to be it ... but it's rather abstract, I admit .. I'll give it another shot
re 2: sure (increased maintenance effort / failure potential is a con, but I guess I'll add it and see how it goes ..)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, actually, I'm still not sure what to write, as changing this should really be a reaction to where CVT is spending its time ... which currently can only be observed in statsdata.json ...
however, there are plans to condense this info to a live stat -- so I propose to update this when introducing these stats.


**Example**

```sh
Copy link
Contributor

Choose a reason for hiding this comment

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

again, maybe mention default

@alexandernutz alexandernutz merged commit 3abf130 into master Jan 26, 2024
2 checks passed
@alexandernutz
Copy link
Contributor Author

ugh, I guess I landed this rashly, will make an extra PR for your nits, Shelly

@alexandernutz alexandernutz deleted the alex/control-flow-splitting branch January 26, 2024 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
existing feature new documentation for an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants