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 react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

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 e71990f

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)