Drop Python 3.9, use 3.10 for dist, bump Ledger - #819
Conversation
8489c74 to
57f6690
Compare
57f6690 to
a608ea3
Compare
|
Rebased after #818 landed, ready for review. I lightly tested the dist builds on Ubuntu (x86_64, including qt) and macOS (only command line). I ran the GUI in Wine, but that doesn't have USB access so not very informative. I also ran it on a native Windows machine. Windows Defender wasn't happy about it, but I didn't do any code signing. There it detected a connected device, so that's good. I wonder if it makes sense for the deterministic binary releases to jump straight ahead to a more modern Python? Also, would it make sense to do one final HWI release with Python 3.9 before merging this? |
|
Added commit to re-enable the Ledger tests. |
a4e3a84 to
ae9e49d
Compare
|
Rebased just in case after recent merges. |
|
Does the dist build work on in an arm docker container, as described in the release process? |
ae9e49d to
f473a2e
Compare
I built inside an arm64 docker container on the x86 linux machine. I also made it do an Rebased again, this time using The first time I ran that command, it moved I also built and tested the x86 macOS build on a native machine. I was able to run the QT application and it connected to a test deviced and fetch the descriptors. Checksums: |
|
The spurious Ledger failure appears to be the result of the test suite occasionally hitting the Cancel / Reject button. Where Added a commit to drop that rule, since it's unused anyway. |
3ac16e0 to
5197ac2
Compare
|
Rebased just in case.
I briefly tested the x86 linux gui. |
|
Added a commit here to fix Trezor CI. Unrelated so can be its own PR if it works. |
c85d39c to
a8bd342
Compare
|
The one bitbox01 and one ledger failures seem spurious. But the jade test failures were real, so I'm pushing a new fix shortly (basically #832). |
d2093d9 to
d21b545
Compare
|
Actually #832 did not do the trick. This time I ran the simulator locally to reproduce the issue. It was the PSBTv2 change on Bitcoin Core's master (again, this change bit me multiple times). I dropped the cbor commits, because they're not needed here, but based on local testing they should not be a problem. I opened a fresh PR for just the CI fixes: #836 |
d21b545 to
712823a
Compare
|
The one failing ledger just is spurious. @andreasgriffin much of this was written with the help of Claude and Codex, so I wouldn't expect it to find anything. Unless you give it a more specific prompt based on your own review, of something that might be incorrect. In general for this Python bump it would be great if someone can test the binaries on every supported platform. I did that before the rebase, so I don't expect anything new to have broken, but still. |
712823a to
887843c
Compare
|
Fable discovered that the one flaky failure here was due to a base58 address ending in It's a pre-existing bug, but I kept the fix be87c6e here, because it only impacts tests that were disabled before this PR. |
Tested (and they work) https://github.com/bitcoin-core/HWI/actions/runs/28703459171/artifacts/8080924437 (from tests) wheel file on
|
d3e4ce7 test: only match Ledger "To" screen on the title row (Sjors Provoost) ca2caf0 psbt: don't overwrite PSBTv2 tx version and fallback locktime (Sjors Provoost) f15d6d9 Fix repo renamings for Ledger apps (Salvatore Ingala) 7ac09cf ci: drop unused Python 3.7 Docker file (Sjors Provoost) 9ded73e ci: drop cirrus leftovers (Sjors Provoost) eb36bd6 test: pin Trezor T Rust nightly to 2025-04-15 (Sjors Provoost) Pull request description: Commits are taken from #819 (which is now based on this). - Trezor builds started to fail when Rust nightly introduced some new rule, so we pin the nightly - bitcoin/bitcoin#21283 broke the Jade tests (and probably more than that) Two minor commits drop unused CI code. Ledger moved repos around, so also include: - #837 ACKs for top commit: achow101: ACK d3e4ce7 Tree-SHA512: d9b9bc47a9f1d72bb13166edc382f545eefcac19472883e929d23dc879c7dd497d85b2719329095f0a121337912248f3956ec280b3b49edcf0499e809b09b0f5
887843c to
b43b016
Compare
|
Rebased after #836. |
|
c0990a1 is incorrect and should be dropped. It excludes the ui files necessary for |
|
Edit: This was a local docker issue apparently. Building the arm container fails: |
|
@achow101 Codex says it's your fault :-)
|
831193d to
76bd097
Compare
Well, since I do the releases, if it doesn't work on my machine, it ain't gonna be merged. |
|
@achow101 did the suggestion work? I can't reproduce the issue, so will need more details. |
|
The current error is |
|
Apparently I have to disable ASLR... |
| git clone --branch 2.4.6 --depth 1 https://github.com/LedgerHQ/app-bitcoin.git | ||
| cd app-bitcoin | ||
| make DEBUG=1 BOLOS_SDK=$NANOX_SDK | ||
| # Work around register-wallet policy parsing/display bugs fixed after v2.4.6. |
There was a problem hiding this comment.
In 50da3f1 "Bump Speculos and Ledger Bitcoin app"
Can we instead pin a commit that has these included instead of cherry picking them? We're already building from source anyways.
There was a problem hiding this comment.
I bumped to 2.5.0, which has been released since and contains these fixes.
| # Work around -Werror build failures in Speculos' bundled deps. | ||
| # GCC < 15 errors out on unknown "-Wno-error=..." options, so only add the | ||
| # unterminated-string-initialization suppression when the compiler supports it. | ||
| CFLAGS="-O -fno-builtin -fPIC -Wall -Wextra -Werror -Wno-error=maybe-uninitialized -Wno-error=array-parameter -Wno-error=array-bounds -Wno-error=discarded-qualifiers" |
There was a problem hiding this comment.
In 50da3f1 "Bump Speculos and Ledger Bitcoin app"
Where did these flags come from? I don't see them in the speculos codebase at all.
There was a problem hiding this comment.
Dropped. Not sure if this was useful in earlier versions (it's been there since at least January), but it's definitely not needed now.
76bd097 to
8ae811e
Compare
Most device simulators accept arbitrary keypool paths, so keep testing that behavior by default. Some devices enforce their own derivation path policies. Let those emulators opt out of the arbitrary-path portion while still running the remaining keypool checks.
It's unused and occasionally trips up a test.
Limit the warning rule to the centered title so it does not also click the repeated header on the details screen. This prevents the queued extra click from selecting Back to safety before Continue anyway is confirmed.
8ae811e to
ac68456
Compare
|
Rebased after #838 and addressed inline feedback. |
4 participants
This PR first switches the deterministic builds to Python 3.10. It then drops the end-of-life 3.9.
Then it bumps Speculos and Ledger and enables the tests that were previously disabled.
There's also a few minor cleanup commits, plus changes to make the Docker containers Podman friendly so I can easily run them locally.