diff --git a/releases/Next-ChangeLog.md b/releases/Next-ChangeLog.md index ba339b6b..e788e6e9 100644 --- a/releases/Next-ChangeLog.md +++ b/releases/Next-ChangeLog.md @@ -13,6 +13,7 @@ This lists the new changes that have not yet been published in a normal release. When both UTXO fields are present the full non_witness_utxo is now preferred for amount/script lookup. Thanks, @Damir - Bugfix: Emit warning and do not calculate fee for legacy UTXOs with only witness utxo - Bugfix: Disable Virtual Disk and NFC before activating HSM +- Bugfix: P2PK signing was broken. Now supports both compressed and uncompressed P2PK spend - Bugfix: Custom address default menu position wrong - Bugfix: Delta Mode Trick PIN was never restored from backup - Bugfix: Proper error message for incorrect 7z headers diff --git a/shared/auth.py b/shared/auth.py index 5db13b1d..639aed61 100644 --- a/shared/auth.py +++ b/shared/auth.py @@ -651,7 +651,8 @@ class ApproveTransaction(UserAuthorizedAction): has_change = True total_change += tx_out.nValue if len(largest_change) < MAX_VISIBLE_CHANGE: - largest_change.append((tx_out.nValue, self.chain.render_address(tx_out.scriptPubKey))) + _, addr = self.render_output(tx_out) + largest_change.append((tx_out.nValue, addr)) if len(largest_change) == MAX_VISIBLE_CHANGE: largest_change = sorted(largest_change, key=lambda x: x[0], reverse=True) continue @@ -676,12 +677,9 @@ class ApproveTransaction(UserAuthorizedAction): continue # too small largest.pop(-1) - if outp.is_change: - ret = (here, self.chain.render_address(tx_out.scriptPubKey)) - else: - rendered, _ = self.render_output(tx_out) - ret = (here, rendered) - largest.insert(keep, ret) + + rendered, dest = self.render_output(tx_out) + largest.insert(keep, (here, dest if outp.is_change else rendered)) # foreign outputs (soon to be other people's coins) visible_out_sum = 0 @@ -1701,9 +1699,11 @@ class TXInpExplorer(TXExplorer): item += "=== UTXO ===\n\n%s %s\n\n%s\n\n" % (val, unit, spk) if addr: item += show_single_address(addr) + "\n\n" - item += "Address Format: %s\n\n" % chains.addr_fmt_str(inp.addr_fmt) qr_items.append(addr) + if inp.addr_fmt is not None: + item += "Address Format: %s\n\n" % chains.addr_fmt_str(inp.addr_fmt) + if self.user_auth_action.psbt.txn_version >= 2: has_rtl = inp.has_relative_timelock(txin) if has_rtl: diff --git a/shared/chains.py b/shared/chains.py index 8aeace43..9214ceb3 100644 --- a/shared/chains.py +++ b/shared/chains.py @@ -5,7 +5,7 @@ import ngu from uhashlib import sha256 from ubinascii import hexlify as b2a_hex -from public_constants import AF_CLASSIC, AF_P2WPKH, AF_P2TR +from public_constants import AF_BARE_PK, AF_CLASSIC, AF_P2WPKH, AF_P2TR from public_constants import AF_P2SH, AF_P2WSH, AF_P2WPKH_P2SH, AF_P2WSH_P2SH from public_constants import AFC_PUBKEY, AFC_SEGWIT, AFC_BECH32, AFC_SCRIPT from block_height import BLOCK_HEIGHT @@ -450,6 +450,7 @@ def addr_fmt_label(addr_fmt): def addr_fmt_str(addr_fmt): # Short string codes used for address format (industry standard) return {AF_CLASSIC: "p2pkh", + AF_BARE_PK: "p2pk", AF_P2SH: "p2sh", AF_P2TR: "p2tr", AF_P2WPKH: "p2wpkh", diff --git a/shared/psbt.py b/shared/psbt.py index d9eb5861..d7c8d0b6 100644 --- a/shared/psbt.py +++ b/shared/psbt.py @@ -440,7 +440,7 @@ class psbtOutputProxy(psbtProxy): if af == AF_BARE_PK: # output is public key (not a hash, much less common) - assert len(addr_or_pubkey) == 33 + assert len(addr_or_pubkey) in (33, 65) # compressed or uncompressed if addr_or_pubkey != expect_pubkey: raise FraudulentChangeOutput(out_idx, "P2PK change output is fraudulent") @@ -885,7 +885,7 @@ class psbtInputProxy(psbtProxy): elif self.addr_fmt == AF_BARE_PK: # input is single public key (less common) self.scriptSig = utxo.scriptPubKey - assert len(addr_or_pubkey) == 33 + assert len(addr_or_pubkey) in (33, 65) # compressed or uncompressed if addr_or_pubkey in subpaths: which_key = addr_or_pubkey @@ -2084,7 +2084,13 @@ class psbtObject(psbtProxy): node = sv.derive_path(skp) # check the pubkey of this BIP-32 node - if target_pk == node.pubkey(): + pu = node.pubkey() # always 33-byte compressed + if len(target_pk) == 65: + # P2PK with uncompressed pubkey: re-serialize node's pubkey in + # uncompressed form for direct comparison. + pu = ngu.secp256k1.pubkey(pu).to_bytes(True) + + if target_pk == pu: return node return None @@ -2218,14 +2224,9 @@ class psbtObject(psbtProxy): else: assert which_key in inp.subpaths, 'unk key' # get node required - skp = keypath_to_str(inp.subpaths[which_key]) - node = sv.derive_path(skp, register=False) - - # expensive test, but works... and important - pu = node.pubkey() - - assert pu == which_key, \ - "Path (%s) led to wrong pubkey for input#%d"%(skp, in_idx) + node = self.check_pubkey_at_path(sv, inp.subpaths[which_key], which_key) + assert node, "Path (%s) led to wrong pubkey for input#%d" % ( + keypath_to_str(inp.subpaths[which_key]), in_idx) # track wallet usage @@ -2588,7 +2589,12 @@ class psbtObject(psbtProxy): else: pubkey, der_sig = ssig - txi.scriptSig = ser_push_data(der_sig) + ser_push_data(pubkey) + if inp.addr_fmt == AF_BARE_PK: + # P2PK: pubkey is already in scriptPubKey, scriptSig is just + txi.scriptSig = ser_push_data(der_sig) + else: + # P2PKH: scriptSig is + txi.scriptSig = ser_push_data(der_sig) + ser_push_data(pubkey) fd.write(txi.serialize()) diff --git a/shared/serializations.py b/shared/serializations.py index 014f39cc..21ea4e52 100755 --- a/shared/serializations.py +++ b/shared/serializations.py @@ -376,8 +376,10 @@ class CTxOut(object): return AF_P2SH, self.scriptPubKey[2:2+20], False if self.is_p2pk(): - # rare, pay to full pubkey - return AF_BARE_PK, self.scriptPubKey[2:2+33], False + # rare, pay to full pubkey: OP_CHECKSIG + # push_op is 0x21 (33) for compressed, 0x41 (65) for uncompressed + pk_len = self.scriptPubKey[0] + return AF_BARE_PK, self.scriptPubKey[1:1+pk_len], False if self.is_op_return(): return OP_RETURN, self.scriptPubKey, False @@ -407,8 +409,8 @@ class CTxOut(object): def is_p2pk(self): return (len(self.scriptPubKey) == 35 or len(self.scriptPubKey) == 67) \ - and (self.scriptPubKey[0] == 0x21 or self.scriptPubKey[0] == 0x41) \ - and self.scriptPubKey[-1] == 0xac + and self.scriptPubKey[0] == len(self.scriptPubKey) - 2 \ + and self.scriptPubKey[-1] == OP_CHECKSIG def is_op_return(self): return self.scriptPubKey and (self.scriptPubKey[0] == OP_RETURN) diff --git a/testing/helpers.py b/testing/helpers.py index 9ef8babf..049ad3e2 100644 --- a/testing/helpers.py +++ b/testing/helpers.py @@ -50,6 +50,14 @@ def fake_dest_addr(style='p2pkh'): if style == 'p2pkh': return bytes([0x76, 0xa9, 0x14]) + prandom(20) + bytes([0x88, 0xac]) + if style in ('p2pk', 'p2pk-compressed'): + pubkey = bytes([random.choice((2, 3))]) + prandom(32) + return bytes([len(pubkey)]) + pubkey + bytes([0xac]) + + if style == 'p2pk-uncompressed': + pubkey = bytes([4]) + prandom(64) + return bytes([len(pubkey)]) + pubkey + bytes([0xac]) + if style == "p2tr": return bytes([81, 32]) + prandom(32) @@ -58,8 +66,6 @@ def fake_dest_addr(style='p2pkh'): hex_str = "049f7b2a5cb17576a914371c20fb2e9899338ce5e99908e64fd30b78931388ac" return bytes.fromhex(hex_str) - # missing: if style == 'p2pk' => pay to pubkey, considered obsolete - raise ValueError('not supported: ' + style) def make_change_addr(wallet, style): @@ -77,9 +83,13 @@ def make_change_addr(wallet, style): assert len(target) == 20 is_segwit = True + pubkey = dest.sec(compressed=(style != 'p2pk-uncompressed')) if style == 'p2pkh': redeem_scr = bytes([0x76, 0xa9, 0x14]) + target + bytes([0x88, 0xac]) is_segwit = False + elif style in ('p2pk', 'p2pk-compressed', 'p2pk-uncompressed'): + redeem_scr = bytes([len(pubkey)]) + pubkey + bytes([0xac]) + is_segwit = False elif style == 'p2wpkh': redeem_scr = bytes([0, 20]) + target elif style == 'p2wpkh-p2sh': @@ -92,7 +102,7 @@ def make_change_addr(wallet, style): else: raise ValueError('cant make fake change output of type: ' + style) - return redeem_scr, actual_scr, is_segwit, dest.sec(), struct.pack('4I', xfp, *deriv) + return redeem_scr, actual_scr, is_segwit, pubkey, struct.pack('4I', xfp, *deriv) def swab32(n): # endian swap: 32 bits diff --git a/testing/test_sign.py b/testing/test_sign.py index 1c63a581..5174aa32 100644 --- a/testing/test_sign.py +++ b/testing/test_sign.py @@ -3932,4 +3932,207 @@ def test_empty_input_scriptPubKey(segwit_in, dev, fake_txn, start_sign, cap_stor title, _ = cap_story() assert title == "Failure" + +@pytest.mark.bitcoind +@pytest.mark.parametrize('finalize', [True, False]) +@pytest.mark.parametrize('mode', ['compressed', 'uncompressed']) +def test_spend_p2pk(mode, finalize, bitcoind, bitcoind_d_wallet, dev, + start_sign, end_sign, cap_story, use_regtest, goto_home): + use_regtest() + goto_home() + + xfp_str = xfp2str(dev.master_fingerprint).lower() + path = "44h/1h/0h/0/0" + xpub = dev.send_recv(CCProtocolPacker.get_xpub("m/" + path), timeout=5000) + node = BIP32Node.from_wallet_key(xpub) + + if mode == 'compressed': + pubkey_hex = node.node.public_key.sec(compressed=True).hex() + else: + pubkey_hex = node.node.public_key.sec(compressed=False).hex() + + # pk() descriptor with origin info — Core writes the (xfp, path) into the + # PSBT's BIP32_DERIVATION so Coldcard recognizes it as its own key. + desc = "pk([%s/%s]%s)" % (xfp_str, path, pubkey_hex) + desc = bitcoind.rpc.getdescriptorinfo(desc)["descriptor"] + + res = bitcoind_d_wallet.importdescriptors([{ + "desc": desc, "timestamp": "now", "watchonly": True, + }]) + assert res[0]["success"], res + + # Fund the P2PK output. No Core RPC accepts a raw scriptPubKey as an + # output, so we hand-build a skeleton tx with the P2PK output and let + # fundrawtransaction fill in inputs, change, and fee from supply_wallet. + push_op = b'\x21' if mode == 'compressed' else b'\x41' + spk = push_op + bytes.fromhex(pubkey_hex) + b'\xac' + txn = CTransaction() + txn.vout = [CTxOut(5_00_000_000, spk)] # 5 BTC + + funded = bitcoind.supply_wallet.fundrawtransaction(txn.serialize().hex()) + signed = bitcoind.supply_wallet.signrawtransactionwithwallet(funded["hex"]) + bitcoind.rpc.sendrawtransaction(signed["hex"]) + bitcoind.supply_wallet.generatetoaddress(1, bitcoind.supply_wallet.getnewaddress()) + assert bitcoind_d_wallet.listunspent() + + dest = bitcoind.supply_wallet.getnewaddress() + resp = bitcoind_d_wallet.walletcreatefundedpsbt( + [], [{dest: bitcoind_d_wallet.getbalance()}], 0, + {"fee_rate": 3, "subtractFeeFromOutputs": [0]}) + psbt_bytes = base64.b64decode(resp["psbt"]) + + # Sanity: confirm Core encoded the pubkey form we asked for in the PSBT's + # BIP32_DERIVATION (key field). 33 bytes for compressed, 65 for uncompressed. + # Single-sig P2PK has exactly one entry; `all` rejects any extra entries + # in unexpected form. + expect_pk_len = 33 if mode == 'compressed' else 65 + pre_po = BasicPSBT().parse(psbt_bytes) + pre_keys = list(pre_po.inputs[0].bip32_paths.keys()) + assert pre_keys + assert all(len(k) == expect_pk_len for k in pre_keys) + + start_sign(psbt_bytes, finalize=finalize) + time.sleep(.1) + title, story = cap_story() + assert title == "OK TO SEND?" + out = end_sign(accept=True, finalize=finalize) + + if finalize: + # Coldcard returned the network tx; broadcast directly. + tx_hex = out.hex() + else: + # Coldcard returned a partially-signed PSBT. Verify the partial sig + # carries the same-form pubkey as keydata, then have bitcoind finalize. + signed_po = BasicPSBT().parse(out) + sig_keys = list(signed_po.inputs[0].part_sigs.keys()) + assert sig_keys + assert all(len(k) == expect_pk_len for k in sig_keys) + + finalized = bitcoind.rpc.finalizepsbt(base64.b64encode(out).decode()) + assert finalized["complete"], finalized + tx_hex = finalized["hex"] + + accept = bitcoind.rpc.testmempoolaccept([tx_hex]) + assert accept[0]["allowed"], accept + txid = bitcoind.rpc.sendrawtransaction(tx_hex) + assert len(txid) == 64 + + goto_home() + + +@pytest.mark.parametrize('finalize', [True, False]) +@pytest.mark.parametrize('mode', ['compressed', 'uncompressed']) +def test_fake_txn_spend_p2pk(mode, finalize, fake_txn, start_sign, end_sign, cap_story): + expect_pk_len = 33 if mode == 'compressed' else 65 + style = 'p2pk' if mode == 'compressed' else 'p2pk-uncompressed' + psbt = fake_txn(1, 2, p2pk_in=mode, outstyles=[style, 'p2pkh'], change_outputs=[0]) + + pre = BasicPSBT().parse(psbt) + change_keys = list(pre.outputs[0].bip32_paths.keys()) + assert change_keys and all(len(k) == expect_pk_len for k in change_keys) + + start_sign(psbt, finalize=finalize) + time.sleep(.1) + title, story = cap_story() + assert title == "OK TO SEND?" + assert "Change back:" in story + + out = end_sign(accept=True, finalize=finalize) + if not finalize: + signed = BasicPSBT().parse(out) + sig_keys = list(signed.inputs[0].part_sigs.keys()) + assert sig_keys and all(len(k) == expect_pk_len for k in sig_keys) + + +@pytest.mark.parametrize('mode', ['compressed', 'uncompressed']) +@pytest.mark.parametrize('change', [True, False]) +def test_p2pk_change_output_renders(mode, change, fake_txn, start_sign, cap_story, dev, + goto_home, need_keypress, pick_menu_item, press_cancel): + master_xpub = dev.master_xpub or simulator_fixed_tprv + mk = BIP32Node.from_wallet_key(master_xpub) + xfp = mk.fingerprint() + + input_leaf = mk.subkey_for_path("0/0") + input_pk = input_leaf.node.public_key.sec(compressed=(mode == 'compressed')) + input_spk = bytes([len(input_pk)]) + input_pk + b'\xac' + + leaf = mk.subkey_for_path("0/77") + if mode == 'compressed': + pk = leaf.node.public_key.sec(compressed=True) + push_op = b'\x21' + else: + pk = leaf.node.public_key.sec(compressed=False) + push_op = b'\x41' + + assert len(pk) == (33 if mode == 'compressed' else 65) + p2pk_spk = push_op + pk + b'\xac' # OP_CHECKSIG + + def hack(psbt): + t = CTransaction() + t.deserialize(BytesIO(psbt.txn)) + t.vout[0].scriptPubKey = p2pk_spk + psbt.txn = t.serialize_with_witness() + if change: + psbt.outputs[0].bip32_paths = {pk: xfp + struct.pack(' OP_CHECKSIG + scr = bytes([len(sec)]) + sec + b'\xac' + elif segwit_in: # p2wpkh scr = bytes([0x00, 0x14]) + subkey.hash160() if wrapped: @@ -259,6 +268,10 @@ def render_address(script, testnet=True): b58_script = bytes([196]) b58_privkey = bytes([239]) + if ll in (35, 67) and script[0] == (ll - 2) and script[-1] == 0xac: + # does not have address format - just show raw scriptPubKey + return b2a_hex(script).decode() + # P2PKH if ll == 25 and script[0:3] == b'\x76\xA9\x14' and script[23:26] == b'\x88\xAC': return encode_base58_checksum(b58_addr + script[3:3+20])