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-367] persistent ghosts explained with examples #177

Merged
merged 3 commits into from
Dec 25, 2023

Conversation

shellygr
Copy link
Contributor

@shellygr shellygr commented Dec 5, 2023

Before requesting review:

  • Make sure there is a ticket in the DOC board in Jira
  • Make sure CI is passing
    • Spell check failure may require adding backticks around code or updating spelling_wordlist.txt
    • See README.md for information about style and markdown syntax
    • If the CI Details link gives a 404, you need to log in to readthedocs.com
  • Add link to generated documentation here
    • you can find this by following the read the docs link from the CI check
  • Ask for help in #documentation

Jira ticket: TODO
Link to generated documentation: TODO

@shellygr shellygr added the future documentation for features that haven't landed yet label Dec 5, 2023
@shellygr shellygr requested a review from a team as a code owner December 5, 2023 17:25
docs/cvl/ghosts.md Show resolved Hide resolved
docs/cvl/ghosts.md Outdated Show resolved Hide resolved
docs/cvl/ghosts.md Outdated Show resolved Hide resolved
docs/cvl/ghosts.md Outdated Show resolved Hide resolved
1. A call to `a.transfer` which cannot be resolved and results in a havoc operation. Non-persistent ghosts are havoced, in particular `reentrancy_happened`.
2. A `CALL` hook executes, updating `reentrancy_happened` based on its havoced value, meaning it can turn to true.

Therefore even if the addresses of `a` and `NotReentrant` are distinct, we could still falsely detect a reentrant call as `reentrancy_happened` was set to true due to non-determinism.
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 this is pretty opaque. Worth expanding on why this can happen:

"During a havoc operation, the prover conservatively assumes that almost any possible behavior can occur. In particular, it must assume that during the execution of the transfer call, some reentrancy occurs, and thus considers the case where reentrancy_happened is set to true. Thus, when the CALL hook executes, it does so where the reentrancy_happened value is already true, and thus the value after the hook will remain true."

or something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reentrancy actually cannot occur. It does not matter because reentrancy_happened is in some state we track based on contracts, not tied to a particular contract. I added the wordier explanation, although for me it's harder to grasp

docs/cvl/ghosts.md Outdated Show resolved Hide resolved

It is expected that the method `userDefinedRequireMsg` and only it will satisfy the rule.
Copy link
Contributor

Choose a reason for hiding this comment

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

"it is expected that the method userDefinedRequireMsg..." is not a complete fragment. I think you forgot a verb?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"will satisfy". tried to reword

docs/cvl/ghosts.md Outdated Show resolved Hide resolved
It is expected that the method `userDefinedRequireMsg` and only it will satisfy the rule.
However, if we use regular ghosts, the ghost variable `saw_user_defined_revert_msg` will revert in case the input argument `a` is equal to 0, together with the contract itself.
This means the value of `saw_user_defined_revert_msg` will remain false, and thus the `satisfy` statement will fail.
With regular ghosts, the rule would of course fail also for the two other methods, meaning that non-persistent ghosts are not suitable for distinguishing between different reverts.
Copy link
Contributor

Choose a reason for hiding this comment

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

I've read this so many times, and I still have no idea what it's saying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried to reword

@mdgeorge4153 mdgeorge4153 changed the title persistent ghosts explained with examples [DOC-367] persistent ghosts explained with examples Dec 12, 2023
@shellygr shellygr requested a review from jtoman December 25, 2023 14:51
@shellygr shellygr merged commit b263e5a into master Dec 25, 2023
2 checks passed
@shellygr shellygr deleted the shelly/persistentghosts branch December 25, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future documentation for features that haven't landed yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants