diff --git a/.changeset/great-ads-rhyme.md b/.changeset/great-ads-rhyme.md new file mode 100644 index 000000000..920b86519 --- /dev/null +++ b/.changeset/great-ads-rhyme.md @@ -0,0 +1,6 @@ +--- +'@sphinx-labs/plugins': patch +'@sphinx-labs/core': patch +--- + +Fix simulation bugs diff --git a/packages/core/src/utils.ts b/packages/core/src/utils.ts index 0512ec55a..aeb7ea467 100644 --- a/packages/core/src/utils.ts +++ b/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 } diff --git a/packages/plugins/src/hardhat/simulate.ts b/packages/plugins/src/hardhat/simulate.ts index 8cc4ced2f..b55bbd798 100644 --- a/packages/plugins/src/hardhat/simulate.ts +++ b/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 => diff --git a/packages/plugins/test/mocha/common.ts b/packages/plugins/test/mocha/common.ts index 5846cd326..e2bc5ea14 100644 --- a/packages/plugins/test/mocha/common.ts +++ b/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(() => {}) diff --git a/packages/plugins/test/mocha/simulate.spec.ts b/packages/plugins/test/mocha/simulate.spec.ts index de2610ae6..dc8b49736 100644 --- a/packages/plugins/test/mocha/simulate.spec.ts +++ b/packages/plugins/test/mocha/simulate.spec.ts @@ -27,7 +27,7 @@ import { makeStandardDeployment, promiseThatNeverSettles, startForkedAnvilNodes, - sumGeometricSeries, + sumEvenNumbers, } from './common' import { simulationConstants, @@ -238,10 +238,18 @@ 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 @@ -249,6 +257,11 @@ describe('createHardhatEthersProviderProxy', () => { .callsFake(async (time: number): Promise => { 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(