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

improve reentrancy guard #62

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

improve reentrancy guard #62

wants to merge 6 commits into from

Conversation

0age
Copy link
Contributor

@0age 0age commented Mar 9, 2024

No description provided.

Comment on lines +170 to +172
// Retrieve the reentrancy guard sentinel value.
let reentrancyGuard := sload(_REENTRANCY_GUARD_SLOT)

Copy link
Collaborator

@Saw-mon-and-Natalie Saw-mon-and-Natalie Mar 12, 2024

Choose a reason for hiding this comment

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

These sloads get hoisted out of the for loop by the LoopInvariantCodeMotion yul optimization since its value is independent of each iteration:

For example the compiled ir code for assertAcceptingNativeTokens() looks like:

function fun_assertAcceptingNativeTokens()
{
  /// @src 51:14266:14287  "_tstoreInitialSupport"
  let _1 := loadimmutable("53400")
  /// @src 51:14377:18103  "assembly {..."
  let _2 := 2459889172
  let usr$reentrancyGuard := sload(_2)
  let _3 := iszero(usr$reentrancyGuard)
  let _4 := iszero(eq(usr$reentrancyGuard, 3))
  for { } 1 { }
  {
      if _1
      {
          if iszero(eq(tload(_2), 2))
          {
              mstore(0, 2786847216)
              mstore(32, callvalue())
              revert(28, 36)
          }
          break
      }
      if _3
      {
          if iszero(eq(tload(_2), 2))
          {
              mstore(0, 2786847216)
              mstore(32, callvalue())
              revert(28, 36)
          }
          break
      }
      if _4
      {
          mstore(0, 2786847216)
          mstore(32, callvalue())
          revert(28, 36)
      }
      break
  }
}

So sload will be used even if transient storage is already available during deployment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment applies to other for loops of similar form in this and other contracts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like setReentrancyGuard(...) is not affected but we have 2 yul functions fun_setReentrancyGuard_X() and fun_setReentrancyGuard_Y() where _ENTERED_XSTORE is specialised. So in total 3 setReentrancyGuard(...) functions. This probably comes from:

  1. setReentrancyGuard(false) in _prepareBasicFulfillmentFromCalldata
  2. setReentrancyGuard(true) in _validateOrdersAndPrepareToFulfill

Copy link
Collaborator

Choose a reason for hiding this comment

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

The specialisation might make the runtime code a bit cheaper, but it would increase the deployment gas cost and deployed byte code size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do we have a way to coerce the compiler to avoid the sload when not required here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed a fix for the hoisting issue and verified against the IR output; given we have the runtime code headroom we may as well leave the specialization in for now @Saw-mon-and-Natalie

2c7e977

Copy link
Collaborator

Choose a reason for hiding this comment

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

@0age another solution would have been to remove that optimisation step from the optimisation sequence LoopInvariantCodeMotion M

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.

3 participants