-
Notifications
You must be signed in to change notification settings - Fork 267
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: values for sepolia deployment #10362
Conversation
@@ -93,7 +93,7 @@ spec: | |||
source /shared/p2p/p2p-addresses && \ | |||
source /shared/config/service-addresses && \ | |||
env && \ | |||
node --no-warnings /usr/src/yarn-project/aztec/dest/bin/index.js start --node --archiver --sequencer --pxe |
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 drop the PXE?
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.
wasn't doing anything right? I believe it was only used for the get-node-info
call from other services to check boo-node is up but this has now been added to Aztec Node API
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.
Hm yes, but I think this now conflicts with spartan/aztec-network/files/config/config-validator-env.sh
, since it is asking for a PXE url.
It is convenient in that case because when "dynamic boot node" is enabled, the validators use each other to bootstrap, so they use the PXE service, which is connected to the boot node if the network is public, and the validator service otherwise.
So I think the answer is to update the wait-for-services
init container in the validators to use echo "{{ include "aztec-network.validatorUrl" . }}" > /shared/boot_node/boot_node_url
, and then change back the config-validator-env
to use the node url.
Could/should just be a separate issue though.
|
||
validator: | ||
replicas: 3 | ||
validatorKeys: ${VALIDATOR_KEYS} |
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.
How do these get set? I didn't think you could pull from env like this in a values file.
38fbedf
to
c33c122
Compare
"$(git rev-parse --show-toplevel)/scripts/run_interleaved.sh" "${CMD[@]}" | ||
else | ||
# Use run_interleaved with a wait condition | ||
WAIT_CONDITION="curl -s http://127.0.0.1:$FIRST_PORT/status >/dev/null" |
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.
was this wait condition added to prevent them being added with the same key? #10327 this pr pre adds the validators at contract deployment time so may invalidate the need for this work around
--l1-chain-id "$L1_CHAIN_ID" \ | ||
--mnemonic "$MNEMONIC" \ | ||
--json | tee ./basic_contracts.json | ||
if ${{ inputs.sepolia_deployment }}; 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.
A nit, not for now, but a bunch of the script above here is to set up port forwarding, but we use none of that in this if
. Seems we should just have a different step, and condition on inputs.sepolia_deployment
, and then a common step beneath for uploading.
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.
added here
@@ -1,3 +1,4 @@ | |||
{{- if not .Values.network.disableEthNode }} |
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.
instead of adding a new flag, could this be replaced by checking if .Values.ethereum.externalHost is set? If we wanted a flag, we could create a helper based off checking that value?
@@ -53,8 +53,8 @@ get_service_address() { | |||
} | |||
|
|||
# Configure Ethereum address | |||
if [ "${ETHEREUM_EXTERNAL_HOST}" != "" ]; then | |||
ETHEREUM_ADDR="${ETHEREUM_EXTERNAL_HOST}" | |||
if [ "${EXTERNAL_ETHEREUM_HOST}" != "" ]; 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.
Sorry about this one. Hope you didn't lose much time.
-var="VALIDATOR_KEYS=${{ secrets.VALIDATOR_KEYS }}" \ | ||
-var="BOOT_NODE_SEQ_PUBLISHER_PRIVATE_KEY=${{ secrets.BOOT_NODE_SEQ_PUBLISHER_PRIVATE_KEY }}" \ | ||
-var="PROVER_NODE_PROVER_PUBLISHER_PRIVATE_KEY=${{ secrets.PROVER_NODE_PROVER_PUBLISHER_PRIVATE_KEY }}" \ | ||
-var="ETHEREUM_EXTERNAL_HOST=${{ secrets.SEPOLIA_EXTERNAL_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.
Is this a secret because it has the API key in the 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.
yep 👍
set_list { | ||
name = "validator.validatorKeys" | ||
value = var.VALIDATOR_KEYS | ||
} |
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.
set_list { | |
name = "validator.validatorKeys" | |
value = var.VALIDATOR_KEYS | |
} | |
dynamic "set_list" { | |
for_each = length(var.VALIDATOR_KEYS) > 0 ? [1] : [] | |
content { | |
name = "validator.validatorKeys" | |
value = var.VALIDATOR_KEYS | |
} | |
} |
I'm pretty sure the code as written will break deployments with the deploy-network
workflow because the new variables don't have a default value, so the terraform command will just hang awaiting input. But also, if we set the default values to be empty, we need a dynamic block like the suggestion so that we only set when the values are not empty so that we actually use the values in the underlying yaml file.
|
||
variable "ETHEREUM_EXTERNAL_HOST" { | ||
description = "External host to use for the ethereum node" | ||
type = string |
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.
So this should be sensitive? Also, as noted in main.tf, I think all of these need defaults to support the existing flows.
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.
super nit, but can we just rename this to exp-2.yaml
?
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! Thanks!
* master: (119 commits) chore(master): Release 0.68.0 (#10834) feat: enable profiling with local PXE in cli-wallet (#10736) chore: values for sepolia deployment (#10362) fix: vm_full_tests.yml (#10912) chore: disable bb-sanitizers.yml (#10901) chore: Check yarn version during bootstrap (#10910) fix(p2p): default peer score penalties (#10896) feat: Updated metrics (#10885) feat: Reduce bundle sizes, remove polyfills (#10877) chore: Replace bbup.dev link (#10908) chore(avm): extra column information in lookups (#10905) git subrepo push --branch=master noir-projects/aztec-nr git_subrepo.sh: Fix parent in .gitrepo file. [skip ci] chore: replace relative paths to noir-protocol-circuits git subrepo push --branch=master barretenberg chore: disable flake in e2e_fees/private_payments (#10898) chore(ci): disable e2e_cheat_codes.test.ts (#10897) fix: increase default heartbeat (#10891) fix(p2p): less verbose error (#10886) refactor: `contact` --> `sender` in PXE API (#10861) ...
Fixes #9926