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

Introduce transient storage support for inline assembly #14737

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

matheusaaguiar
Copy link
Collaborator

@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
@@ -314,6 +314,11 @@ u256 EVMInstructionInterpreter::eval(
accessMemory(arg[0], arg[1]);
logTrace(_instruction, arg);
return 0;
case Instruction::TLOAD:
return m_state.storage[h256(arg[0])];
Copy link
Member

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
Member

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.

@ekpyron ekpyron mentioned this pull request Dec 20, 2023
9 tasks
libyul/AsmAnalysis.cpp Outdated Show resolved Hide resolved
@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
Collaborator

@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.

libevmasm/GasMeter.cpp Outdated Show resolved Hide resolved
libevmasm/Instruction.cpp Outdated Show resolved Hide resolved
libevmasm/SemanticInformation.cpp Show resolved Hide resolved
libyul/backends/evm/EVMDialect.cpp Outdated Show resolved Hide resolved
test/tools/yulInterpreter/Interpreter.h Outdated Show resolved Hide resolved
@cameel cameel force-pushed the transientStorageBasicSupport branch from d3f0271 to 461523a Compare January 8, 2024 13:36
@cameel
Copy link
Member

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.

Changelog.md Outdated Show resolved Hide resolved
@matheusaaguiar matheusaaguiar force-pushed the transientStorageBasicSupport branch 2 times, most recently from 2eefa79 to 35fe148 Compare January 11, 2024 03:05
libevmasm/SemanticInformation.cpp Show resolved Hide resolved
libevmasm/SemanticInformation.h Show resolved Hide resolved
docs/grammar/SolidityLexer.g4 Outdated Show resolved Hide resolved
libyul/AsmAnalysis.cpp Outdated Show resolved Hide resolved
libyul/SideEffects.h Show resolved Hide resolved
libyul/SideEffects.h Show resolved Hide resolved
libyul/backends/evm/EVMDialect.cpp Outdated Show resolved Hide resolved
@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 2 times, most recently from 74da68c to 3638d7a Compare January 24, 2024 01:37
Copy link
Member

@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
Collaborator

@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
Member

cameel commented Jan 24, 2024

I just merged MCOPY PR so this needs conflict resolutions.

@matheusaaguiar
Copy link
Collaborator Author

Rebased and squashed commits.

Copy link
Member

@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.

@cameel cameel merged commit 7e7c45c into develop Jan 25, 2024
65 checks passed
@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.

None yet

6 participants