Add an option to keep existing enclave when starting local testnet

[jimmygchen]

Issue Addressed

This PR contains the following improvements to local testnet scripts:

1. Add a -k (KEEP_ENCLAVE) option to allow starting the testnet without destroying the existing one. This allows starting from the existing db with an updated image. modifying log levels, add additional services and re-run the testnet without having to destroy and start from scratch, using the -k option of this script. Note that updating image will cause the container to be recreated, and causes the node to lose its state. If all nodes lose their state, the network will stall.

2. Move the additional_services back into the network_params.yaml. Dynamically modifying the network_params file (other than CI) is quite a confusing behaviour, because user's manually added services would get overridden.

3. Add --image-download always to the kurtosis run command to make sure we're not testing against outdated images.

[jimmygchen]

NOTE: This allows kurtosis to evaluate the file against existing enclave and make only necessary changes.

This behaviour is the default and does not require --image-download to be present.

[jimmygchen]

Oops I might be wrong about this. Just ran into some issues locally but didn't have time to investigate. I'll test again tomorrow before we merge this.

[jimmygchen]

Ok so it turns out that -k and image-download=always may not be compatible.

Because if we re-run a testnet with image-download=always, it pulls new image and re-creates all containers (we lose all beacon chain data), and none of the LH node is able to sync from genesis.

However they do work independently and are both useful for the purpose of local testing. I think it's just an edge case we need to keep in mind.

TL;DR:

• we can modify network params, log levels, add additional services and re-run the testnet without having to destroy and start from scratch, using the -k option of this script.
• --image-download=always will make sure we're always testing using the latest image (e.g. geth) and not testing against outdated ones.

Thanks @chong-he for help with testing!

[pawanjay176]

I didn't fully understand. If we rerun with -k option and image-download=always erases the beacon chain data, then wouldn't we need an additional option to toggle image-download=always?

[jimmygchen]

Yes you are right, I thought about this but didn't bother with it because we use lighthouse:local by default and it wouldn't trigger a container recreation, but if user switches to use sigp/lighthouse then it would be an issue - maybe I should only include image-download=always when -k is not present, would this be better? or should we introduce another option? I prefer the former, because If user wants to update any of the images, they probably wouldn't use -k. (I misunderstood the behaviour earlier!)

Unrelated note: The original thinking for this script was to support the common use cases of non kurtosis/ existing users, and avoid turning this into a monster script - I'm still hoping we could keep this small and simple. For more advance users they would potentially prefer to use kurtosis directly for more flexibility. If the additional option is not useful, I'm happy to just drop it.

[pawanjay176]

Yeah maybe we can just drop image-download=always if -k isn't present. That seems like a better option if we don't want to add unnecessarily to the script.

[jimmygchen]

Done!

[chong-he]

Do we need to add -h to the CLI help text?

lighthouse/scripts/local_testnet/start_local_testnet.sh

Lines 28 to 34 in 9e12c21

	echo "Options:"
	echo " -e: enclave name default: $ENCLAVE_NAME"
	echo " -b: whether to build Lighthouse docker image default: $BUILD_IMAGE"
	echo " -n: kurtosis network params file path default: $NETWORK_PARAMS_FILE"
	echo " -p: enable builder proposals"
	echo " -c: CI mode, run without other additional services like Grafana and Dora explorer"
	echo " -h: this help"

I can't comment/give suggestion on collapsed line (not sure if this is doable, found this: https://github.com/orgs/community/discussions/4452), but we can add:

echo " -k: keeping enclave to allow starting the testnet without destroying the existing one"
after line 33

[jimmygchen]

Done, thanks CK!

[jimmygchen]

@mergify queue

[mergify]

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at adfa512