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

Contract storage fields in CSE #571

Closed
wants to merge 18 commits into from
Closed

Contract storage fields in CSE #571

wants to merge 18 commits into from

Conversation

anvacaru
Copy link
Contributor

@anvacaru anvacaru commented May 17, 2024

Blocked on : #570 to bring in the changes from runtimeverification/evm-semantics#2433 to clean some of the code first.

First PR for #362:

  • extending the StorageField class to store the numberOfBytes property
  • implementing _create_initial_storage in prove.py that creates the KAst representation of the storage layout for a given set of storage fields.
    This representation is used to populate the storage of CONTRACT_ID symbolic account in CSE only.
    For now supporting just uint*, address and bool

@anvacaru anvacaru self-assigned this May 17, 2024
@anvacaru anvacaru requested review from PetarMax, tothtamas28 and palinatolmach and removed request for PetarMax May 20, 2024 06:41
@anvacaru anvacaru added enhancement New feature or request cse labels May 20, 2024
@PetarMax
Copy link
Contributor

There is a problem with test_add_const in CSE, it would appear the summaries are not getting used. Investigating now.

@PetarMax PetarMax added the do not merge Do not merge this PR while label present label May 24, 2024
┃ │ (464 steps)
┃ └─ 12 (vacuous, leaf)
┃ k: #execute ~> #return 128 0 ~> #pc [ CALL ] ~> #execute ~> CONTINUATION:K
┃ │ (979 steps)
Copy link
Contributor

Choose a reason for hiding this comment

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

This increase is due to a problem with the specification of setConst.

│ (1123 steps)
├─ 13 (terminal)
│ (1417 steps)
Copy link
Contributor

Choose a reason for hiding this comment

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

This increase is due to a problem with the specification of setConst.

@PetarMax
Copy link
Contributor

We have a problem. By setting the storage in this way, we cannot use the generated specifications when a slot has not been explicitly initialised---this, for example, is the case for the AddConst.setConst function, where slot 0 holds an uint256 and whose specification has

<storage>
  ( 0 |-> ( SF_C:Int => VV0_x_114b9705:Int ) )
</storage>

but when the CSETest.test_add_const initialises the AddConst contract, the storage is set to

<storage>
  .Map
</storage>

implicitly having all slots set to zero and using #lookup to leverage this implicitness. As a result, the specification of setConst is not used in test_add_const, and this is visible in the increase of steps in the expected outpute here and here.

I have two solutions in mind, first more favourable than the second, I think:

  1. We do not create an explicit storage layout, but instead have a fully symbolic storage and work in terms of #lookup and &Int / >>Int masks and shifts, just like the compiler does. For the setConst contract, this would mean just introducing the constraints 0 <=Int #lookup(STORAGE, 0) andBool #lookup(STORAGE, 0) <Int pow256, which actually hold by construction, and the specification would have <storage> STORAGE => STORAGE [ 0 <-| VV0_x_114b9705:Int ] </storage>. I see the following pros and cons
    • Pros: resilient to presence or absence of explicit fields in storage, can handle all storage constructs;
    • Cons: more complex expressions in the path constraint, hindering readability and potentially slowing down execution.
  2. We create an explicit storage layout as much as we can for tests as well. I am not sure how this would be done other than intercepting constructor calls, and it would not be feasible for mappings, which would have to remain implicit in the style of solution 1.

@anvacaru, @ehildenb, @palinatolmach, let's discuss this a bit, please. Other ideas welcome.

@anvacaru
Copy link
Contributor Author

Closing this since we won't merge it.

@anvacaru anvacaru closed this May 24, 2024
Comment on lines +1303 to +1315
def type_constraint(data_type: str, symbolic_var: KVariable) -> KApply:
# TODO: fill with rest of the cases
data_type = data_type.lower()
if data_type.startswith('uint'):
return mlEqualsTrue(KEVM.range_uint(int(data_type[4:]), symbolic_var))

match data_type:
case 'address':
return mlEqualsTrue(KEVM.range_address(symbolic_var))
case 'bool':
return mlEqualsTrue(KEVM.range_bool(symbolic_var))
case _:
raise ValueError(f'Unimplemented: {data_type}')
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: This will be useful for future.

Comment on lines +1303 to +1315
def type_constraint(data_type: str, symbolic_var: KVariable) -> KApply:
# TODO: fill with rest of the cases
data_type = data_type.lower()
if data_type.startswith('uint'):
return mlEqualsTrue(KEVM.range_uint(int(data_type[4:]), symbolic_var))

match data_type:
case 'address':
return mlEqualsTrue(KEVM.range_address(symbolic_var))
case 'bool':
return mlEqualsTrue(KEVM.range_bool(symbolic_var))
case _:
raise ValueError(f'Unimplemented: {data_type}')
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this will be useful in future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cse do not merge Do not merge this PR while label present enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants