Skip to content

Introduce transient storage support for inline assembly#14737

Merged
cameel merged 1 commit intodevelopfrom
transientStorageBasicSupport
Jan 25, 2024
Merged

Introduce transient storage support for inline assembly#14737
cameel merged 1 commit intodevelopfrom
transientStorageBasicSupport

Conversation

@matheusaaguiar
Copy link
Contributor

@matheusaaguiar matheusaaguiar commented Dec 18, 2023

This PR introduces basic support for the transient storage opcodes TSTORE and TLOAD for inline assembly only.
Based on previous groundwork by @hrkrshnn (Thank you 🙏).
Part of #14739

@matheusaaguiar matheusaaguiar force-pushed the transientStorageBasicSupport branch 3 times, most recently from 36ce9cb to 69fabc5 Compare December 18, 2023 20:16
logTrace(_instruction, arg);
return 0;
case Instruction::TLOAD:
return m_state.storage[h256(arg[0])];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will need as separate key-value store in m_state, i.e. you'll need to add
std::map<util::h256, util::h256> triansientStorage; to InterpreterState. Also a quick look at whether we do anything specially for InterpreterState::storage would be good (e.g. if we reset it somewhere). Eventually for actual fuzzing/tracing, we will need an analog of InterpreterState::dumpStorage(std::ostream& _out) const; as well, but we can add that in a separate PR with fuzzing support for transient storage (@bshastry may want to get to that eventually - in that PR we can also check if we need to do anything further to account for the special lifetime of transient storage in the fuzzing setup)

@ekpyron
Copy link
Collaborator

ekpyron commented Dec 18, 2023

I just had another quick look through this, and we should still really have some semantics tests for this.

The easiest one would be a simple reentrancy lock, so something like:

contract C {
  modifier nonreentrant {
    assembly {
      if tload(0) { revert(0, 0) }
      tstore(0, 1)
    }
    _;
   assembly {
      tstore(0, 0)
   }
  }
  function f(bool simulateReentrancy) nonreentrant public {
    if (simulateReentrancy) {
       f(false);
    }
  }
}
// ----
// f(bool): false ->
// f(bool): true -> FAILURE

For more complex cases, like testing the behaviour across multiple transactions, we'd actually need to see how evmone handles this right now. So I'm actually not sure what the following test would do currently:

contract C {
   function f() external { assembly { tstore(0, 42) } }
   function g() external returns(uint r) { assembly { r := tload(0) } }
}
// ----
// f() ->
// g() -> which is it, 0 or 42? depends on whether evmone and the way we call this consider these calls separate transactions

Also a few of other sanity tests (showing collision-freeness with memory and storage and such) may be nice.
I mean, we're more testing evmone here than us, but still we should have these tests to see if anything breaks (it could also be us assigning incorrect opcode values or such, e.g. by accident swapping tstore and tload - or not accounting for an opcode reassignment since Hari's draft etc)

tstore(0, 21)
mstore(0, 42)
sstore(0, 42)
if iszero(eq(tload(0), 21)) {
  revert(0, 0)
}

@ekpyron

This comment was marked as resolved.

@matheusaaguiar matheusaaguiar force-pushed the transientStorageBasicSupport branch from b0b2f80 to 5e3c8d8 Compare December 19, 2023 02:39
@ekpyron ekpyron mentioned this pull request Dec 20, 2023
9 tasks
@ekpyron ekpyron added this to the 0.8.24 milestone Dec 20, 2023
@ekpyron

This comment was marked as resolved.

@ekpyron

This comment was marked as resolved.

@matheusaaguiar matheusaaguiar force-pushed the transientStorageBasicSupport branch 2 times, most recently from 4659659 to d3f0271 Compare December 22, 2023 05:29
Copy link
Contributor

@nikola-matic nikola-matic left a comment

Choose a reason for hiding this comment

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

This is gonna needs docs and a Changelog entry as well.

@cameel cameel force-pushed the transientStorageBasicSupport branch from d3f0271 to 461523a Compare January 8, 2024 13:36
@cameel
Copy link
Collaborator

cameel commented Jan 8, 2024

I rebased the PR on develop and added a fixup with some smaller tweaks that weren't worth a comment.

@matheusaaguiar matheusaaguiar force-pushed the transientStorageBasicSupport branch from 4a440c8 to dc62b1d Compare January 9, 2024 21:11
@matheusaaguiar matheusaaguiar force-pushed the transientStorageBasicSupport branch from dc62b1d to db30a43 Compare January 10, 2024 03:54
@matheusaaguiar matheusaaguiar force-pushed the transientStorageBasicSupport branch 2 times, most recently from 2eefa79 to 35fe148 Compare January 11, 2024 03:05
@cameel cameel force-pushed the transientStorageBasicSupport branch 3 times, most recently from 31de262 to 185bd64 Compare January 19, 2024 18:28
@matheusaaguiar matheusaaguiar force-pushed the transientStorageBasicSupport branch 2 times, most recently from d1f8767 to dc4b94e Compare January 22, 2024 18:15
@matheusaaguiar matheusaaguiar force-pushed the transientStorageBasicSupport branch from 3764555 to d3682a7 Compare January 22, 2024 18:48
@matheusaaguiar matheusaaguiar force-pushed the transientStorageBasicSupport branch 2 times, most recently from 74da68c to 3638d7a Compare January 24, 2024 01:37
Copy link
Collaborator

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Looks like #14737 (comment) and #14737 (comment) are the only remaining things here. Once those are taken care of I'm going to approve it.

Copy link
Contributor

@nikola-matic nikola-matic left a comment

Choose a reason for hiding this comment

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

There are a few optimizer tests missing, but otherwise looks good.

@cameel
Copy link
Collaborator

cameel commented Jan 24, 2024

I just merged MCOPY PR so this needs conflict resolutions.

@matheusaaguiar matheusaaguiar force-pushed the transientStorageBasicSupport branch from b8f0d28 to 28e286a Compare January 24, 2024 19:39
@matheusaaguiar
Copy link
Contributor Author

Rebased and squashed commits.

Copy link
Collaborator

@cameel cameel left a comment

Choose a reason for hiding this comment

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

ok, looks like we're done here. Please squash and we can merge.

@matheusaaguiar matheusaaguiar force-pushed the transientStorageBasicSupport branch from 37b2751 to 89b73b6 Compare January 24, 2024 22:10
@cameel cameel merged commit 7e7c45c into develop Jan 25, 2024
@cameel cameel deleted the transientStorageBasicSupport branch January 25, 2024 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

6 participants