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

Replace local testnet script with Kurtosis #5865

Merged
merged 13 commits into from
Jun 4, 2024

Conversation

jimmygchen
Copy link
Member

@jimmygchen jimmygchen commented May 30, 2024

Issue Addressed

The local testnet script has become quite difficult to maintain over time and using Kurtosis for local testing is now the prefer approach.

This PR replaces the scripts with a kurtosis config file and runs the same test on CI.

TODO

  • Migrate doppelganger tests to kurtosis
  • Remove lcli commands that are no longer needed (e.g. new-testnet, insecure-validators etc)
  • Update Lighthouse book and README

Future considerations

@jimmygchen jimmygchen force-pushed the local-testnet branch 3 times, most recently from 9df8055 to 1dca23d Compare May 30, 2024 05:05
@jimmygchen jimmygchen force-pushed the local-testnet branch 3 times, most recently from 094c5d8 to aa8dba7 Compare May 30, 2024 05:35
@jimmygchen jimmygchen added the skip-ci Don't run the `test-suite` label May 30, 2024
@realbigsean
Copy link
Member

legend

b) BUILD_IMAGE=${OPTARG};;
n) NETWORK_PARAMS_FILE=${OPTARG};;
p) BUILDER_PROPOSALS=true;;
c) CI=true;;
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't include DEBUG_LEVEL and VC_COUNT, as they are already in network_parameters.yaml and is easy to modify if needed rather than adding a bunch of flags.

Some reasoning for the default values:

  • BUILD_IMAGE=true: when testing locally, we usually want to test the code in working directory, so this is on by default. If anyone wants to use latest image, can also set this to false and update the image in network_parameters.yaml file
  • CI=false: when testing locally, a block explorer and metrics explorer is usually handy but not useful on CI.

Copy link
Member

Choose a reason for hiding this comment

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

Wondering if its easier from the end user perspective if BUILD_IMAGE is the actual docker image that the user wants to use, and if nothing is specified, then we build the local image and use it?

e.g.

# Uses the remote docker image
./start_local_testnet -b ethpandaops/lighthouse:unstable
# Builds local image and uses that
./start_local_testnet

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the boolean flag may still be useful in the case of local testing, because if a user already have a local image and want to re-use it, then they could run it without rebuilding, and sometimes they would want to rebuild.

My thoughts around designing the params is to prefer the use the network_parameters.yaml file where possible - this essentially replaces the vars.env - and avoid having config in two places for simplicity and not having to worry about priority.

Same reason why I removed DEBUG_LEVEL and VC_COUNT from the scripts. I'm not a big fan of maintaining large shell scripts and would love to keep functionalities as small as possible.

@jimmygchen jimmygchen force-pushed the local-testnet branch 2 times, most recently from 649d699 to 48c8d40 Compare May 31, 2024 05:54
@pawanjay176
Copy link
Member

The issue with the doppelganger tests was that the doppelganger VC was starting before epoch 1 and we currently don't support that prior to epoch 1 . See

let remaining_epochs = if current_epoch <= genesis_epoch {
// Disable doppelganger protection when the validator was initialized before genesis.
//
// Without this, all validators would simply miss the first
// `DEFAULT_REMAINING_DETECTION_EPOCHS` epochs and then all start at the same time. This
// would be pointless.
//
// The downside of this is that no validators have doppelganger protection at genesis.
// It's an unfortunate trade-off.
0

Fixed that to start the VC exactly on epoch 1 and its working fine. Although we still need to capture the exit code correctly i think.

@jimmygchen jimmygchen force-pushed the local-testnet branch 2 times, most recently from cc60edd to f2f5113 Compare May 31, 2024 12:53
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed skip-ci Don't run the `test-suite` labels May 31, 2024
Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

Looks good to me overall. Just a few minor nits.
I'm happy with merging this as is and addressing them in the subsequent docs PR.

scripts/local_testnet/stop_local_testnet.sh Show resolved Hide resolved
scripts/local_testnet/network_params.yaml Outdated Show resolved Hide resolved
scripts/local_testnet/start_local_testnet.sh Show resolved Hide resolved
b) BUILD_IMAGE=${OPTARG};;
n) NETWORK_PARAMS_FILE=${OPTARG};;
p) BUILDER_PROPOSALS=true;;
c) CI=true;;
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if its easier from the end user perspective if BUILD_IMAGE is the actual docker image that the user wants to use, and if nothing is specified, then we build the local image and use it?

e.g.

# Uses the remote docker image
./start_local_testnet -b ethpandaops/lighthouse:unstable
# Builds local image and uses that
./start_local_testnet

@jimmygchen
Copy link
Member Author

@mergify queue

Copy link

mergify bot commented Jun 4, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 5fc0145

@jimmygchen jimmygchen added the ready-for-merge This PR is ready to merge. label Jun 4, 2024
mergify bot added a commit that referenced this pull request Jun 4, 2024
@jimmygchen jimmygchen removed the ready-for-review The code is ready for review label Jun 4, 2024
@jimmygchen
Copy link
Member Author

Thanks for the review @pawanjay176 , happy to discuss / address the comments above further.

mergify bot added a commit that referenced this pull request Jun 4, 2024
@mergify mergify bot merged commit 5fc0145 into sigp:unstable Jun 4, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra-ci ready-for-merge This PR is ready to merge. test improvement Improve tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants