Skip to content

Conversation

@roshii
Copy link
Contributor

@roshii roshii commented Oct 25, 2025

  • Introduced a new compose.yml file for running tests in an isolated Docker environment.
  • Updated Dockerfile to support multi-stage builds, including a dedicated test stage with Bitcoin Core.
  • Enhanced install.sh to allow installation of dependencies only, without building JoinMarket.
  • Updated documentation to include Docker usage instructions for building images and running tests.

Dockerfile Outdated
&& apt-get clean \
&& rm -rf /var/lib/apt/lists/*

FROM bitcoin/bitcoin:29.2 AS bitcoin
Copy link

@3nprob 3nprob Oct 26, 2025

Choose a reason for hiding this comment

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

As tests use compose anyway, why not remove/split out the bitcoin and test stages from the production Dockerfile? I don't want to redundantly have to pull the bitcoin image every time I build in a clean environment.

If a bundled image for testing is desired, I suggest putting that under test/Dockerfiles/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the test or bitcoin layer are not included in the final one.

$  docker build .
[+] Building 9.0s (12/12) FINISHED                                                                                                                                          docker:desktop-linux
 => [internal] load build definition from Dockerfile                                                                                                                                        0.0s
 => => transferring dockerfile: 717B                                                                                                                                                        0.0s
 => [internal] load metadata for docker.io/library/python:3.11-slim-trixie                                                                                                                  0.9s
 => [internal] load .dockerignore                                                                                                                                                           0.0s
 => => transferring context: 67B                                                                                                                                                            0.0s
 => [joinmarket 1/7] FROM docker.io/library/python:3.11-slim-trixie@sha256:8eb5fc663972b871c528fef04be4eaa9ab8ab4539a5316c4b8c133771214a617                                                 0.0s
 => [internal] load build context                                                                                                                                                           0.1s
 => => transferring context: 1.70MB                                                                                                                                                         0.1s
 => CACHED [joinmarket 2/7] WORKDIR /jm/clientserver                                                                                                                                        0.0s
 => CACHED [joinmarket 3/7] COPY ./pubkeys ./pubkeys                                                                                                                                        0.0s
 => CACHED [joinmarket 4/7] COPY ./install.sh ./install.sh                                                                                                                                  0.0s
 => CACHED [joinmarket 5/7] RUN apt-get update   && apt-get install -y --no-install-recommends     gnupg     ca-certificates     curl   && ./install.sh --docker-install --deps-only   &&   0.0s
 => [joinmarket 6/7] COPY . .                                                                                                                                                               0.1s
 => [joinmarket 7/7] RUN apt-get update   && ./install.sh --docker-install --no-deps   && apt-get clean   && rm -rf /var/lib/apt/lists/*                                                    7.7s
 => exporting to image                                                                                                                                                                      0.3s 
 => => exporting layers                                                                                                                                                                     0.3s 
 => => writing image sha256:17292e6798277f17d6542eb9370083c031add6e477739e2938767f3bb8135263    

Copy link

@3nprob 3nprob Oct 26, 2025

Choose a reason for hiding this comment

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

Suggestion: to minimize confusion, indicate that this is intended for tests by e.g. moving to test/Dockerfiles/compose.yml or test/docker-compose.yml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is meant to be extended at some point, should not be restricted to test imho. Further to that, developer experience feels better to me if you can run all from root folder

Copy link

Choose a reason for hiding this comment

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

Extension can still happen in separate Dockerfiles, no?

Docker is generally "smart" enough to reuse cache layers even if you build from a separate Dockerfile.

Further extending the production Dockerfile and making it more complex without benefiting the runtime image is an antipattern IMO. You can still build and run it from the root directory even if the Dockerfile is in a subdir.

Dockerfile Outdated
gnupg \
ca-certificates \
curl \
&& ./install.sh --docker-install --deps-only \
Copy link

Choose a reason for hiding this comment

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

Suggested change
&& ./install.sh --docker-install --deps-only \
&& apt-get purge -y --autoremove python3-pip build-essential python3-dev python3-pip automake pkg-config libffi-dev libssl-dev \

these packages are not necessary at runtime so can be removed to save space.

(feel free to ignore; I can make a separate PR for that)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can only agree. Install dependencies optimization deserves a PR on its own imo, we could further optimize layers with a more granular script.

Copy link

Choose a reason for hiding this comment

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

Dockerfile Outdated
COPY --from=bitcoin /opt/bitcoin-29.2/bin /usr/local/bin/
RUN ./install.sh --docker-install --no-deps --develop

FROM joinmarket AS final
Copy link

Choose a reason for hiding this comment

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

IMO a major benefit of multi-stage builds is to do the build in a separate branch from the final image, basing the final image on a clean base such as debian:slim or python:3.12-slim, and copy over the necessary artefacts from the build image (like you are doing with bitcoin here). Since build scripts have never been run in the context of the final image this way, this can have both security and space-efficiency benefits.

Did you consider this approach? What's the benefit of this approach of multi-stage where the final image is simply a copy of the build step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, this multi layer approach allows to extend the main layer with test dependencies, no more. Ideally we indeed want a thin final layer, which requires a refactoring of the install script imo, in a subsequent PR.

@roshii roshii force-pushed the build/docker branch 3 times, most recently from 1c13e06 to 8ebbdef Compare October 26, 2025 18:12
@3nprob
Copy link

3nprob commented Oct 27, 2025

nACK on colocating bitcoin and joinmarket in the same image.

This gives joinmarket full (root) access to the bitcoin daemon (and vice versa). Resolving this is possible but would make the Dockerfile a lot more complex for little benefit when the idiomatic approach is using separate containers and link them in the compose file.

We don't want users to be lured into running such a weakened setup in prod. Having a more representative setup for tests is good.

I do think joinmarket could benefit from a multi-stage build that separates the build- and runtime steps for joinmarket-clientserver itself but that's not what this is.

BTW, this results in a final production image size increase of 14% (without bitcoind).

@roshii
Copy link
Contributor Author

roshii commented Oct 28, 2025

a couple minor updates, most notably a fix introducing an entrypoint to final image allowing command execution as before refactoring, despite virtualenv now being used in docker image

  • chore: rename script flags for consistency
  • chore: revert to python 3.12 image
  • chore: ignore docker files
  • fix: add image entrypoint

@roshii
Copy link
Contributor Author

roshii commented Oct 28, 2025

This gives joinmarket full (root) access to the bitcoin daemon (and vice versa). Resolving this is possible but would make the Dockerfile a lot more complex for little benefit when the idiomatic approach is using separate containers and link them in the compose file.

bitcoin binaries are not included in the final image.

$ docker build -t jm .
[...]
$ docker run -it --rm jm which bitcoind || echo "bitcoind not found"  
bitcoind not found

@roshii roshii force-pushed the build/docker branch 2 times, most recently from 3439e37 to 91875d3 Compare October 29, 2025 09:06
@roshii
Copy link
Contributor Author

roshii commented Oct 29, 2025

a couple minor updates:

  • fix: add shellcheck directive
  • refactor: keep install.sh simple

@roshii roshii force-pushed the build/docker branch 2 times, most recently from 2dd8928 to 852dee9 Compare November 1, 2025 13:46
@roshii
Copy link
Contributor Author

roshii commented Nov 1, 2025

Another refactoring, leaving install.sh intact, and optimizing layers.

H/T to @3nprob and his work on #1818 for the inspiration

@roshii
Copy link
Contributor Author

roshii commented Nov 2, 2025

  • rebased
  • amended to py 3.13 image
  • added obwatcher layer and service

@roshii roshii changed the title build: multi-stage builds and testing configuration build: multi-stage builds and services configuration Nov 11, 2025
@roshii
Copy link
Contributor Author

roshii commented Nov 11, 2025

my previous iteration on this pull request, adding obwatcher service, was not satisfying since it implied hard coding yet another config file for a single service. i have therefore work on #1823 that allows overcoming this limitation in different scenario, and on which this pr now depends.

other than leveraging this proposed feature, this pr is amended to consistently use editable install, and implement a base service that others can simply extend, avoiding repetition.

- Updated `Dockerfile` to support multi-stage builds
- Introduced a new `compose.yml` file for service definition
- Updated documentation

build: consistently use editable install

build: extend base service

feat: obwatcher config from environment variable

revert: obwatcher
@roshii
Copy link
Contributor Author

roshii commented Nov 23, 2025

removed obwatcher service from this pr, which will come in a subsequent pr. this pr has no more dependency.

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.

2 participants