jjcnn
commented
1 year ago 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_mutableis alwaystrue), and the C++ side can't perform it (is_mutableis always assumed to betrue). For this migration to take place we would need to be able to update immutables through IPC. - Once the migration has happened,
scilla-runnershould 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.
jubnzv
This PR implements remote reads of contract parameters.
Coupled with https://github.com/Zilliqa/Zilliqa/pull/3337. The two PRs cannot work separatedly, and must be tested and released together.
Fixes #1035.