Fix CI ... again#836
Open
Sjors wants to merge 6 commits into
Open
Conversation
The rolling nightly toolchain has drifted past what trezor-firmware core/v2.9.6 supports: recent nightlies reject its reexport_test_harness_main attribute with error E0658. Pin the nightly to 2025-04-15, matching the firmware's own shell.nix. Co-authored-by: Claude (Fable 5) <noreply@anthropic.com>
This was referenced Jul 3, 2026
Draft
Contributor
|
Might want to bring in the commit from #837 (I think the CI would now fail otherwise) |
Member
Author
|
Done! |
Member
Author
|
The keepkey failures seem spurious. |
achow101
reviewed
Jul 9, 2026
Comment on lines
+1050
to
+1052
| # Don't check self.tx instead of self.version: for PSBTv2 it is | ||
| # merely an empty placeholder, and its default version and locktime | ||
| # would overwrite the deserialized values. |
Member
There was a problem hiding this comment.
In 4d97455 "psbt: don't overwrite PSBTv2 tx version and fallback locktime"
This much commentary is not necessary
| # Don't check self.tx instead of self.version: for PSBTv2 it is | ||
| # merely an empty placeholder, and its default version and locktime | ||
| # would overwrite the deserialized values. | ||
| if self.version == 0: |
Member
There was a problem hiding this comment.
In 4d97455 "psbt: don't overwrite PSBTv2 tx version and fallback locktime"
The version 2 checks are kind of split between a version check and if self.tx.is_null(). Could unify them under one pattern in this commit.
Member
Author
There was a problem hiding this comment.
Done.
Claude found a bunch of other issues with the v2 implementation, so I ended up adding all test vectors, and fixes, in an incremental manner. See #839.
This PR only adds the vectors for this bug fix.
Also drops Python 3.6 dataclasses leftover.
The Ledger bitcoin app repos have moved: - new: from LedgerHQ/app-bitcoin-new to LedgerHQ/app-bitcoin - legacy: from LedgerHQ/app-bitcoin to LedgerHQ/app-bitcoin-legacy
self.tx is never None, for PSBTv2 it's an empty placeholder. Its defaults would overwrite the deserialized tx version and fallback locktime, invalidating any existing signature. Check the PSBT version instead, here and in get_unsigned_tx(). Without this fix all BIP 370 valid test vectors, now added, fail the serialization round-trip.
Member
Author
|
The one ledger legacy CI failure seems spurious (and passed in #839). |
The Speculos automation file advances through screens by matching
text fragments. The rule for the "To" screen matched any fragment
starting with "To", including parts of the destination address
shown below that title.
This is what failed in CI run 28680592019 (job 85066632783). The
address mzmauywUy3WF1TX3YxzQMA5PR4zXqJVLTo was split on the device
screen into "mzmauywUy3WF1TX", "3YxzQMA5PR4zXqJVL" and "To". The
latter confused the automation rule for "To", which pressed an
extra right button:
automation: getting actions for "To" (57, 3)
automation: getting actions for "mzmauywUy3WF1TX" (9, 19)
automation: getting actions for "3YxzQMA5PR4zXqJVL" (8, 33)
automation: getting actions for "To" (57, 47)
seproxyhal: applying automation ['button', 2, True]
seproxyhal: applying automation ['button', 2, False]
From there every press landed one screen late; the approval hit
"Reject transaction" and the app returned 0x6985, so signtx
reported a canceled error. "T" and "o" are both valid base58
characters, and bitcoind generates fresh addresses on every run,
which makes this a rare and random failure. Bech32 addresses
cannot trigger it ("o" is not in the bech32 character set).
Limit the "To" rule to the title row (y=3), where address text
never appears. The rule file format does not allow comments, so a
warning about short words in automation rules goes in the README.
Co-authored-by: Claude (Fable 5) <noreply@anthropic.com>
Member
Author
|
Actually that was another case of https://github.com/bitcoin-core/HWI/actions/runs/29081182723/job/86325723028?pr=836 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commits are taken from #819 (which is now based on this).
Two minor commits drop unused CI code.
Ledger moved repos around, so also include: