fix(bip137): align recid with the script type - #866
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #866 +/- ##
========================================
Coverage 97.48% 97.48%
========================================
Files 84 85 +1
Lines 10955 10992 +37
========================================
+ Hits 10679 10716 +37
Misses 276 276 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e35223d to
4c95cf7
Compare
odudex
left a comment
odudex
left a comment
There was a problem hiding this comment.
The header-byte fix is correct: single-sig P2SH-P2WPKH and P2WPKH wallets were
emitting legacy-message signatures with the P2PKH-compressed header byte (31-34)
instead of the BIP-137 script-type headers (35-38 / 39-42). Signing now routes
through bip137.sign, which preserves the recovery id and rebases the header on
the script type. Verified against the regenerated vectors:
- bc1qgl5... (P2WPKH): 32 -> 40 (39 + recid) OK
- 38CahkV... (P2SH-P2WPKH): 31 -> 35 (35 + recid) OK
- tb1qynp... (P2WPKH tnet): 32 -> 40 (39 + recid) OK
sign_at emits 31 + recid, so recovery_id = raw_header - 31 is correct and
the re-encoding only swaps the script-type base. Tests pass (5 passed).
Blocking: taproot message signing shouldn't stop working
This PR makes single-sig taproot (m/86') message signing fail. From a user's
perspective this is a regression / introduced bug, not a tightening: taproot
signing worked before and Sparrow verified the result as valid.
Mechanism: get_script_type_from_path("m/86'/...") returns "p2tr", which is
passed to bip137.sign -> build_header. build_header has no p2tr branch, so
it raises ValueError, which the menu catches and shows as an error screen.
Why it really was working (so this is a real loss of function, not removal of a
broken path):
- Krux signs the standard BIP-137 commitment with an ECDSA-recoverable
signature using the taproot internal key. - Lenient verifiers (Sparrow / Electrum-style) recover the pubkey and
reconstruct the address of the claimed type from it - for taproot they apply
the taproot tweak (same as script.p2tr) and the recovered+tweaked key matches
the bc1p... address. They effectively ignore the header's script-type bits. - So the signature genuinely proves control of the taproot key and verifies in
practice. It's just not a formally specified scheme (BIP-137 has no taproot;
BIP-322 is the eventual standard).
Required outcome: taproot message signing must keep working. Acceptable options:
- Keep the previous behavior - give build_header a p2tr branch that returns a
valid recoverable header so Krux keeps emitting the Sparrow-verifiable
signature (this is the no-regression path). The segwit header fix (#865) is
independent and does not require dropping taproot. - Implement proper BIP-322 signing for taproot.
What is NOT acceptable: shipping a version where taproot signing errors out.
Other required changes
-
Misleading error message in build_header.
"%s legacy sign not supported"implies the script type itself is
unsupported. p2tr is a supported wallet type; the limitation is that BIP-137
legacy message signing has no taproot scheme. If any reject path remains,
reword to make that clear and point at BIP-322. Also drop the trailing space
(currently shown verbatim asValueError('p2tr legacy sign not supported ')). -
Add the MIT license header to src/krux/bip137.py.
Every other src/krux/*.py file opens with the MIT block; this module starts
straight atP2PKH_HEADER = 31. -
Dead module-level constants. P2PKH_HEADER / P2SH_P2WPKH_HEADER / P2WPKH_HEADER
are defined but never used; build_header hardcodes the literals 31/35/39.
Use the constants or drop them so they cannot drift. -
Unreachable assertion in test_sign_message_p2tr_bip137.
Theassert ctx.input.wait_for_button.call_count == len(btn_seq)sits after
the raising call inside thewith pytest.raises(...)block, so it never runs.
Move it after the block or delete it. (This test should change anyway once
taproot signing is restored.)
Suggested (non-blocking)
- Commit says "add test vectors for bip137 module" but there are no direct unit
tests for bip137.py; coverage is only via the UI. The API-boundary guards in
build_header / sign are never exercised. Add tests/test_bip137.py.
5d5b2eb to
dec7eb1
Compare
|
Why a menu is needed? |
590ba6a to
da35069
Compare
|
This needs to be simplified. |
2d7f727 to
a8274da
Compare
qlrd
changed the title
372d0e0 to
e00edad
Compare
|
|
||
| def build_header(raw_sig, script_type): | ||
| """Build header byte from raw signature and script_type""" | ||
| recid = raw_sig[0] - P2PKH_HEADER |
There was a problem hiding this comment.
un-settled thoughts: wouldn't we just left-fshift 6 bits (correction: byte0 & 3) so that we grab the 2 least significant bits as recId?
There was a problem hiding this comment.
from what i found, it need a "normalization" ((byte0 - 27) & 3 -- the p2pkh one), an this works, confirm?
There was a problem hiding this comment.
I suspect I'm missing an important aspect of bip137, but I'll explain my understanding of at least the header byte, so that I can be corrected on my misunderstanding.
- header byte was set by some ecdsa library when the signature was created,
- we're just trying to dissect the header so that we know what type of address this is, without modifying the signature,
- the 2 least significant bits are the recId and we can have this from any sample_byte between 0-255 via sample_byte & 3 (0b00000011 and any number will result 0-3)
- if we were to subtract 27 from the header byte, which has the 2 least-significant bits high/on/one, then we'd be mucking with whatever recId was set by the ec library.
There was a problem hiding this comment.
Thanks for your concerns @jdlcdl (EDIT: i would be glad if i can be corrected on any misunderstanding of mine).
I did that from an interpretation between BitcoinJ sample on BIP137 and ecdsa_sign_recoverable.
From what i understood, on the signing side, embit gave a 65-byte recoverable signature with recId, internally -- (sig + bytes([0..3])).
After, we read that and packed the byte0 = 27 + recId + 4.
So, when raw_sig came with byte0 (27 + recId + 4) — which is why i did (header - 27) & 3 and asked for a review on that.
|
removed all bip322 as suggested before the diff gets big and added some battery of manual tests to reproduce |
qlrd
changed the title
recid with the script type
There was a problem hiding this comment.
Pull request overview
This PR fixes BIP-137 message signing header construction so the recovery ID (“recid”) aligns with the script type (notably improving interoperability with older Sparrow versions), and updates message-signing flows to treat raw hashes differently from BIP-137 messages.
Changes:
- Introduces a dedicated
krux/bip137.pymodule with header construction and message commitment logic. - Updates Sign Message UI flows to sign BIP-137 messages via the new module, while routing raw-hash signing to DER signatures.
- Adds/updates tests to cover strict vs lenient verification behavior and updated UI signature expectations; updates changelog entry.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_bip137.py | Adds new unit tests for BIP-137 commitment/header behavior and strict/lenient verification expectations. |
| tests/pages/home_pages/test_sign_message_ui.py | Updates UI test vectors to match new signature behavior and adjusts assertions. |
| src/krux/pages/home_pages/sign_message_ui.py | Switches signing paths to use krux.bip137 for non-raw-hash messages and keeps DER signing for raw hashes. |
| src/krux/bip137.py | New module implementing BIP-137 message commitment and header/recid handling. |
| CHANGELOG.md | Notes the recid-related message-signing fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Sorry for copilot messages, forgot to deactivate it |
10d3c5d to
c9d7349
Compare
|
@odudex, baked a |
|
Let's merge this, then you focus on BIP322 |
Just to document a required review from @joaozinhom (i think his fresh knowledge could be worth of). |
joaozinhom
left a comment
joaozinhom
left a comment
There was a problem hiding this comment.
Left three inline comments two things I'd change plus a question about the standard-message format. A few small things below, none blocking.
src/krux/bip137.py line 47: the compressed parameter is unreachable in production nothing in krux passes compressed=False, only the tests do. It's also what makes the expression on lines 63–68 hard to read, for a branch that can't be taken. Since this is new API, easier not to ship it than to remove it later.
src/krux/bip137.py line 28: RECID_OFFSET isn't a recid offset, it's the compressed-pubkey offset (+4). Worth renaming now while the module is new.
tests/test_bip137.py lines 153 and 186: print("Case: %d", i) uses a comma instead of %, so it prints the literal %d. Line 230 has it right.
Claude helped me summarize everything and make my text clear.
| ).digest() | ||
|
|
||
| sig = self.ctx.wallet.key.sign_at(derivation, message_hash) | ||
| script_type = self.get_script_type_from_path(derivation_str) or "p2pkh" |
There was a problem hiding this comment.
The recid fix itself is correct — I decoded the new vectors and they line up (KN/4… → byte 40 = P2WPKH_HEADER + 1, IyH8… → 35 = P2SH_P2WPKH_HEADER + 0).
What this line exposes is that the SD flow now decides the script type twice, from two independent sources. _sign_at_address_from_sd reads it from the file on line 217 and uses it to build the address on line 223, but doesn't pass it down on line 225 — so this line re-derives it from the path instead.
Before this PR that didn't matter: the header was always 31–34 and asserted nothing, so a disagreement was invisible. Now the header is authoritative, and the two sources can contradict each other:
path file says path implies address displayed header
m/84h/0h/0h/0/0 p2wpkh p2wpkh bc1qcr8te4kr609gcawutmrza0j4xv80jy8z306fyu 39 ok
m/0h/0h/0h/0/0 p2wpkh None→p2pkh bc1qaaauaz73eyjyla73u7ahrx0vrlsgf0elwqrhyv 31 ✗
The second row doesn't need a malformed file. _is_valid_derivation_path only checks BIP32 syntax, so it accepts any purpose; get_script_type_from_path then finds no match in SINGLESIG_SCRIPT_PURPOSE (which only has 44/49/84/86), returns None, and the or "p2pkh" fallback here guesses p2pkh while the address on screen is a bc1q…. The .txt we export on lines 341–347 pairs that bech32 address with a header telling the verifier to recover a p2pkh one, so a strict verifier rejects it.
Repro — drop in tests/, passes on this branch:
from .pages.home_pages.test_home import tdata
from .pages import create_ctx
def test_sd_script_type_mismatch(mocker, m5stickv, tdata):
from krux.pages.home_pages.sign_message_ui import SignMessage
from krux.wallet import Wallet
from krux.input import BUTTON_ENTER
# non-standard purpose; file declares p2wpkh
file_content = b"msg\nm/0h/0h/0h/0/0\np2wpkh"
ctx = create_ctx(mocker, [BUTTON_ENTER, BUTTON_ENTER],
Wallet(tdata.SINGLESIG_SIGNING_KEY))
sig, _, address = SignMessage(ctx)._sign_at_address_from_sd(file_content)
assert address.startswith("bc1q") # p2wpkh address
assert 31 <= sig[0] <= 34 # p2pkh headerNobody loses funds over it, and the common case (Sparrow-generated files) is correct. But the PR does introduce a state where the screen shows one address and the signature claims another with no indication. Giving _sign_at_address an optional script_type and having the SD caller pass the one it already validated on line 218 — falling back to the path otherwise — removes the whole class in about three lines.
There was a problem hiding this comment.
Great finding @joaozinhom, i will try to fix this one.
| if not self.prompt(t("Proceed?"), BOTTOM_PROMPT_LINE): | ||
| return "" | ||
| else: | ||
| message_hash = bip137.message_commitment(data) |
There was a problem hiding this comment.
This overwrite makes the label on line 252 inaccurate — the screen still says SHA256: but now renders the BIP-137 commitment, which is sha256(sha256(magic ‖ varint ‖ message)). For b"A test message.":
sha256sum of the content : 60e76ce856c4bd873cc66e424d214ced480d77c34091aad5eb190b2cd9668bd8
what the screen shows : 6d0485d73f55fe44f8040b0c5e818bebc8319ce3e61970e8f1fb2376858c47ac
That comparison is the user's only check that the device is signing what they think it is before pressing Sign, and the docs promise it explicitly — docs/getting-started/usage/navigating-the-main-menu.en.md line 330: "you will be shown a preview of the message's SHA-256 hash for confirmation before signing". Someone verifying with sha256sum now sees a mismatch with no way to tell whether it's a bug, the wrong file, or intended behaviour.
It's also inconsistent within the same screen: on the is_raw_hash branch the label is still correct, since it shows the 32 bytes the user supplied. Same label, two different kinds of value depending on what was loaded.
Minor, related: _compute_message_hash still computes hashlib.sha256(data).digest() on line 277 and this line discards it two lines later. Folding the commitment into that helper would fix the dead computation and give one place to name the value.
There was a problem hiding this comment.
Yup, this one have, IMO, a conceptual usage that is more a decision of UI/UX for pleb user flow than tech correctness: the majority of users will not understand the diff between sha256 and sha256(sha256(magic | varint | message)) defined in BIP137; i think those will add more information.
The first path is conceptually lenient; should we accept that we could just call sha256 for normal user even at cost of wrong concept and avoid more explanations (anyway, the user just wanna sign)?
The second path is conceptually strict; it would require add a little translation and diff in docs (what could be fixed in a follow-up if this path is chosen).
There was a problem hiding this comment.
Minor, related:
_compute_message_hashstill computeshashlib.sha256(data).digest()on line 277 and this line discards it two lines later. Folding the commitment into that helper would fix the dead computation and give one place to name the value.
Yup, didn't touched this one i think because could be more than a chore or refactor. Maybe a suggestion or a followup?
There was a problem hiding this comment.
yeah i think this can be a follow up...
| if is_raw_hash: | ||
| sig = key.sign(message_hash).serialize() | ||
| else: | ||
| _, sig = bip137.sign( |
There was a problem hiding this comment.
Question rather than a change request: was the format change on this path intentional? The PR description is about the recid at derived addresses, and this is a separate behavioural change that isn't mentioned.
For non-raw-hash input we used to emit DER over sha256(data); now it's a 65-byte BIP-137 compact signature. Decoding the old and new vectors for case 2 ("hello world") in tests/pages/home_pages/test_sign_message_ui.py line 71:
before: 70 bytes, byte0 = 0x30 -> DER SEQUENCE
after: 65 bytes, byte0 = 0x28 -> BIP-137 header (39 + recid = p2wpkh)
Release signing is unaffected, which I checked: the firmware hash is 64 hex chars, so _compute_message_hash sets is_raw_hash=True and line 260 still produces DER.
The case that changes is signing a plaintext or binary file loaded from SD. That used to yield a signature accepted by openssl sha256 <file -binary | openssl pkeyutl -verify -pubin -inkey pubkey.pem -sigfile sig.bin — the exact command in ./krux verify line 262 — using the hex public key we export right afterwards. A 65-byte compact signature isn't, and navigating-the-main-menu.en.md line 334 still tells users that exported key "can be used by others to verify your signature".
Worth noting too that the header carries no usable meaning here. key.derivation is the account node (src/krux/key.py lines 180–186), so a default singlesig wallet emits header 40, and a strict verifier would recover the pubkey and derive bc1ql5f64jdzjsvgehlpxvdgm9ygp0xta7xpnueh03 — the p2wpkh address of the account node, which no wallet ever displays. For multisig/miniscript key.script_type is p2wsh, which falls through build_header and silently gets a p2pkh header.
If this is wanted, the docs need to follow. If it isn't, keeping BIP-137 to the at-address flow would scope the PR tightly to the bug it names.
There was a problem hiding this comment.
Question rather than a change request: was the format change on this path intentional? The PR description is about the recid at derived addresses, and this is a separate behavioural change that isn't mentioned.
Yes, and actually this is what @odudex and @jdlcdl warned me about break firmware sign IIRC. Before i did like you described, since it's a separated behavioral change. But from what i understand, it touches a fundamental part of firmware and a firmware signature isn't the same thing as bip137 signature. Where you think should I point on docs your suggestion?
There was a problem hiding this comment.
docs/getting-started/usage/navigating-the-main-menu.en.md around line 330 — that's where I cited it. And also around line 334 where the openssl pkeyutl verification command is shown, since that command only works for DER over plain SHA-256, not a 65-byte BIP-137 compact signature.
There was a problem hiding this comment.
Yup,
DER over plain SHA-256, not a 65-byte BIP-137 compact signature.
This was the exact moral barrier to me 🤕
This commit add checks for signing message UI using recid following spec from BIP137.
Added unit tests agains BIP137 spec and apply lenient agains strict checks.
What is this PR for?
Minor bug fix.
Previously, the header byte could not reflect the script type's recid when using
sparrow <2.5.xfor sign messages on some script types (mostlyp2sh-p2wpkh). Through some manual experiments with different versions ofsparrow, a possibility that a header byte could mismatched to the script type causes verification to fail on those versions.This commit add a check of a
recidas well structure abip137.pythat is strict on context ofsparrow 2.5.xand lenient forsparrow <2.5.x(similar what used inelectrumclients), as well routesraw hashesto be not signed asbip137but instead asderone.Changes made to:
Did you build the code and tested on device?
sign [-u]flagWhat is the purpose of this pull request?
Context
The bip322 will be added to a follow up so the diffs could be simplified
Steps to reproduce (WIP)
Context: built different versions of Sparrow (at least 2 with version
<2.5.x) from source and madeP2PKH,P2SH_P2WPKHandP2WPKHsign message procedures with a flashed firmware on device with a simpleoi(hion portuguese) on Sparrow on given address.Below was noted some different behaviours for some different Sparrow's versions. Since used different mnemonics on
testnetandtestnet4networks, i think any mnemonic here will be sufficient.While reproduce expects these results below -- deselecting any mode on Sparrow (
Standard(electrum),trezor(BIP137)andBIP322-- when available):Sparrow 2.3.1
Sparrow 2.4.2
Sparrow 2.5.2
Krux file signer
Download and update to
mainbranch; and run the following command:uv run poe sign -f README.md # follow the procedures, krux will show `raw hash`.Then the command below should produce:
uv run poe verify -f README.md -s README.md.sig -p pubkey.pem Poe => python src/ksigner.py verify -f README.md -s README.md.sig -p pubkey.pem [HH:MM:ss MM/DD/YY time] Verifying signature: Signature Verified SuccessfullyAlso need to verify with
--uncompressedflag:uv run poe verify -f README.md -s README.md.u.sig -p pubkey.pem Poe => python src/ksigner.py verify -f README.md -s README.md.sig -p pubkey.pem [HH:MM:ss MM/DD/YY time] Verifying signature: Signature Verified SuccessfullyNote that the
-uflag should produce different pubvkeys.--
Footnotes
Sparrow disables trezor(BIP137) and BIP322 for P2PKH (not disabled on previous versions) ↩
Sparrow disable BIP322 for P2SH_P2WPKH (not disabled on previous versions) ↩