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!: Revise VM storage interface #73

Merged
merged 4 commits into from
Sep 30, 2024

Conversation

slowli
Copy link
Contributor

@slowli slowli commented Sep 26, 2024

What ❔

Replaces storage slot data returned by StorageInterface with a dedicated type (essentially, (U256, bool)).

Why ❔

Currently, StorageInterface reads storage slots as Option<U256> where None corresponds to initial writes. This loses information during sandboxed execution. Indeed, there may be slots which have non-zero value, but still are initial writes (i.e., these slots were first written to during previous blocks / transactions in the sandboxed batch).

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted via cargo fmt and cargo clippy.

Slot initialness and its value are not tied
during sandboxed VM execution. E.g., a slot may be initial
while having a non-zero value.
crates/vm2/src/world_diff.rs Show resolved Hide resolved
crates/vm2/src/world_diff.rs Show resolved Hide resolved
@slowli slowli marked this pull request as ready for review September 26, 2024 14:58
@slowli slowli requested a review from joonazan as a code owner September 26, 2024 14:58
@joonazan
Copy link
Contributor

For maximum efficiency with Postgres there'd have to have one method returning (U256, bool) and another returning just U256. I was planning to do that optimization at some point but it is quite complex, so I think it might not be worth it.

I would write the integration of this so that RocksDB/hashmap cache is only queried once per method call. That would help performance a lot, at least according some benchmark flamegraphs.

@slowli
Copy link
Contributor Author

slowli commented Sep 27, 2024

For maximum efficiency with Postgres there'd have to have one method returning (U256, bool) and another returning just U256.

That's what I've implemented 🙃 I agree that it looks like the most efficient approach. Regarding changes in the Era codebase, that's a separate topic that we can approach iteratively.

@slowli slowli merged commit a233d44 into master Sep 30, 2024
9 checks passed
@slowli slowli deleted the aov-pla-1036-revise-vm-storage-interface branch September 30, 2024 08:45
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.

2 participants