Skip to content

Commit

Permalink
fix(pg): make merkle leaf gas estimation more robust
Browse files Browse the repository at this point in the history
  • Loading branch information
@sam-goldman
sam-goldman committed Dec 13, 2023
1 parent 617b754 commit 5b511e9
Show file tree
Hide file tree
Showing 5 changed files with 114 additions and 199 deletions.
6 changes: 6 additions & 0 deletions .changeset/lovely-beers-obey.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@sphinx-labs/contracts': patch
'@sphinx-labs/plugins': patch
---

Make gas estimate more robust by using `gasleft()`
6 changes: 1 addition & 5 deletions packages/contracts/contracts/foundry/SphinxPluginTypes.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,11 +239,7 @@ contract SphinxPluginTypes {

function getSphinxConfig() external view returns (SphinxConfig memory sphinxConfig) {}

function leafGasParams()
external
view
returns (SphinxTransaction[] memory txnArray, uint256[] memory chainIds)
{}
function leafGasParams() external view returns (SphinxTransaction[] memory txnArray) {}

function sphinxLeafWithProofType()
external
Expand Down
98 changes: 55 additions & 43 deletions packages/plugins/contracts/foundry/Sphinx.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -636,60 +636,72 @@ abstract contract Sphinx {
}

/**
* @notice Estimates the values of the `gas` fields in the Sphinx Merkle leaves.
* @notice Estimates the values of the `gas` fields in the Merkle leaves using `gasleft`. This
* provides a more accurate estimate than simulating the transactions and retrieving
* them from Foundry's broadcast file. Particularly, it's possible to underestimate the
* Merkle leaf's gas with the simulation approach. Consider this (contrived) edge case:
* Say a user's transaction deploys a contract, which costs ~2 million gas, and also
* involves a large gas refund (~500k gas). Since gas refunds occur after the
* transaction is executed, the broadcast file will have a gas estimate of ~1.5 million
* gas. However, the user's transaction costs 2 million gas. This will cause Sphinx to
* underestimate the Merkle leaf's gas, resulting in a failed deployment on-chain. This
* situation uses contrived numbers, but the point is that using `gasleft` is accurate
* even if there's a large gas refund.
*
* @return abiEncodedGasArray The ABI encoded array of gas estimates. There's one element per
* `EXECUTE` Merkle leaf. We ABI encode the array because Foundry
* makes it difficult to reliably parse complex data types off-chain.
* Specifically, an array element looks like this in the returned
* JSON: `27222 [2.722e4]`.
*/
function sphinxEstimateMerkleLeafGas(string memory _leafGasParamsFilePath) external {
(SphinxTransaction[] memory txnArray, uint256[] memory chainIds) = abi.decode(
function sphinxEstimateMerkleLeafGas(
string memory _leafGasParamsFilePath,
uint256 _chainId
) external returns (bytes memory abiEncodedGasArray) {
SphinxTransaction[] memory txnArray = abi.decode(
vm.parseBytes(vm.readFile(_leafGasParamsFilePath)),
(SphinxTransaction[], uint256[])
);
require(
txnArray.length == chainIds.length,
"Sphinx: Array length mismatch. Should never happen."
(SphinxTransaction[])
);

IGnosisSafe safe = IGnosisSafe(sphinxSafe());
address module = sphinxModule();
address managedServiceAddress = constants.managedServiceAddress();

uint256[] memory uniqueChainIds = sphinxUtils.getUniqueUint256(chainIds);
for (uint256 i = 0; i < uniqueChainIds.length; i++) {
// Create a fork of the target network.
NetworkInfo memory currentNetworkInfo = sphinxUtils.findNetworkInfoByChainId(
uniqueChainIds[i]
);
vm.createSelectFork(vm.rpcUrl(currentNetworkInfo.name));
uint256[] memory gasEstimates = new uint256[](txnArray.length);

// Deploy the Sphinx Module and Gnosis Safe if they're not already deployed.
if (address(safe).code.length == 0) {
// Deploy the Gnosis Safe and Sphinx Module. We can't broadcast this transaction
// from the Gnosis Safe or the Sphinx Module because Foundry throws an error if we
// attempt to broadcast from the same contract address that we're deploying. We
// broadcast from the Managed Service contract so that this transaction is included
// in the off-chain gas estimation logic.
vm.startBroadcast(managedServiceAddress);
_sphinxDeployModuleAndGnosisSafe();
vm.stopBroadcast();
}
// Create a fork of the target network.
NetworkInfo memory networkInfo = sphinxUtils.findNetworkInfoByChainId(_chainId);
vm.createSelectFork(vm.rpcUrl(networkInfo.name));

// We broadcast from the Sphinx Module to replicate the production environment. In prod,
// the Sphinx Module calls the Gnosis Safe.
vm.startBroadcast(module);

for (uint256 j = 0; j < txnArray.length; j++) {
if (chainIds[j] == currentNetworkInfo.chainId) {
SphinxTransaction memory txn = txnArray[j];
bool success = safe.execTransactionFromModule(
txn.to,
txn.value,
txn.txData,
txn.operation
);
require(success, "Sphinx: failed to call Gnosis Safe from Sphinx Module");
}
}
vm.stopBroadcast();
// Deploy the Sphinx Module and Gnosis Safe if they're not already deployed.
if (address(safe).code.length == 0) {
// Deploy the Gnosis Safe and Sphinx Module. It's not strictly necessary to prank the
// Managed Service contract, but this replicates the prod environment, so we do it
// anyways.
vm.startPrank(managedServiceAddress);
_sphinxDeployModuleAndGnosisSafe();
vm.stopPrank();
}

// We prank the Sphinx Module to replicate the production environment. In prod, the Sphinx
// Module calls the Gnosis Safe.
vm.startPrank(module);

for (uint256 i = 0; i < txnArray.length; i++) {
SphinxTransaction memory txn = txnArray[i];
uint256 startGas = gasleft();
bool success = safe.execTransactionFromModule(
txn.to,
txn.value,
txn.txData,
txn.operation
);
gasEstimates[i] = startGas - gasleft();
require(success, "Sphinx: failed to call Gnosis Safe from Sphinx Module");
}
vm.stopPrank();

return abi.encode(gasEstimates);
}

/**
Expand Down
8 changes: 4 additions & 4 deletions packages/plugins/script/Sample.s.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,19 @@ contract Sample is Sphinx {
}

function run() public override sphinx {
new MyContract1{ salt: bytes32(uint(1)) }(
new MyContract1{ salt: bytes32(uint(1231231)) }(
-1,
2,
address(1),
address(2)
);
new MyContract1{ salt: bytes32(uint(2)) }(
new MyContract1{ salt: bytes32(uint(2231313)) }(
-1,
2,
address(1),
address(2)
);
new MyContract1{ salt: bytes32(uint(3)) }(
new MyContract1{ salt: bytes32(uint(3213123)) }(
-1,
2,
address(1),
Expand All @@ -42,7 +42,7 @@ contract Sample is Sphinx {

bytes memory initCode =
abi.encodePacked(type(MyContract1).creationCode, abi.encode(1, 2, address(1), address(2)));
address deployed = CREATE3.deploy(bytes32(0), initCode, 0);
address deployed = CREATE3.deploy(bytes32(uint(93849038439)), initCode, 0);
sphinxLabel(deployed, "contracts/test/MyContracts.sol:MyContract1");
}
}
195 changes: 48 additions & 147 deletions packages/plugins/src/foundry/utils/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ import {
SphinxLeafType,
SphinxMerkleTree,
SphinxModuleABI,
SphinxTransaction,
parseFoundryArtifact,
remove0x,
} from '@sphinx-labs/contracts'
Expand Down Expand Up @@ -639,165 +638,67 @@ export const getSphinxLeafGasEstimates = async (
targetContract?: string,
spinner?: ora.Ora
): Promise<Array<Array<string>>> => {
const leafGasParamsFragment = sphinxPluginTypesInterface.fragments
.filter(ethers.Fragment.isFunction)
.find((fragment) => fragment.name === 'leafGasParams')
if (!leafGasParamsFragment) {
throw new Error(`Could not find fragment in ABI. Should never happen.`)
}
const leafGasParamsFragment = findFunctionFragment(
sphinxPluginTypesInterface,
'leafGasParams'
)

const coder = ethers.AbiCoder.defaultAbiCoder()
const leafGasInputsFilePath = join(
foundryToml.cachePath,
'sphinx-estimate-leaf-gas.txt'
)

const chainIds: Array<number> = []
const txns: Array<SphinxTransaction> = []
for (const { actionInputs, deploymentInfo } of collected) {
for (const actionInput of actionInputs) {
chainIds.push(Number(deploymentInfo.chainId))
txns.push(toSphinxTransaction(actionInput))
}
}

const encodedTxnArray = coder.encode(leafGasParamsFragment.outputs, [
txns,
chainIds,
])

// Write the ABI encoded data to the file system. We'll read it in the Forge script. We do this
// instead of passing in the data as a parameter to the Forge script because it's possible to hit
// Node's `spawn` input size limit if the data is large. This is particularly a concern because
// the data contains contract init code.
writeFileSync(leafGasInputsFilePath, encodedTxnArray)

const leafGasEstimationScriptArgs = [
'script',
scriptPath,
'--sig',
'sphinxEstimateMerkleLeafGas(string)',
leafGasInputsFilePath,
// Set the gas estimate multiplier to be 30%. This is Foundry's default multiplier, but we
// hard-code it just in case Foundry changes the default value in the future. In practice, this
// tends to produce a gas estimate multiplier that's around 35% to 55% higher than the actual
// gas used instead of 30%.
'--gas-estimate-multiplier',
'130',
]
if (targetContract) {
leafGasEstimationScriptArgs.push('--target-contract', targetContract)
}

const dateBeforeForgeScript = new Date()
const gasEstimationSpawnOutput = await spawnAsync(
'forge',
leafGasEstimationScriptArgs
)
if (gasEstimationSpawnOutput.code !== 0) {
spinner?.stop()
// The `stdout` contains the trace of the error.
console.log(gasEstimationSpawnOutput.stdout)
// The `stderr` contains the error message.
console.log(gasEstimationSpawnOutput.stderr)
process.exit(1)
}

// Get the number of chains that contain at least one transaction to execute.
const containsActionInput = collected.filter(
({ actionInputs }) => actionInputs.length > 0
)

const dryRun =
containsActionInput.length > 1
? readFoundryMultiChainDryRun(
foundryToml.broadcastFolder,
scriptPath,
`sphinxEstimateMerkleLeafGas`,
dateBeforeForgeScript
)
: readFoundrySingleChainDryRun(
foundryToml.broadcastFolder,
scriptPath,
containsActionInput[0].deploymentInfo.chainId,
`sphinxEstimateMerkleLeafGas`,
dateBeforeForgeScript
)

if (!dryRun) {
// This should never happen because the presence of action inputs should always mean that a
// broadcast occurred.
throw new Error(`Could not read Foundry dry run file. Should never happen.`)
}

const gasEstimatesArray: Array<Array<string>> = []
for (const { actionInputs, deploymentInfo } of collected) {
let transactions: Array<FoundryDryRunTransaction> = []
if (isFoundryMultiChainDryRun(dryRun)) {
// Find the dry run that corresponds to the current network.
const deploymentOnNetwork = dryRun.deployments.find(
(deployment) => deployment.chain.toString() === deploymentInfo.chainId
)
// If we couldn't find a dry run that corresponds to the current network, then there must
// not be any transactions to execute on it. We use an empty transactions array in this case.
transactions = deploymentOnNetwork ? deploymentOnNetwork.transactions : []
} else if (isFoundrySingleChainDryRun(dryRun)) {
// Check if the current network matches the network of the dry run. If the current network
// doesn't match the dry run's network, then this means there weren't any transactions
// executed on the current network. We use an empty transactions array in this case.
transactions =
deploymentInfo.chainId === dryRun.chain.toString()
? dryRun.transactions
: []
} else {
throw new Error(
`Foundry dry run is an incompatible type. Should never happen.`
)
const txns = actionInputs.map(toSphinxTransaction)
const encodedTxnArray = coder.encode(leafGasParamsFragment.outputs, [txns])

// Write the ABI encoded data to the file system. We'll read it in the Forge script. We do this
// instead of passing in the data as a parameter to the Forge script because it's possible to hit
// Node's `spawn` input size limit if the data is large. This is particularly a concern because
// the data contains contract init code.
writeFileSync(leafGasInputsFilePath, encodedTxnArray)

const leafGasEstimationScriptArgs = [
'script',
scriptPath,
'--sig',
'sphinxEstimateMerkleLeafGas(string,uint256)',
leafGasInputsFilePath,
deploymentInfo.chainId,
'--silent', // Silence compiler output
'--json',
]
if (targetContract) {
leafGasEstimationScriptArgs.push('--target-contract', targetContract)
}

let gasEstimates: Array<string> = []
if (deploymentInfo.chainId === '421614') {
// We hard-code the Merkle leaf's gas on Arbitrum Sepolia because Foundry's gas estimation for
// this network is currently broken. The issue is described here:
// https://github.com/foundry-rs/foundry/issues/6591
// We hard-code it to be 50 million because the block gas limit on Arbitrum Sepolia is
// ~1 quadrillion, so there's no harm in having a high estimate. For reference, it costs
// ~15 million gas to deploy `MyLargeContract` in the `plugins` package tests.
gasEstimates = new Array(transactions.length).fill('50000000')
} else {
gasEstimates = transactions
// We remove any transactions that weren't broadcasted from the Sphinx Module. Particularly,
// we remove the transaction that deploys the Gnosis Safe and Sphinx Module, which is
// broadcasted from a different address. This transaction isn't relevant because we're only
// interested in estimating the gas of the Sphinx leaves.
.filter(
(tx) =>
typeof tx.transaction.from === 'string' &&
ethers.getAddress(tx.transaction.from) ===
deploymentInfo.moduleAddress
)
.map((tx) => tx.transaction.gas)
// Narrows the TypeScript type of `gas` from `string | null` to `string`.
.map((gas) => {
if (typeof gas !== 'string') {
throw new Error(
`Detected a 'gas' field that is not a string. Should never happen.`
)
}
return gas
})
// Convert the gas values from hex strings to decimal strings.
.map((gas) => parseInt(gas, 16).toString())

// Sanity check that there's a gas estimate for each transaction.
if (gasEstimates.length !== actionInputs.length) {
throw new Error(
`Mismatch between the number of transactions and the number of gas estimates. Should never happen.`
)
}
const gasEstimationSpawnOutput = await spawnAsync(
'forge',
leafGasEstimationScriptArgs
)
if (gasEstimationSpawnOutput.code !== 0) {
spinner?.stop()
// The `stdout` contains the trace of the error.
console.log(gasEstimationSpawnOutput.stdout)
// The `stderr` contains the error message.
console.log(gasEstimationSpawnOutput.stderr)
process.exit(1)
}

gasEstimatesArray.push(gasEstimates)
const returned = JSON.parse(gasEstimationSpawnOutput.stdout).returns
.abiEncodedGasArray.value
// ABI decode the gas array.
const [decoded] = coder.decode(['uint256[]'], returned)
// Convert the BigInt elements to Numbers, then multiply by a buffer of 1.5x. This ensures the
// user's transaction doesn't fail on-chain due to variations in the chain state, which could
// occur between the time of the simulation and execution.
const returnedGasArrayWithBuffer = decoded
.map(Number)
.map((gas) => Math.round(gas * 1.5).toString())

gasEstimatesArray.push(returnedGasArrayWithBuffer)
}

return gasEstimatesArray
Expand Down

0 comments on commit 5b511e9

Please sign in to comment.