feat: Gas Utils for L1 operations #9834
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Changes to circuit sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
Changes to public function bytecode sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
spalladino
requested changes
Nov 13, 2024
There was a problem hiding this comment.
The scripts look good, but I think the gas utils have a few issues mostly around race conditions and maxFee vs maxPrioFee. We could split up this PR to get the scripts in, while we keep iterating on the gas utils.
Aside from the issues, I'm worried the API for using it makes it much more verbose. It'd be good to have an API where we just pass in the result of simulate, and then the util handles the first send, the monitoring, the retries, etc. Something like:
const { request } = await this.rollupContract.simulate.submitEpochRootProof(txArgs);
return await waitWithRetries(request, walletClient, gasOpts);We should also tweak the e2e test (which maybe we can degrade to a "unit" test that calls startAnvil, using a lower block time?) so that it exercises more scenarios: priority fee spiking instead of just base fee spiking, tx getting dropped (anvil_dropTransaction cheat code is your friend), an older replacement tx getting mined, etc.
Comment on lines
56
to
60
| if (isAnvilTestChain(chainId)) { | ||
| dualLog(`Funding validator on L1`); | ||
| const cheatCodes = new EthCheatCodes(rpcUrl, debugLogger); | ||
| await cheatCodes.setBalance(validatorAddress, 10n ** 20n); | ||
| } |
There was a problem hiding this comment.
Should we check balance if we're not on anvil, and emit a noisy log remembering the user to fund the address manually?
| .addOption(pxeOption) | ||
| .description('Gets the information of an Aztec node from a PXE or directly from an Aztec node.') | ||
| .option('--node-url <string>', 'URL of the node.', `http://${LOCALHOST}:8080`) | ||
| .option('--from-node', 'Get the info directly from an Aztec node.', false) |
There was a problem hiding this comment.
Why not decide based on whether node-url or pxe-url is set, instead of having this flag?
There was a problem hiding this comment.
I realize now that's what it actually does in the action below, I forgot to remove this option
| @@ -42,4 +42,4 @@ function filter_noise() { | |||
| } | |||
|
|
|||
| # Start the Aztec node with the sequencer and archiver | |||
| node --no-warnings "$REPO"/yarn-project/aztec/dest/bin/index.js start --node --archiver --sequencer --pxe 2>&1 | filter_noise | |||
There was a problem hiding this comment.
We were starting an entire PXE just to answer getNodeInfo? Damn.
| export L1_PRIVATE_KEY=$VALIDATOR_PRIVATE_KEY | ||
| export SEQ_PUBLISHER_PRIVATE_KEY=$VALIDATOR_PRIVATE_KEY | ||
| export DEBUG=${DEBUG:-"aztec:*,-aztec:avm_simulator*,-aztec:libp2p_service*,-aztec:circuits:artifact_hash,-json-rpc*,-aztec:l2_block_stream,-aztec:world-state:*"} | ||
| export ETHEREUM_HOST="http://127.0.0.1:8545" | ||
| export ETHEREUM_HOST=${ETHEREUM_HOST:-"http://127.0.0.1:8545"} | ||
| export IS_ANVIL=${IS_ANVIL:-"true"} |
There was a problem hiding this comment.
You can use something along these lines (with a try/catch!) to query whether it is anvil or not, so we don't risk forgetting it:
curl -s -H "Content-Type: application/json" -X POST --data '{"method":"web3_clientVersion","params":[],"id":49,"jsonrpc":"2.0"}' $ETHEREUM_HOST | jq .result | grep -q anvil
| P2P_PORT=$((40401 + i)) | ||
| IDX=$((i + 1)) | ||
|
|
||
| # Use empty string as default if variables are not set |
There was a problem hiding this comment.
Wouldn't it be safer to fail loudly if the private key is not set?
There was a problem hiding this comment.
we don't actually want to fail if it's not set, these would only be set when running against public networks like Sepolia where we need to use funded accounts.
In local testing the keys & addresses are later generated in validator.sh and funded when doing add-l1-validator
There was a problem hiding this comment.
Makes sense! Could you add a comment with that, so next reader doesn't fall into the same?
Comment on lines
183
to
184
| // Transaction not found and enough time has passed - might be stuck | ||
| if (Date.now() - lastSeen > config.stallTimeMs && attempts < config.maxAttempts) { |
There was a problem hiding this comment.
IIUC this bumps gas price if nodes drop the tx altogether, but not if it just sits in the mempool without being mined. We should add another condition for that.
Comment on lines
120
to
129
| return retry( | ||
| async () => { | ||
| const gas = await estimator(); | ||
| return gas + (gas * this.gasConfig.bufferPercentage) / 100n; | ||
| }, | ||
| 'gas estimation', | ||
| makeBackoff([1, 2, 3]), | ||
| this.logger, | ||
| false, | ||
| ); |
There was a problem hiding this comment.
Why the retry here as opposed to on other rpc calls? If we are seeing flaky rpc endpoints where we need to retry, we should tackle that at a lower level (on viem's "transport" most likely).
| this.logger?.info(`Transaction ${currentTxHash} pending`); | ||
| if (tx) { | ||
| lastSeen = Date.now(); | ||
| await new Promise(resolve => setTimeout(resolve, config.checkIntervalMs)); |
There was a problem hiding this comment.
Suggested change
| await new Promise(resolve => setTimeout(resolve, config.checkIntervalMs)); | |
| await sleep(config.checkIntervalMs); |
Comment on lines
175
to
176
| const tx = await this.publicClient.getTransaction({ hash: currentTxHash }); | ||
| this.logger?.info(`Transaction ${currentTxHash} pending`); |
There was a problem hiding this comment.
You can do the same trick as with the receipt here. Instead of doing getTx on every hash you've sent, you can query getTxCount using the pending tag, which should consider the txs that are sitting on the mempool for the given sender.
| data: originalTx.data, | ||
| nonce: originalTx.nonce, | ||
| gas: originalTx.gasLimit, | ||
| maxFeePerGas: newGasPrice, |
There was a problem hiding this comment.
I'm not sure if bumping only maxFeePerGas without touching maxPriorityFeePerGas has the intended effect. If maxPriorityFeePerGas is not set, then viem defaults it to either a chain default or to whatever the json rpc interface returns (see here). So we're not actually bumping the priority fee, we're just setting it to whatever nodes say is the latest value to use, and ignoring the gasPriceIncrease.
Maybe we should be setting maxPriorityFeePerGas as well? I'm thinking we could just always set maxFeePerGas to gasConfig.maxGwei, and then just handle stuck txs by bumping the maxPriorityFeePerGas, since the user never gets charged more than baseFee + priorityFee. We could get the max prio fee from either eth_maxPriorityFeePerGas RPC as viem does, or from a gas price oracle like BlockNative. Then, for every successive retry, we bump the maxPrioFee to the new eth_maxPriorityFeePerGas we see, or to a multiplier of the previous prio fee, whatever's greater.
|
I guess you're not going to choose me as reviewer in the future, sorry for so throwing in many comments. |
spypsy
force-pushed
the
spy/gas-utils
branch
from
68af7ec
to
f96b1d8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #9833
Also
getNodeInfotoAztecNodeinterface