fix(protocol): tstore is not suppported on L2 now

[dantaik]

According to David C, L2 is not on the same hardfork.
We can somehow use tstore for L1 contracts. But not sure what's the best way of doing it.

See foundry-rs/foundry#6099

Summary by CodeRabbit

• Refactor
  • Renamed and updated logic for reentry handling in OwnerUUPSUpgradable.sol.
  • Introduced a new constant and updated context functions in Bridge.sol for improved functionality.
• Chores
  • Enhanced deployment script with EVM version specification for better compatibility and testing.
• New Features
  • Adjusted contract configuration generation to include new property settings, enhancing the contract's capabilities.

[coderabbitai]

Walkthrough

The changes across the files aim to enhance the protocol's functionality and deployment process. Variable names are refined, storage operations are optimized, and deployment configurations are adjusted to improve efficiency and maintainability based on blockchain chain IDs.

Changes

File Path	Change Summary
.../OwnerUUPSUpgradable.sol, .../bridge/Bridge.sol	Renamed _reentryDeprecated to _reentry; Updated logic in _storeReentryLock and _loadReentryLock functions. Introduced conditional checks based on block.chainid.
.../bridge/Bridge.sol	Introduce constant PLACEHOLDER for transient storage. Update context function to handle PLACEHOLDER. Modify _resetContext, _storeContext, and _loadContext to conditionally handle logic based on blockchain chain ID.

🐇🌟
Amongst the bytes and lines so fine,
A rabbit danced, its work align.
Variables shifted, logic refined,
Code now sings, a melody defined.
With each hop, a clearer scheme,
Simplified, a developer's dream.
🌌🐾

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

---

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit-tests for this file.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit tests for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository from git and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit tests.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

[coderabbitai]

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between a3a12de and 43737d7.

Files selected for processing (1)

• packages/protocol/contracts/common/OwnerUUPSUpgradable.sol (2 hunks)

Additional comments: 1

packages/protocol/contracts/common/OwnerUUPSUpgradable.sol (1)

• 97-100: The updated _storeReentryLock function directly assigns the reentry parameter to the _reentry variable, removing the need for assembly. This simplification improves readability and safety. However, ensure that the removal of assembly code does not impact the intended low-level functionality, especially in the context of L2 solutions.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 43737d7 and 8e53b15.

Files ignored due to path filters (2)

• packages/protocol/foundry.toml is excluded by: !**/*.toml
• packages/protocol/package.json is excluded by: !**/*.json

Files selected for processing (3)

• packages/protocol/contracts/common/OwnerUUPSUpgradable.sol (2 hunks)
• packages/protocol/genesis/generate_genesis.test.sh (1 hunks)
• packages/protocol/utils/generate_genesis/taikoL2.ts (5 hunks)

Files skipped from review due to trivial changes (1)

• packages/protocol/genesis/generate_genesis.test.sh

Files skipped from review as they are similar to previous changes (1)

• packages/protocol/contracts/common/OwnerUUPSUpgradable.sol

Additional comments: 5

packages/protocol/utils/generate_genesis/taikoL2.ts (5)

• 302-302: The addition of _reentry: 1 is intended to indicate a false state for reentrancy checks. Confirm that the value 1 correctly represents the intended false state in the context of the contract's logic.
• 337-337: Similar to the previous comment, verify that _reentry: 1 accurately reflects the intended logic, especially since it's being used to indicate a non-reentrant state.
• 372-372: Ensure that the use of _reentry: 1 for the ERC721Vault contract aligns with the contract's reentrancy guard logic and that 1 is the correct representation of the non-reentrant state.
• 407-407: For the ERC1155Vault contract, confirm that _reentry: 1 is the appropriate value to signify the non-reentrant state, consistent with the contract's reentrancy guard mechanism.
• 466-466: Lastly, verify the correctness of _reentry: 1 for the SignalService contract, ensuring it properly represents the non-reentrant state as intended by the contract's logic.

[Brechtpd]

• Would it be possible to switch on block.chainid == 1 to enable/disable the use of the tstore version? Because we'll want to support cancun as soon as possible on L2 I assume (even for mainnet), so having both code paths running seems like a good test? (and this code path being part of the audit as well)
• Bridge.sol also uses transient storage for the context, so will also have to be updated to remove it/conditionally disable it.

[dantaik]

• block.chainid == 1

A good idea, but I wonder if it will work.
Will give it a try and let Dave to test.

[davidtaikocha]

• Would it be possible to switch on block.chainid == 1 to enable/disable the use of the tstore version? Because we'll want to support cancun as soon as possible on L2 I assume (even for mainnet), so having both code paths running seems like a good test? (and this code path being part of the audit as well)
• Bridge.sol also uses transient storage for the context, so will also have to be updated to remove it/conditionally disable it.

I dont think #15810 will work, since the compiled bytecode will still contain the TSTORE opcode... right?

[Brechtpd]

I dont think #15810 will work, since the compiled bytecode will still contain the TSTORE opcode... right?

Yes but that is currently okay right I think? Because currently the code is not checked yet when it is deployed (maybe in the future it will), so if some random opcode/byte value is part of the bytecode but it is not executed, I think it should be fine.

[dantaik]

I don't think it will work. Please see foundry-rs/foundry#6099

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 8e53b15 and 8d72475.

Files selected for processing (2)

• packages/protocol/script/test_deploy_on_l1.sh (1 hunks)
• packages/protocol/utils/generate_genesis/taikoL2.ts (5 hunks)

Files skipped from review due to trivial changes (1)

• packages/protocol/script/test_deploy_on_l1.sh

Files skipped from review as they are similar to previous changes (1)

• packages/protocol/utils/generate_genesis/taikoL2.ts

[dantaik]

@Brechtpd I think for Bridge and OwnerUUPSUpgradable.sol, maybe we can extract the code that use transient storage into a small solidity file respectively then have a non-transient version. Then we write a preprocessing script to swap versions by renaming files, based on an env var.

@davidtaikocha do you think that will work?

[Brechtpd]

Note that code compiled with this flag may not work as desired on any network where the hard fork is not yet implemented.

It is important to note that at the moment the code generator does not automatically make use of the new opcodes to improve the emitted code and the optimizer support is minimal. Code generation and optimizer improvements will follow in subsequent releases.

So currently it should be fine to do it! But future versions of solidity it may be a problem.

[dantaik]

OK, seems we have a solution but PR still needs to be updated to change the Bridge.

• we keep two implementation, one with transient storage and one without, using 'block.chainid == 1' as the condition.
• For L2 contracts, we use cancun to compile, but the transitent storage code will never run, so it should be fine.

Brecht, if you think this is a good solution, could you also modify the Bridge code?

[Brechtpd]

@Brechtpd I think for Bridge and OwnerUUPSUpgradable.sol, maybe we can extract the code that use transient storage into a small solidity file respectively then have a non-transient version. Then we write a preprocessing script to swap versions by renaming files, based on an env var.

How long do we think it will take to update our L2 to cancun? Is the plan to do it before mainnet or not? If we will do it for mainnet, then maybe this seems more work than necessary, and the conditional trick should be okay I believe. If not, than indeed some longer term solution needs to be found and then something like this seems like a reasonable approach. Or maybe just use inheritance and have different contracts for L1 and L2?

[davidtaikocha]

I dont think #15810 will work, since the compiled bytecode will still contain the TSTORE opcode... right?

Yes but that is currently okay right I think? Because currently the code is not checked yet when it is deployed (maybe in the future it will), so if some random opcode/byte value is part of the bytecode but it is not executed, I think it should be fine.

@Brechtpd I think for Bridge and OwnerUUPSUpgradable.sol, maybe we can extract the code that use transient storage into a small solidity file respectively then have a non-transient version. Then we write a preprocessing script to swap versions by renaming files, based on an env var.

@davidtaikocha do you think that will work?

Tested in go-ethereum, it works. (compiled the code by a cancun compiler, then let it run inside a shanghai evm)

[Brechtpd]

Brecht, if you think this is a good solution, could you also modify the Bridge code?

Okay will do!

[dantaik]

@Brechtpd I think for Bridge and OwnerUUPSUpgradable.sol, maybe we can extract the code that use transient storage into a small solidity file respectively then have a non-transient version. Then we write a preprocessing script to swap versions by renaming files, based on an env var.

How long do we think it will take to update our L2 to cancun? Is the plan to do it before mainnet or not? If we will do it for mainnet, then maybe this seems more work than necessary, and the conditional trick should be okay I believe. If not, than indeed some longer term solution needs to be found and then something like this seems like a reasonable approach. Or maybe just use inheritance and have different contracts for L1 and L2?

I would say later, definitely not now.

[Brechtpd]

@Brechtpd I think for Bridge and OwnerUUPSUpgradable.sol, maybe we can extract the code that use transient storage into a small solidity file respectively then have a non-transient version. Then we write a preprocessing script to swap versions by renaming files, based on an env var.

How long do we think it will take to update our L2 to cancun? Is the plan to do it before mainnet or not? If we will do it for mainnet, then maybe this seems more work than necessary, and the conditional trick should be okay I believe. If not, than indeed some longer term solution needs to be found and then something like this seems like a reasonable approach. Or maybe just use inheritance and have different contracts for L1 and L2?

I would say later, definitely not now.

Okay, then conditional trick and just sticking to 0.8.24 compiler is actually fine as well. We can see with future compiler versions if there would be problems.

[Brechtpd]

Bridge updated

[coderabbitai]

Review Status

Actionable comments generated: 4

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 8d72475 and bc50c15.

Files selected for processing (2)

• packages/protocol/contracts/bridge/Bridge.sol (3 hunks)
• packages/protocol/contracts/common/OwnerUUPSUpgradable.sol (2 hunks)

Additional comments: 4

packages/protocol/contracts/bridge/Bridge.sol (4)

• 53-54: Introduction of PLACEHOLDER constant is a good practice for clarity when indicating non-use of transient storage. Ensure its usage is consistent across the contract.
• 448-448: The check in context function using PLACEHOLDER ensures invalid contexts are correctly identified. This is a good use of the newly introduced constant.
• 548-552: The conditional logic in _resetContext based on block.chainid is clear and maintains separation between L1 and L2 logic. However, ensure that the use of PLACEHOLDER for non-L1 chains aligns with the contract's overall design and security model.
• 570-581: Similar to _storeContext, the _loadContext function uses conditional logic based on block.chainid effectively. Ensure that the separation of logic does not introduce any unintended side effects.