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

Scratchpad #434

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Scratchpad #434

wants to merge 5 commits into from

Conversation

chfast
Copy link
Member

@chfast chfast commented Sep 26, 2019

The idea behind this is the following:

  1. Scratchpad is a small buffer attached to evmc_result. That's there is up the the creator of the evmc_result object.
  2. The VM can copy small EVM outputs there (no dynamic allocation). Usually solidity functions return nothing or 32 bytes.
  3. This is also the way to deliver "create address" from Client to VM. Union is used because the "create address" and output is never needed at the same time.

@chfast chfast requested review from axic and gumb0 September 26, 2019 15:48
@gumb0
Copy link
Member

gumb0 commented Sep 26, 2019

I think it certainly looks simpler without additional free functions

* Also extends the size of the evmc_result to 64 bytes (full cache line).
*/
uint8_t padding[4];
union evmc_result_scratchpad scratchpad;
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 for a 4 byte saving this complicates matters too much for bindings, such as go or rust.

@chfast
Copy link
Member Author

chfast commented Sep 27, 2019

Risks:

  1. In theory, union memory layout is not binary compatible across different compilers / compilers versions.
  2. Not everything FFI/bindings support unions. E.g. Go will represent this union as bytes.
  3. Using the same memory space for different purposes.

A more conservative variant is to place the scratchpad after create_address.

@chfast
Copy link
Member Author

chfast commented Sep 28, 2019

Update version: create_address separated from the scratchpad.

Still union is used - this is the easiest way to make sure the scratchpad is aligned memory (fix needed for 32-bit arch). However, using unions still might be not ideal for bindings.

Alternatively, we can define scratchpad as uint8_t scratchpad[32 + 4] and then add helpers like:

uint64_t* get_scratchpad_as_words(result*)
{
    (uint64_t*)&result->scratchpad[4];
}

@axic
Copy link
Member

axic commented Sep 28, 2019

Added the size tests to Rust too.

@axic
Copy link
Member

axic commented Nov 5, 2019

@chfast how about this PR? The current version seems to be good, at least from the C and Rust perspectives.

@chfast
Copy link
Member Author

chfast commented Nov 5, 2019

Not now. I have some other ideas in this subject.

@atoulme atoulme closed this Jan 21, 2022
@chfast chfast reopened this Jan 22, 2022
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.

4 participants