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

Fix error in SMTChecker: Use rich indentifier instead of external identifier to encode member access to functions #14344

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

pgebal
Copy link
Collaborator

@pgebal pgebal commented Jun 21, 2023

Closes #14181

Cause:

  • we use FunctionType::externalIdentifier in SMTEncoder to encode functions, externalIdentifier requires all parameters to have external type. Error happens when function parameter is an internal function

Solution:

  • use a hash of rich identifier instead of external identifier when encoding function in SMTEncoder

@github-actions
Copy link

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

@pgebal pgebal requested a review from leonardoalt June 21, 2023 16:00

function f() public {
function()internal x;
x.value(42);
Copy link
Member

Choose a reason for hiding this comment

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

What exactly was triggering the assertion before, was it x.value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes

Copy link
Member

Choose a reason for hiding this comment

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

I think I still don't fully understand the bug. The MemberAccess in the C++ code has function type, right? But it's not the function type of the variable x, it's the function type of x.value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right. MemberAccess is about the value function. It's the function type of value or x.value function from library.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@leonardoalt I was wrong about that. I updated PR description with hopefully true cause.

Changelog.md Outdated
@@ -20,6 +20,7 @@ Bugfixes:
* Commandline Interface: It is no longer possible to specify both ``--optimize-yul`` and ``--no-optimize-yul`` at the same time.
* SMTChecker: Fix encoding of side-effects inside ``if`` and ``ternary conditional``statements in the BMC engine.
* SMTChecker: Fix false negative when a verification target can be violated only by trusted external call from another public function.
* SMTChecker: Fix internal error when verifying code with internal functions.
Copy link
Member

Choose a reason for hiding this comment

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

@pgebal can you also update the bugfix message here with something more precise?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

…ntifier to ecnode member access to functions
@pgebal pgebal force-pushed the pgebal/fix-ice-internal-type branch from 520e666 to 826fd90 Compare June 23, 2023 13:25
@pgebal pgebal requested a review from leonardoalt June 23, 2023 13:27
@pgebal pgebal changed the title Fix bug in model checker: Use rich identifier instead of external identifier for SMT encoding Fix error in SMTChecker: Use rich indentifier instead of external identifier to encode member access to functions Jun 23, 2023
@cameel cameel added the smt label Jun 23, 2023
@leonardoalt leonardoalt merged commit 2cf03e3 into ethereum:develop Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SMTChecker] ICE because parameter must have external type
3 participants