The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33051.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #34547 (lint: modernise lint tooling by willcl-ark)
• #34427 (lint: Flatten lint image entry points by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

the lint CI failed

https://cirrus-ci.com/task/4540580631412736?logs=build#L1735:

[12:20:39.690] 38.29 ++ cat /.python-version
[12:20:39.690] 38.29 + env CC=clang python-build 3.10 /python_build
[12:20:39.690] 38.31 python-build: definition not found: 3.10
[12:20:39.690] ------

Ah I see. For some reason the linter is calling python-build directly instead of pyenv install, which just picks a patch version. Added a workaround.

E.g. if the user installed Python 3.10.14 and 3.10.16 through PyEnv, if HWI was locked to 3.10 it would default to 3.10.16. HWI then can't find any of its dependencies.

Then the user should reinstall the HWI dependencies, given they've changed Python version. They would have the same issue any time they update their Python, to a newer 3.10.x version.

Regardless, can you solve your issue using something out of https://github.com/pyenv/pyenv?tab=readme-ov-file#understanding-python-version-selection. Us having to change our Python version selection, to accomodate the Python version selection of a different project, would seem to undermine the point of both projects using Python version management.

It's not really a problem for me, but I noticed HWI stopped using patch versions in bitcoin-core/HWI#695 and I don't think there's a good reason for us to insist on one.

Everywhere else in the project we only specify the Python minor version. And if there's ever a security issue with the patch version we picked, we probably should go and update it (which we've never done).

and I don't think there's a good reason for us to insist on one.

Well at least one is that we don't need to add more code/"workarounds", for a problem we don't have. (#33051 (comment))

But why are we cloning the PyEnv repo, but then not just using pyenv install 3.10? Then the workaround wouldn't be needed.

but then not just using pyenv install

pyenv install is a wrapper around python-build. Looks like it was used to use slightly less resources.

It's defined here: https://github.com/pyenv/pyenv/blob/master/plugins/python-build/bin/pyenv-install

Which internally calls pyenv-latest --known: https://github.com/pyenv/pyenv/blob/master/libexec/pyenv-latest

Which it turn calls python-build --definitions as I did.

lgtm ACK e71990fabc53c53d1524ea00851e0d9a67a8f86f

CI: https://cirrus-ci.com/task/5023354685489152?logs=build#L1722

[15:02:47.597] #10 36.72 ++ cat /.python-version
[15:02:47.813] #10 36.72 + PYTHON_VERSION=3.10
[15:02:47.813] #10 36.72 ++ python-build --definitions
[15:02:47.813] #10 36.72 ++ grep '^3.10'
[15:02:47.813] #10 36.72 ++ tail -1
[15:02:47.813] #10 36.73 + PYTHON_PATCH_VERSION=3.10.18
[15:02:47.813] #10 36.73 + env CC=clang python-build 3.10.18 /python_build
[15:02:47.813] #10 36.78 Downloading Python-3.10.18.tar.xz...
[15:02:47.813] #10 36.78 -> https://www.python.org/ftp/python/3.10.18/Python-3.10.18.tar.xz
[15:02:49.055] #10 38.17 Installing Python-3.10.18...
[15:04:10.084] #10 119.2 Installed Python-3.10.18 to /python_build
[15:04:10.275] #10 119.4 + export PATH=/python_build/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
[15:04:10.275] #10 119.4 + PATH=/python_build/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
[15:04:10.275] #10 119.4 + command -v python3
[15:04:10.275] #10 119.4 + python3 --version
[15:04:10.427] #10 119.4 /python_build/bin/python3
[15:04:10.427] #10 119.4 Python 3.10.18

In theory this could increase CI non-determinism, but we are using Ubuntu as base image, so that goal is already lost.

The PR description and commit message will need re-writing, to explain the motivation, as the current motivaiton is just user error (updating python and not reinstalling dependencies).

But why are we cloning the PyEnv repo, but then not just using pyenv install 3.10? Then the workaround wouldn't be needed.

If we are going to change anything here, then we might as well swap to this. Given the original motivation is unclear, and it'd be less lines of code.

Original discussion: #26716 (comment)

I reckon the lower memory usage is due to using clang instead of gcc.

pyenv install also honors CC=clang (tested by setting CC=fkfkkf which fails)

it'd be less lines of code.

It removes a few lines for the installer, but adds some for pyenv global and pyenv init.

Using PyEnv directly does make it easier to use multiple Python versions, in case any of our linters would benefit from a more recent version.

I updated the commit message and PR description.

(added rationale to the first commit message)

Seems fine to change. I just recall another proposed change in #34391 (comment) which conflicts with this pull?

Again, seems fine. It would just not be ideal if this was changing code that is removed in the next commit/pull anyway.

Rebased just in case.

I just recall another proposed change in #34391 (comment) which conflicts with this pull?

@willcl-ark would it?

Yes, it modifies the same lines of code, which is a git merge conflict.

Rebased after #34427, applying changes to 06_script.sh instead of container-entrypoint.sh.

I didn't notice a problem with #34391 in the rebase fwiw, but haven't investigated it.

Spotted this again from drahtbot linking it in #34547

Concept ACK on dropping the patch version.

My personal preference on the second commit though would be to switch to uv rather than modifying the pyenv install method as I have done in #34547, as IMO, uv is just all-round better tooling at this stage. If you agreed on this approach, we could either drop the first commit here, or I could cherry-pick d7c0170 into #34547?

@willcl-ark I'm not familiar with uv, so if you can cherry-pick d7c0170 that works for me.

I have cherry-picked into #34547. I'll let you know in here if that PR isn't merged so you can re-open this one if you want.