fix(pg): fix simulation bugs

.changeset/great-ads-rhyme.md

@@ -0,0 +1,6 @@
+---
+'@sphinx-labs/plugins': patch
+'@sphinx-labs/core': patch
+---
+
+Fix simulation bugs

packages/core/src/utils.ts

@@ -1667,4 +1667,39 @@ export const trimQuotes = (str: string): string => {
   return str.replace(/^['"]+|['"]+$/g, '')
 }
 
-export const sphinxCoreUtils = { sleep, callWithTimeout }
+/**
+ * Checks if a given property of an object is a public asynchronous method.
+ *
+ * This function iterates over the prototype chain of the object to check if the specified property
+ * is an asynchronous function that is not intended to be private (not starting with '_'). We check
+ * for a leading underscore to determine whether a function is meant to be private because
+ * JavaScript doesn't have a native way to check this. This function stops the search once it
+ * reaches the top of the prototype chain or finds a match.
+ *
+ * @param {any} obj - The object to inspect.
+ * @param {string | symbol} prop - The property name or symbol to check.
+ * @returns {boolean} - `true` if the property is a public asynchronous method, `false` otherwise.
+ */
+export const isPublicAsyncMethod = (
+  obj: any,
+  prop: string | symbol
+): boolean => {
+  let currentObj = obj
+
+  while (currentObj && currentObj !== Object.prototype) {
+    const propValue = currentObj[prop]
+    if (
+      typeof propValue === 'function' &&
+      propValue.constructor.name === 'AsyncFunction' &&
+      typeof prop === 'string' &&
+      !prop.startsWith('_')
+    ) {
+      return true
+    }
+    currentObj = Object.getPrototypeOf(currentObj)
+  }
+
+  return false
+}
+
+export const sphinxCoreUtils = { sleep, callWithTimeout, isPublicAsyncMethod }

packages/plugins/src/hardhat/simulate.ts

@@ -55,11 +55,14 @@ export type SimulationTransactions = Array<{
  * before timing out. An RPC request can stall if the RPC provider has degraded service, which would
  * cause the simulation to stall indefinitely if we don't time out. We set this value to be
  * relatively high because the execution process may submit very large transactions (specifically
- * transactions with `EXECUTE` Merkle leaves), which can cause the RPC request to be slow.
+ * transactions with `EXECUTE` Merkle leaves), which can cause the RPC request to be slow. We also
+ * set it high because the Hardhat provider appears to make a few retries (in
+ * `hardhat/internal/core/providers/http.ts`), which can contribute to the duration of a single RPC
+ * call sent from our provider proxy.
  */
 export const simulationConstants = {
-  maxAttempts: 7,
-  timeout: 60_000,
+  maxAttempts: 10,
+  timeout: 150_000,
 }
 
 /**
@@ -459,13 +462,10 @@ export const handleSimulationSuccess = async (
 
 /**
  * Create a Proxy that wraps a `HardhatEthersProvider` to implement retry and timeout logic, which
- * isn't available natively in this provider.
+ * isn't robust in the native provider.
  *
- * This function uses an exponential backoff strategy for retries. After each failed attempt, we
- * wait `2 ** (attempt + 1)` seconds before trying again. At the time of writing this, we allow 7
- * attempts, which means we'll wait a total of `2 + 4 + 8 + 16 + 32 + 64` seconds, which equals 126.
- * Notice that there are 6 terms in that equation instead of 7 because we'll immediately revert if
- * the last attempt fails.
+ * This function uses a linear backoff strategy for retries. We use a multiple of 2, and start with
+ * a backoff period of two seconds.
  *
  * This function uses the `evm_snapshot` and `evm_revert` RPC methods to prevent a 'nonce too low'
  * bug caused by the Hardhat simulation (context: https://github.com/sphinx-labs/sphinx/pull/1565).
@@ -482,26 +482,12 @@ export const handleSimulationSuccess = async (
 export const createHardhatEthersProviderProxy = (
   ethersProvider: HardhatEthersProvider
 ): HardhatEthersProvider => {
-  return new Proxy(ethersProvider, {
+  const proxy = new Proxy(ethersProvider, {
     get: (target, prop) => {
       return (...args: any[]) => {
-        // Queue a snapshot of the Hardhat node state. We may revert to this snapshot later in this
-        // function to prevent a 'nonce too low' bug in Hardhat. We must queue the snapshot before
-        // calling `target[prop](...args)` to avoid this nonce bug, which occurs in spite of the
-        // fact that we don't immediately await either of these calls. We don't await this call yet
-        // because the `target[prop](...args)` call may be synchronous. If we await this call, then
-        // every call routed through this Proxy will become asynchronous, which could lead to
-        // expected behavior.
-        //
-        // More info on the nonce error is in this pull request description:
-        // https://github.com/sphinx-labs/sphinx/pull/1565.
-        const snapshotIdPromise = target.send('evm_snapshot', [])
-
-        // Call the method on the EthersJS provider.
-        const initialResult = target[prop](...args)
-        // Check if the call is synchronous. If so, return it.
-        if (!(initialResult instanceof Promise)) {
-          return initialResult
+        // Return the result directly if the method isn't asynchronous.
+        if (!sphinxCoreUtils.isPublicAsyncMethod(ethersProvider, prop)) {
+          return target[prop](...args)
         }
 
         // A helper function that implements the timeout and retry logic for asynchronous calls to
@@ -514,30 +500,44 @@ export const createHardhatEthersProviderProxy = (
             throw new Error(HardhatResetNotAllowedErrorMessage)
           }
 
-          // Resolve the promise that we initially created.
-          let snapshotId = await snapshotIdPromise
-
+          let snapshotId: string
           for (
             let attempt = 0;
             attempt < simulationConstants.maxAttempts;
             attempt++
           ) {
-            try {
-              // If this is the first attempt, we use the `initialResult` instead of calling
-              // `target[prop](...args)` again to avoid unintended consequences of calling the
-              // method on the provider more than once.
-              const result =
-                attempt === 0 ? initialResult : target[prop](...args)
+            // Create a snapshot of the Hardhat node state. We may revert to this snapshot later in
+            // this function to prevent a 'nonce too low' bug in Hardhat. We must queue the snapshot
+            // before calling `target[prop](...args)` to avoid this nonce bug.
+            //
+            // More info on the nonce error is in this pull request description:
+            // https://github.com/sphinx-labs/sphinx/pull/1565.
+            //
+            // This RPC method is outside of the try...catch statement below because this call
+            // should never error, so if it does, it'd preferable to throw the error immediately.
+            snapshotId = await target.send('evm_snapshot', [])
 
+            try {
               // Forward the call to the Hardhat provider. We include a timeout to ensure that an
               // RPC provider with degraded service doesn't cause this call to hang indefinitely.
               // See this pull request description for more info:
               // https://github.com/sphinx-labs/sphinx/pull/1565
-              return await sphinxCoreUtils.callWithTimeout(
-                result,
+
+              const result = await sphinxCoreUtils.callWithTimeout(
+                target[prop](...args),
                 simulationConstants.timeout,
                 getRpcRequestStalledErrorMessage(simulationConstants.timeout)
               )
+
+              if (prop === 'getSigner') {
+                // By default, the `HardhatEthersProxy.getSigner` method returns a signer connected
+                // to the `HardhatEthersProvider` instead of this Proxy, which prevents our timeout
+                // and retry logic from being used when the signer executes transactions. To avoid
+                // this, we set the signer's provider to be the current Proxy instance.
+                return (result as ethers.Signer).connect(proxy)
+              } else {
+                return result
+              }
             } catch (error) {
               // The most likely reason that the call failed is a rate limit.
 
@@ -562,12 +562,10 @@ export const createHardhatEthersProviderProxy = (
                 throw error
               }
 
-              // We use exponential backoff starting at 2 seconds. This serves as a cooldown period
+              // We use linear backoff starting at 2 seconds. This serves as a cooldown period
               // for rate limit errors.
-              const sleepTime = 2 ** (attempt + 1)
+              const sleepTime = 2 * (attempt + 1) * 1000
               await sphinxCoreUtils.sleep(sleepTime)
-
-              snapshotId = await target.send('evm_snapshot', [])
             }
           }
         }
@@ -590,6 +588,7 @@ export const createHardhatEthersProviderProxy = (
       }
     },
   })
+  return proxy
 }
 
 export const getUndeployedContractErrorMesage = (address: string): string =>

packages/plugins/test/mocha/common.ts

@@ -1105,8 +1105,13 @@ export const makeActionInputsWithoutGas = (
   return actionInputs
 }
 
-export const sumGeometricSeries = (a: number, r: number, n: number): number =>
-  a * ((1 - Math.pow(r, n)) / (1 - r))
+export const sumEvenNumbers = (start: number, numTerms: number): number => {
+  let sum = 0
+  for (let i = 0; i < numTerms; i++) {
+    sum += start + 2 * i
+  }
+  return sum
+}
 
 // eslint-disable-next-line @typescript-eslint/no-empty-function
 export const promiseThatNeverSettles = new Promise(() => {})

packages/plugins/test/mocha/simulate.spec.ts

@@ -27,7 +27,7 @@ import {
   makeStandardDeployment,
   promiseThatNeverSettles,
   startForkedAnvilNodes,
-  sumGeometricSeries,
+  sumEvenNumbers,
 } from './common'
 import {
   simulationConstants,
@@ -238,17 +238,30 @@ describe('handleSimulationSuccess', () => {
 })
 
 describe('createHardhatEthersProviderProxy', () => {
+  const asyncMethods = [
+    { name: 'send', isAsync: true },
+    { name: 'hardhat_reset', isAsync: true },
+    { name: 'getBlockNumber', isAsync: true },
+    { name: 'toJSON', isAsync: false },
+  ]
+
   let ethersProvider: any
   let proxy: HardhatEthersProvider
   let timeSum: number = 0
   let sendStub: sinon.SinonStub
+  let isPublicAsyncMethodStub: sinon.SinonStub
 
   beforeEach(() => {
     sinon
       .stub(sphinxCoreUtils, 'sleep')
       .callsFake(async (time: number): Promise<void> => {
         timeSum += time
       })
+    isPublicAsyncMethodStub = sinon.stub(sphinxCoreUtils, 'isPublicAsyncMethod')
+
+    for (const { name, isAsync } of asyncMethods) {
+      isPublicAsyncMethodStub.withArgs(sinon.match.any, name).returns(isAsync)
+    }
 
     sendStub = sinon.stub().resolves('defaultPromiseValue')
     sendStub.withArgs('evm_snapshot', []).resolves('snapshotId')
@@ -278,7 +291,7 @@ describe('createHardhatEthersProviderProxy', () => {
 
     ethersProvider.getBlockNumber = sinon
       .stub()
-      .returns(promiseThatNeverSettles)
+      .resolves(promiseThatNeverSettles)
 
     const callWithTimeoutSpy = sinon.spy(sphinxCoreUtils, 'callWithTimeout')
 
@@ -310,14 +323,16 @@ describe('createHardhatEthersProviderProxy', () => {
       simulationConstants.maxAttempts
     )
 
-    // Verify exponential backoff timing
-    const expectedDuration = sumGeometricSeries(
-      2,
-      2,
-      // We subtract one because we throw the error after the last attempt instead of waiting.
-      simulationConstants.maxAttempts - 1
-    )
+    // Verify linear backoff timing
+    const expectedDuration =
+      sumEvenNumbers(
+        2,
+        // We subtract one because we throw the error after the last attempt instead of waiting.
+        simulationConstants.maxAttempts - 1
+      ) * 1000
     expect(timeSum).equals(expectedDuration)
+    // Check that the time is denominated in seconds
+    expect(timeSum % 1000).equals(0)
 
     // The following is a regression test for the 'nonce too low' bug described in this pull request
     // description: https://github.com/sphinx-labs/sphinx/pull/1565
@@ -382,14 +397,16 @@ describe('createHardhatEthersProviderProxy', () => {
       simulationConstants.maxAttempts
     )
 
-    // Verify exponential backoff timing.
-    const expectedDuration = sumGeometricSeries(
-      2,
-      2,
-      // We subtract one because the last attempt succeeded.
-      simulationConstants.maxAttempts - 1
-    )
+    // Verify linear backoff timing
+    const expectedDuration =
+      sumEvenNumbers(
+        2,
+        // We subtract one because we throw the error after the last attempt instead of waiting.
+        simulationConstants.maxAttempts - 1
+      ) * 1000
     expect(timeSum).equals(expectedDuration)
+    // Check that the time is denominated in seconds
+    expect(timeSum % 1000).equals(0)
 
     // Check that the last call was successful
     expect(