Skip to content
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

Merged
merged 31 commits into from
Dec 20, 2024
Merged

Conversation

spypsy
Copy link
Member

@spypsy spypsy commented Dec 3, 2024

Fixes #9926

@spypsy spypsy marked this pull request as ready for review December 3, 2024 10:43
Base automatically changed from spy/gas-utils to master December 4, 2024 13:05
@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why drop the PXE?

Copy link
Member Author

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

Copy link
Collaborator

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}
Copy link
Collaborator

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.

"$(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"
Copy link
Member

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
Copy link
Collaborator

@just-mitch just-mitch Dec 13, 2024

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.

Copy link
Member Author

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 }}
Copy link
Collaborator

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
Copy link
Collaborator

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 }}" \
Copy link
Collaborator

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep 👍

Comment on lines 70 to 73
set_list {
name = "validator.validatorKeys"
value = var.VALIDATOR_KEYS
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Collaborator

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.

Copy link
Collaborator

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?

Copy link
Collaborator

@just-mitch just-mitch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks!

@spypsy spypsy enabled auto-merge (squash) December 20, 2024 15:39
@spypsy spypsy merged commit 74cfe0d into master Dec 20, 2024
49 checks passed
@spypsy spypsy deleted the spy/sepolia-spartan-updates branch December 20, 2024 16:14
TomAFrench added a commit that referenced this pull request Dec 20, 2024
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System run on a kube cluster
3 participants