-
Notifications
You must be signed in to change notification settings - Fork 331
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
chore!: enable multiple L1 nodes to be used #11945
Conversation
Changes to public function bytecode sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hell of a PR. Found a few instances that need attention, but aside from that, looks good.
docker-compose.yml
Outdated
@@ -64,7 +64,7 @@ services: | |||
- p2p-boot-node | |||
entrypoint: | | |||
/bin/sh -c ' | |||
export ETHEREUM_HOST=$$(cat /var/run/secrets/ethereum-host) | |||
export ETHEREUM_HOSTS=$$(cat /var/run/secrets/ethereum-host) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to pluralize the secret as well? (I'm fine if we don't, just checking we didn't chan ge the secret but forgot to change here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only for consistency's sake, will do
until curl -s -X POST -H 'Content-Type: application/json' \ | ||
-d '{"jsonrpc":"2.0","method":"eth_chainId","params":[],"id":67}' \ | ||
${ETHEREUM_HOST} | grep 0x; do | ||
echo "Waiting for Ethereum node ${ETHEREUM_HOST}..." | ||
${ETHEREUM_HOSTS} | grep 0x; do | ||
echo "Waiting for Ethereum node ${ETHEREUM_HOSTS}..." | ||
sleep 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will break if there's more than one value in ETHEREUM_HOSTS
, we need to loop over all values and wait until all (or one?) is available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think one? point of this is redundancy so we should consider just one as 'ready'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense!
const publicClient = createPublicClient({ | ||
chain: chain.chainInfo, | ||
transport: http(chain.rpcUrl), | ||
transport: fallback(config.l1RpcUrls.map(url => http(url))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
this.walletClient = createWalletClient({ | ||
account: this.account, | ||
chain: chain.chainInfo, | ||
transport: ViemHttp(chain.rpcUrl), | ||
transport: fallback([ViemHttp(chain.rpcUrls[0])]), | ||
}); | ||
|
||
this.publicClient = createPublicClient({ | ||
chain: chain.chainInfo, | ||
transport: ViemHttp(chain.rpcUrl), | ||
transport: fallback([ViemHttp(chain.rpcUrls[0])]), | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why rpcUrls[0] instead of mapping over all urls, as done in the archiver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question, I don't think I meant to do that. will update
private publicClient: ViemPublicClient, | ||
private walletClient: ViemWalletClient, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but we should stop using the two clients simultaneously, and just use the wallet.extend(publicActions)
to have one client with the capabilities of both.
export L1_CHAIN_ID=$(curl -s -X POST -H "Content-Type: application/json" \ | ||
--data '{"jsonrpc":"2.0","method":"eth_chainId","params":[],"id":1}' \ | ||
$ETHEREUM_HOST | grep -o '"result":"0x[^"]*"' | cut -d'"' -f4 | xargs printf "%d\n") | ||
$ETHEREUM_HOSTS | grep -o '"result":"0x[^"]*"' | cut -d'"' -f4 | xargs printf "%d\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
# Automatically detect if we're using Anvil | ||
if 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; then | ||
if curl -s -H "Content-Type: application/json" -X POST --data '{"method":"web3_clientVersion","params":[],"id":49,"jsonrpc":"2.0"}' $ETHEREUM_HOSTS | jq .result | grep -q anvil; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems we don't actually use IS_ANVIL anymore, will remove
@@ -162,7 +162,7 @@ describe('End-to-end tests for devnet', () => { | |||
claimSecret: { value: string }; | |||
messageLeafIndex: string; | |||
}>('bridge-fee-juice', [amount, l2Account.getAddress()], { | |||
'l1-rpc-url': ETHEREUM_HOST!, | |||
'l1-rpc-url': ETHEREUM_HOSTS!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'l1-rpc-url': ETHEREUM_HOSTS!, | |
'l1-rpc-urls': ETHEREUM_HOSTS!, |
I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch indeed!
@@ -279,7 +279,7 @@ describe('End-to-end tests for devnet', () => { | |||
|
|||
async function getL1Balance(address: string, token?: EthAddress): Promise<bigint> { | |||
const { balance } = await cli<{ balance: string }>('get-l1-balance', [address], { | |||
'l1-rpc-url': ETHEREUM_HOST!, | |||
'l1-rpc-url': ETHEREUM_HOSTS!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
export type ViemClient = Client< | ||
HttpTransport, | ||
Chain, | ||
PrivateKeyAccount, | ||
[...PublicRpcSchema, ...WalletRpcSchema], | ||
PublicActions<HttpTransport, Chain> & WalletActions<Chain, PrivateKeyAccount> | ||
>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why we had to re-define and re-export this type from here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah this was initially the only place I ran into it so only had it here. then realized it's used in more places and added in types, will update 👍
There are also a few mentions to |
And it'd be nice if we could tweak the cheat codes to also handle multiple rpc urls, it's the only component left that still forces one rpc url. |
-d '{"jsonrpc":"2.0","method":"eth_chainId","params":[],"id":67}' \ | ||
${HOST} | grep -q 0x; then | ||
echo "Ethereum node ${HOST} is ready!" | ||
exit 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the scope of this exit 0? IIUC this template gets pasted as-is where it is include
d, so any operations after this helper would not run due to the early exit. This would affect eg deploy-l1-contracts
:
- name: deploy-l1-contracts
{{- include "aztec-network.image" . | nindent 10 }}
command:
- /bin/bash
- -c
- |
cp /scripts/deploy-l1-contracts.sh /tmp/deploy-l1-contracts.sh
chmod +x /tmp/deploy-l1-contracts.sh
source /shared/config/service-addresses
source /shared/config/validator-addresses
{{- include "aztec-network.waitForEthereum" . | nindent 14 }}
/tmp/deploy-l1-contracts.sh "{{ .Values.aztec.l1Salt }}" "{{ .Values.ethereum.chainId }}" "$VALIDATOR_ADDRESSES"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, reviewed this in an outdated commit and it was already fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still, I think the new version doesn't do what we expect. I understand it loops over every host, waiting for 5s inbetween them, and exits once the first one replies. But it doesn't go back to the first one to retry if all failed, it just goes through, right?
found_node=0 | ||
for HOST in $(echo "${ETHEREUM_HOSTS}" | tr ',' '\n'); do | ||
if curl -s -X POST -H 'Content-Type: application/json' \ | ||
--data '{"jsonrpc":"2.0","method":"eth_blockNumber","params":[],"id":1}' \ | ||
"$HOST" 2>/dev/null | grep -q 'result'; then | ||
echo "Node $HOST is ready" | ||
found_node=1 | ||
break | ||
fi | ||
sleep 1 | ||
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads up this is not equivalent to the previous one: the new one will exit if the node is not immediately available, while the previous one will retry. Given this is called simultaneously to ETHEREUM_SCRIPT
in scripts/run_native_testnet.sh
, we should keep the behaviour of waiting for the node to be up.
yarn-project/end-to-end/scripts/native-network/deploy-l1-contracts.sh
Outdated
Show resolved
Hide resolved
...t/prover-client/src/proving_broker/proving_broker_database/broker_persisted_database.test.ts
Show resolved
Hide resolved
"$HOST" 2>/dev/null | grep -q 'result'; then | ||
echo "Node $HOST is ready" | ||
found_node=1 | ||
break 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL!
* master: (31 commits) feat: Slack message to ci channel tagging owners on flakes. (#12284) fix: slack notify was broken by quoted commit titles revert: "chore: Fix and reenable fees-settings test (#12302)" fix: run arm64 on master (#12307) yolo fix chore: Fix and reenable fees-settings test (#12302) feat!: rename compute_nullifier_without_context (#12308) chore: Lazy loading artifacts everywhere (#12285) chore: Reenable dapp subscription test (#12304) chore: Run prover test with fake proofs when requested (#12305) chore: Do not set CI_FULL outside CI (#12300) chore: new mnemonic deployments on sepolia (#12076) chore!: enable multiple L1 nodes to be used (#11945) chore: remove no longer supported extension from vscode/extension.json (#12303) fix(e2e): p2p_reqresp (#12297) feat: Sync from noir (#12298) chore: enabling `e2e_contract_updates` in CI + nuking irrelevant test (#12293) feat: prepend based merge (#12093) feat: fetch addresses from registry (#12000) feat: live logs (#12271) ...
ETHEREUM_HOST
env var toETHEREUM_HOSTS
. Using a single host should still work as it did beforeViemWalletClient
&ViemPublicClient
BREAKING CHANGE:
ETHEREUM_HOST
->ETHEREUM_HOSTS
--l1-rpc-url
->--l1-rpc-urls
l1RpcUrl
->l1RpcUrls
l1RpcUrl
->l1RpcUrls
DeployL1Contracts
(type) ->DeployL1ContractsReturnType
Fixes #11790
Follow-up #12254