feat: gating test #9918

just-mitch · 2024-11-13T01:55:03Z

Main feature of this branch (unlike the branch name indicates) is the gating_passive.test.ts.

This test:

updates the aztec network deployment, allowing validators to use each other as boot nodes
applies the "network-requirements" network shaping
permanently disables the boot node
runs 3 epochs during which it:
- kills 25% of the validators
- asserts that we miss less than 50% of slots

Other work in this branch includes:

allows sequencers to start building as soon as they see they can propose in the next L1 slot
add ignoreDroppedReceiptsFor TX wait options
- this allows sending a TX to one node, and awaiting it on another since we need time for p2p propagation
- we need this since we have shifted the PXE to point at the top-level validator service, which load balances across individuals
- this may help with triage: reorged chain needs to "restabilize" before running transactions #9613
scalable loki deployment for prod
more visible logging for core sequencer operations
better error handling during the setup of l2 contracts
better error handling in the pxe
rename the network shaping charts to "aztec-chaos-scenarios"

Fix #9713
Fix #9883

spartan/aztec-chaos-scenarios/values/prover-failure.yaml

spartan/aztec-chaos-scenarios/values/validator-kill.yaml

just-mitch · 2024-11-13T17:12:32Z

spartan/aztec-network/templates/_helpers.tpl

@@ -78,7 +78,7 @@ http://{{ include "aztec-network.fullname" . }}-boot-node-0.{{ include "aztec-ne
 {{- if .Values.validator.externalTcpHost -}}
 http://{{ .Values.validator.externalTcpHost }}:{{ .Values.validator.service.nodePort }}
 {{- else -}}
-http://{{ include "aztec-network.fullname" . }}-validator-0.{{ include "aztec-network.fullname" . }}-validator.{{ .Release.Namespace }}.svc.cluster.local:{{ .Values.validator.service.nodePort }}


Adjusts validatorUrl to load balance over all validators.

just-mitch · 2024-11-13T17:14:44Z

spartan/aztec-network/templates/validator.yaml

+                      # the PXE has its node set to the the validator service.
+                      # and that's all we need to know to bootstrap,
+                      # since it will get a good node ENR from whatever node the PXE connects to.
+                      echo "{{ include "aztec-network.pxeUrl" . }}" > /shared/pxe_url


As noted in the comment above, this looping is really just a safety/sanity check, since as long as one validator is up, the PXE should be routed to it.

just-mitch · 2024-11-13T17:16:11Z

spartan/aztec-network/values/4-validators-with-metrics.yaml

smallest set where we can kill 25%.

just-mitch · 2024-11-13T17:16:52Z

spartan/metrics/install-prod.sh

@@ -3,4 +3,4 @@ set -eu

 cd "$(dirname "${BASH_SOURCE[0]}")"

-helm upgrade metrics . -n metrics --values "./values/prod.yaml" --install --create-namespace --atomic $@


Happy to revert this change, but removing atomic was necessary to try to debug.

yarn-project/protocol-contracts/src/protocol_contract_data.ts

just-mitch · 2024-11-13T17:22:55Z

yarn-project/sequencer-client/src/sequencer/sequencer.ts

-      [SequencerState.PUBLISHING_BLOCK_TO_PEERS]: 5 + this.maxTxsPerBlock * 2, // if we take 5 seconds to create block, then 4 transactions at 2 seconds each
-      [SequencerState.WAITING_FOR_ATTESTATIONS]: 5 + this.maxTxsPerBlock * 2 + 3, // it shouldn't take 3 seconds to publish to peers
-      [SequencerState.PUBLISHING_BLOCK]: 5 + this.maxTxsPerBlock * 2 + 3 + 5, // wait 5 seconds for attestations
+      [SequencerState.WAITING_FOR_TXS]: 5,


Had to bump these times, as we were often trying to transition to "WAITING_FOR_TXS" at ~3.5 seconds, which was resulting in no blocks getting produced, which resulted in reorgs due at least in part to lack of support for #9404

just-mitch · 2024-11-13T17:24:14Z

yarn-project/sequencer-client/src/sequencer/sequencer.ts

      }

      const txHashes = validTxs.map(tx => tx.getTxHash());

      this.isFlushing = false;
-      this.log.verbose('Collecting attestations');
+      this.log.info('Collecting attestations');


@ludamad I'm actually not seeing these logs under in loki even with debug on. Thoughts? I'm not sure if it is all verbose logs, or what. That was part of the impetus for upping the level.

I filed #9958

Unfortunately I am not sure. We should write good tests along with moving to a non-node debugger (winston? pino?)

This test: - updates the aztec network deployment, allowing validators to use each other as boot nodes - applies the "network-requirements" network shaping - permanently disables the boot node - runs 3 epochs during which it: - kills 25% of the validators - asserts that we miss less than 50% of slots Other work in this branch includes: - add `ignoreDroppedReceiptsFor` TX wait options - this allows sending a TX to one node, and awaiting it on another since we need time for p2p propagation - we need this since we have shifted the PXE to point at the top-level validator service, which load balances across individuals - this may help with #9613 - scalable loki deployment for prod - more visible logging for core sequencer operations - better error handling during the setup of l2 contracts - better error handling in the pxe - rename the network shaping charts to "aztec-chaos-scenarios"

stevenplatt · 2024-11-20T14:54:28Z

Taking a look at this one now...

spalladino

Looked into all non-k8s stuff

spalladino · 2024-11-20T14:55:25Z

yarn-project/end-to-end/src/spartan/utils.ts

+export async function awaitL2BlockNumber(
+  rollupCheatCodes: RollupCheatCodes,
+  blockNumber: bigint,
+  timeoutSeconds: number,
+  logger: Logger,
+) {
+  logger.info(`Waiting for L2 Block ${blockNumber}`);
+  let tips = await rollupCheatCodes.getTips();
+  const endTime = Date.now() + timeoutSeconds * 1000;
+  while (tips.pending < blockNumber && Date.now() < endTime) {
+    logger.info(`At L2 Block ${tips.pending}`);
+    await sleep(1000);
+    tips = await rollupCheatCodes.getTips();
+  }
+  logger.info(`Reached L2 Block ${tips.pending}`);
+}


We probably want to throw loudly if we reach the timeout without hitting the target block number.

spalladino · 2024-11-20T14:57:23Z

yarn-project/pxe/src/pxe_service/pxe_service.ts

+      .catch(err => {
+        this.log.error(err);
+        throw err;
+      });


Does the job queue handle jobs that throw ok? Either way, let's add some context to the error being logged.

spalladino · 2024-11-20T14:57:56Z

yarn-project/pxe/src/pxe_service/pxe_service.ts

+      .catch(err => {
+        this.log.error(err);
+        throw err;
+      });


spalladino · 2024-11-20T14:58:01Z

yarn-project/pxe/src/pxe_service/pxe_service.ts

+    await this.node.sendTx(tx).catch(err => {
+      this.log.error(err);
+      throw err;
+    });


spalladino · 2024-11-20T14:58:09Z

yarn-project/pxe/src/pxe_service/pxe_service.ts

+      .catch(err => {
+        this.log.error(err);
+        throw err;
+      });


spalladino · 2024-11-20T15:03:53Z

yarn-project/sequencer-client/src/sequencer/sequencer.ts

I'm not a fan of bumping so many verbose to info, it'll make all tests so much more verbose. But if we need this inorder to see those logs in k8s tests, I'm fine with it until we move to a better logger.

spalladino · 2024-11-20T15:05:09Z

yarn-project/sequencer-client/src/sequencer/sequencer.ts

    if (this.state === SequencerState.STOPPED && force !== true) {
      this.log.warn(
        `Cannot set sequencer from ${this.state} to ${proposedState} as it is stopped. Set force=true to override.`,
      );
      return;
    }
-    const secondsIntoSlot = getSecondsIntoSlot(this.l1GenesisTime, this.aztecSlotDuration);


Why not pass in the current slot number, and keep the getSecondsIntoSlot call here? Seems like it would reduce some verbosity by not having to manually call getSecondsIntoSlot before each setState.

Maddiaa0 · 2024-11-20T15:06:23Z

spartan/aztec-network/templates/setup-l2-contracts.yaml

@@ -32,7 +32,7 @@ spec:
              echo "L2 contracts initialized"
          env:
            - name: PXE_URL
-              value: {{ include "aztec-network.pxeUrl" . | quote }}
+              value: {{ include "aztec-network.bootNodeUrl" . | quote }}


why has this changed?

Maddiaa0 · 2024-11-20T15:10:42Z

yarn-project/end-to-end/src/spartan/utils.ts


 import { exec, spawn } from 'child_process';
 import path from 'path';
 import { promisify } from 'util';
 import { z } from 'zod';

+import type { RollupCheatCodes } from '../../../aztec.js/src/utils/cheat_codes.js';


turbonit: dodgy update import here

stevenplatt · 2024-11-20T15:12:53Z

spartan/aztec-chaos-scenarios/values.yaml

@@ -1,3 +1,6 @@
+nameOverride: null
+fullnameOverride: null
+
 global:
  # When deploying, override the namespace to where spartan will deploy to, this will apply all chaos experiments to all pods within that namespace
  # run deployment with --values global.namespace=your-namespace


Nice, thanks for including this note. It wasn't clear to me at first why the values files used different namespace refs.

stevenplatt · 2024-11-20T15:15:35Z

spartan/aztec-network/values/48-validators.yaml

@@ -9,7 +9,6 @@ telemetry:
  otelCollectorEndpoint: http://metrics-opentelemetry-collector.metrics:4318

 validator:
-  debug: "aztec:*,-aztec:avm_simulator:*,-aztec:libp2p_service"


Does this level of logging causing stability issues? (for my own understanding)

just-mitch linked an issue Nov 13, 2024 that may be closed by this pull request

chore: bump loki space #9883

Open

just-mitch force-pushed the 9883-chore-bump-loki-space branch from 7ad1018 to 9202220 Compare November 13, 2024 02:08

just-mitch marked this pull request as ready for review November 13, 2024 02:13

just-mitch added network-all Run this CI job. hotfix A PR/issue that needs to be cherrypicked back to the RC labels Nov 13, 2024

just-mitch force-pushed the 9883-chore-bump-loki-space branch from 9202220 to c17fe1d Compare November 13, 2024 12:29