sigp/lcli container image only built for modern x86 CPUs

[dboreham]

Description

The lcli binary shipped in the sigp/lcli container image is built to run on a CPU with extensions (probably AVX2?) that are not present on all x86 CPUs. The result is that when the tool is run on such a machine, it throws an illegal instruction exception.

Such CPUs are not particularly rare : we have encountered the issue both in GitHub runners and in VMs in our internal build infra.

Version

Tested this container image and also a local built from the current stable branch.

Present Behaviour

The lcli tool, when run with the insecure-validators subcommand, throws an illegal instruction exception on some CPUs.

e.g.:

$  ./lcli insecure-validators --count 4 --base-dir $(pwd) --node-count 4
Validator 1
Illegal instruction (core dumped)

Expected Behaviour

Should not throw illegal instruction exception.

Steps to resolve

The lcli container should be built with a portable compiler target, but it isn't.

[dboreham]

It appears that although the Dockerfile has the arg PORTABLE which is set =true in the build script, PORTABLE doesn't actually do anything:

$ grep -R PORTABLE .
./.github/workflows/docker.yml:                      --build-arg PORTABLE=true \
./lcli/Dockerfile:ARG PORTABLE
./lcli/Dockerfile:ENV PORTABLE $PORTABLE

[dboreham]

Related: #1395

[dboreham]

This fixes it for me:

diff --git a/lcli/Dockerfile b/lcli/Dockerfile
index 98f33f215..485ebfd5b 100644
--- a/lcli/Dockerfile
+++ b/lcli/Dockerfile
@@ -6,7 +6,7 @@ RUN apt-get update && apt-get -y upgrade && apt-get install -y cmake libclang-de
 COPY . lighthouse
 ARG PORTABLE
 ENV PORTABLE $PORTABLE
-RUN cd lighthouse && make install-lcli
+RUN if [ "$PORTABLE" = true ] ; then export FEATURES=portable; fi && cd lighthouse && make install-lcli

 FROM ubuntu:22.04
 RUN apt-get update && apt-get -y upgrade && apt-get clean && rm -rf /var/lib/apt/lists/*

$ ./lcli-fixed insecure-validators --count 4 --base-dir $(pwd) --node-count 4
Validator 1
WARN: Scrypt parameters are too weak (n: 2, p: 1, r: 8), we recommend (n: 262144, p: 1, r: 8)
Validator 2
WARN: Scrypt parameters are too weak (n: 2, p: 1, r: 8), we recommend (n: 262144, p: 1, r: 8)
Validator 3
WARN: Scrypt parameters are too weak (n: 2, p: 1, r: 8), we recommend (n: 262144, p: 1, r: 8)
Validator 4
WARN: Scrypt parameters are too weak (n: 2, p: 1, r: 8), we recommend (n: 262144, p: 1, r: 8)

[michaelsproul]

Good catch, thanks

Would you mind opening a PR to fix it? My preffered fix would be to remove the PORTABLE arg and pass FEATURES through from the Dockerfile. The existing makefile will then use those features here:

lighthouse/Makefile

Lines 53 to 58 in c547a11

	# Builds the lcli binary in release (optimized).
	install-lcli:
	cargo install --path lcli --force --locked \
	--features "$(FEATURES)" \
	--profile "$(PROFILE)" \
	$(CARGO_INSTALL_EXTRA_FLAGS)

We should also update the Github action that builds the image so that it uses FEATURES=portable instead of PORTABLE=true:

lighthouse/.github/workflows/docker.yml

Lines 131 to 148 in c547a11

	build-docker-lcli:
	runs-on: ubuntu-22.04
	needs: [extract-version]
	env:
	VERSION: ${{ needs.extract-version.outputs.VERSION }}
	VERSION_SUFFIX: ${{ needs.extract-version.outputs.VERSION_SUFFIX }}
	steps:
	- uses: actions/checkout@v3
	- name: Dockerhub login
	run: |
	echo "${DOCKER_PASSWORD}" | docker login --username ${DOCKER_USERNAME} --password-stdin
	- name: Build lcli dockerfile (with push)
	run: |
	docker build \
	--build-arg PORTABLE=true \
	--tag ${LCLI_IMAGE_NAME}:${VERSION}${VERSION_SUFFIX} \
	--file ./lcli/Dockerfile .
	docker push ${LCLI_IMAGE_NAME}:${VERSION}${VERSION_SUFFIX}