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: msg.data mutability #2140

Merged
merged 2 commits into from Feb 15, 2024
Merged

Conversation

charles-cooper
Copy link
Contributor

@charles-cooper charles-cooper commented Feb 14, 2024

this commit modifies msg.data so that it is an immutable bytes copy of the input. the issue is that right now it is a view of memory, which means the caller can trample the msg.data buffer after the call returns (and msg.data gets mutated). this is not necessarily a problem for the VM correctness, but it is an issue for integrators that inspect msg.data after a call, because msg.data will not retain its original value.

as a refactor, since Computation.memory_read() is completely dead (and more importantly, probably a footgun), this commit also removes memory_read() from the ComputationAPI interface as well as the Computation implementation.

What was wrong?

Related to Issue #
Closes #

How was it fixed?

Todo:

Cute Animal Picture

this commit modifies `msg.data` so that it is an immutable `bytes` copy
of the input. the issue is that right now it is a view of memory, which
means the caller can trample the msg.data buffer after the call returns
(and `msg.data` gets mutated). this is not necessarily a problem for the
VM correctness, but it is an issue for integrators that inspect
`msg.data` after a call, because `msg.data` will not retain its original
value.

as a refactor, since `Computation.memory_read()` is completely dead
(and more importantly, probably a footgun), this commit also removes
`memory_read()` from the `ComputationAPI` interface as well as the
`Computation` implementation.
@charles-cooper
Copy link
Contributor Author

we could also get rid of Message.data_as_bytes() since it doesn't do anything additional now besides just return the Message.data bytes object, but it would be a hygiene thing.

$ python
Python 3.10.12 (main, Jul  7 2023, 18:23:44) [GCC 11.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> s = b"12345"
>>> t = bytes(s)
>>> id(t), id(s)
(139843084676256, 139843084676256)

fselmo added a commit to charles-cooper/py-evm that referenced this pull request Feb 15, 2024
Copy link
Collaborator

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

lgtm, thanks 👍🏼

@fselmo fselmo merged commit c901d54 into ethereum:main Feb 15, 2024
45 checks passed
@charles-cooper charles-cooper deleted the fix/msg-data-inspection branch February 18, 2024 14:47
pacrob pushed a commit to charles-cooper/py-evm that referenced this pull request Feb 21, 2024
pacrob added a commit that referenced this pull request Feb 21, 2024
* Fix error in epub docs build, add epub and pdf docs build to CI, delete unused auto-gen docs that appear with `make build-docs`

* set up separate validate-docs-ci make command to avoid heavy local latex reqs

* Compile release notes for v0.9.0-beta.1

* Bump version: 0.8.0-beta.1 → 0.9.0-beta.1

* fix: msg.data mutability

this commit modifies `msg.data` so that it is an immutable `bytes` copy
of the input. the issue is that right now it is a view of memory, which
means the caller can trample the msg.data buffer after the call returns
(and `msg.data` gets mutated). this is not necessarily a problem for the
VM correctness, but it is an issue for integrators that inspect
`msg.data` after a call, because `msg.data` will not retain its original
value.

as a refactor, since `Computation.memory_read()` is completely dead
(and more importantly, probably a footgun), this commit also removes
`memory_read()` from the `ComputationAPI` interface as well as the
`Computation` implementation.

* newsfragment for #2140

* Establish base feature branch for Cancun network upgrade

* Add cancun to tests / helpers

* Fix lint issues

* Create test run for cancun; silence in circleci for now

* Add Cancun network to chain_vm_configuration

* Update Cancun header to include new fields

- Update ``CancunMiningHeader``, ``CancunBlockHeader`` and ``CancunBlock`` to
  include the new fields: ``blob_gas_used``, ``excess_blob_gas``, and ``parent_beacon_block_root``.

- Inherit from ``ShanghaiBackwardsHeader`` and create a ``CanunBackwardsHeader`` that can serialize all known headers.

* newsfragment for #2134

* Get rid of Cancun mining header concept

* Implement most of EIP-4788 - considerations:

- Should we make sure to update the Cancun state with the contract at
  BEACON_ROOTS_ADDRESS if it doesn't exist yet since it is vital to
  this EIP?

* newsfragment for #2135

* If no code exists at the BEACON_ROOTS_ADDRESS, make it so.

---------

Co-authored-by: Charles Cooper <cooper.charles.m@gmail.com>
Co-authored-by: fselmo <fselmo2@gmail.com>
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.

None yet

2 participants