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

Remote read immutables #1220

Open
wants to merge 40 commits into
base: master
Choose a base branch
from
Open

Remote read immutables #1220

wants to merge 40 commits into from

Conversation

jjcnn
Copy link
Contributor

@jjcnn jjcnn commented Jan 17, 2023

This PR implements remote reads of contract parameters.

Coupled with Zilliqa/Zilliqa#3337. The two PRs cannot work separatedly, and must be tested and released together.

Fixes #1035.

@jjcnn jjcnn changed the title emote read immutables Remote read immutables Jan 23, 2023
@jjcnn
Copy link
Contributor Author

jjcnn commented Jan 30, 2023

I am marking this ready for review, though the listed issues are still outstanding.

@jubnzv: Some of these issues relate to the utilities you have written, so there's a good chance you'll be able to deal with them. They can be handled in a separate PR, though.

Open issues:

  • Gas.ml: We currently charge the same amount of gas for remote reads of immutable fields as we do for mutable fields, but this should either be changed to charge an amount corresponding to the size of init.json (this information is currently not available to Gas.ml, and must be fetched through IPC, which is currently not supported), or the logic of remote lookup for immutables should be changed to be a simple lookup in levelDB (this requires a database migration, since immutables are currently not stored in LevelDB).
  • Cashflow.ml: Does not handle fields declared in address types, and hence does not distinguish between mutables and immutables. (I'm beginning to think the cashflow analyser has rusted beyond the point where it can be saved, so I would suggest retiring it rather than fixing it).
  • DeadCodeDetector: get_used_address_fields, get_addr_fields, check_contract_address_types_in_params, update_contract_params_map, collect_contract_address_types_in_adts need to distinguish between mutable and immutable fields.
  • Formatter.ml: Remote reads of immutables has been implemented, but is yet untested.
  • Merge.ml: Remote reads of immutables has been implemented, but is yet untested.
  • SyntaxAnnotMapper.ml: addr_kind needs to be checked for correctness. My guess is that map_id needs to take mutability into account.
  • Disambiguator.ml: This is the utility that was written as a one-use tool for migrating the levelDB database when we needed to introduce fully qualified names (disambiguation). Since this particular migration never needs to happen again the logic in the file has not been maintained, so the only changes we have made have been to make sure the code still compiles. In general, the code still assumes that all remote fields are mutable, which is no longer the case.

LevelDB migration:

  • In order to fix the issue with Gas.ml (mentioned above) you would need to perform a database migration of contract states. The migration should read all values of contract parameters from the contract's init.json, and store them in LevelDB.
  • Note that immutables and mutables may have the same name, so immutables should be stored using suitable key, e.g., <cparam>.field_name.
  • Note that it is currently impossible to perform an update of immutables through IPC - the OCaml side can't request it (is_mutable is always true), and the C++ side can't perform it (is_mutable is always assumed to be true). For this migration to take place we would need to be able to update immutables through IPC.
  • Once the migration has happened, scilla-runner should store immutables in LevelDB whenever a contract is deployed. Note that this also requires updates of immutables to be supported through IPC. It also allows IPC reads of immutables rather than storing them in the environment, if necessary.

Design decisions that may need to be kept in mind:

  • All special fields (_balance, _this_address, _codehash, _nonce, and _code) are treated as mutables both by the OCaml side and the C++ side.

@jjcnn jjcnn marked this pull request as ready for review January 30, 2023 19:11
@jjcnn
Copy link
Contributor Author

jjcnn commented Jan 30, 2023

Please remember that this PR should not be merged until Zilliqa/Zilliqa#3337 is also ready to be merged.

@jubnzv
Copy link
Contributor

jubnzv commented Feb 26, 2023

To summarize the remaining tasks:

  • Changes in Gas.ml should be additionally discussed and implemented in the Core part first.
  • Add support of remote reads to DeadCodeDetector.
  • Add tests for Formatter. Check that SyntaxAnnotMapper is implemented correctly.
  • Add tests for Merge.

I suggest to consider the Cashflow module as deprecated, because it is not used, and it already has some unimplemented features. For example, it doesn't support procedures with return types.

At the moment I think it will be a good idea to push all the new changes in this branch, without creating separate PRs, since we don't have reviewers for Scilla.

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.

Remote read of contract parameters
2 participants