From c9a70f933872158b43a005d149fc41684dadd830 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Wed, 24 Jun 2026 15:21:09 -0700 Subject: [PATCH] script: qa: Improve Key::Fingerprint type safety --- src/key.cpp | 13 ++++++------- src/key.h | 10 ++++++---- src/musig.cpp | 2 +- src/pubkey.cpp | 9 ++++----- src/pubkey.h | 17 +++++++++++++++-- src/rpc/rawtransaction.cpp | 10 +++++----- src/script/descriptor.cpp | 15 ++++++--------- src/script/keyorigin.h | 19 +++++++------------ src/script/sign.cpp | 5 ++--- src/test/descriptor_tests.cpp | 4 ++-- src/wallet/scriptpubkeyman.cpp | 4 ++-- 11 files changed, 56 insertions(+), 52 deletions(-) diff --git a/src/key.cpp b/src/key.cpp index cc03df1cd94..4cb2663474e 100644 --- a/src/key.cpp +++ b/src/key.cpp @@ -359,8 +359,7 @@ CKey GenerateRandomKey(bool compressed) noexcept bool CExtKey::Derive(CExtKey &out, unsigned int _nChild) const { if (nDepth == std::numeric_limits::max()) return false; out.nDepth = nDepth + 1; - CKeyID id = key.GetPubKey().GetID(); - memcpy(out.vchFingerprint, &id, 4); + out.fingerprint = id_key_fingerprint(); out.nChild = _nChild; return key.Derive(out.key, out.chaincode, _nChild, chaincode); } @@ -374,13 +373,13 @@ void CExtKey::SetSeed(std::span seed) memcpy(chaincode.begin(), vout.data() + 32, 32); nDepth = 0; nChild = 0; - memset(vchFingerprint, 0, sizeof(vchFingerprint)); + fingerprint.fill(0); } CExtPubKey CExtKey::Neuter() const { CExtPubKey ret; ret.nDepth = nDepth; - memcpy(ret.vchFingerprint, vchFingerprint, 4); + ret.fingerprint = fingerprint; ret.nChild = nChild; ret.pubkey = key.GetPubKey(); ret.chaincode = chaincode; @@ -389,7 +388,7 @@ CExtPubKey CExtKey::Neuter() const { void CExtKey::Encode(unsigned char code[BIP32_EXTKEY_SIZE]) const { code[0] = nDepth; - memcpy(code+1, vchFingerprint, 4); + std::ranges::copy(fingerprint, code+1); WriteBE32(code+5, nChild); memcpy(code+9, chaincode.begin(), 32); code[41] = 0; @@ -399,11 +398,11 @@ void CExtKey::Encode(unsigned char code[BIP32_EXTKEY_SIZE]) const { void CExtKey::Decode(const unsigned char code[BIP32_EXTKEY_SIZE]) { nDepth = code[0]; - memcpy(vchFingerprint, code+1, 4); + std::copy_n(code + 1, fingerprint.size(), fingerprint.begin()); nChild = ReadBE32(code+5); memcpy(chaincode.begin(), code+9, 32); key.Set(code+42, code+BIP32_EXTKEY_SIZE, true); - if ((nDepth == 0 && (nChild != 0 || ReadLE32(vchFingerprint) != 0)) || code[41] != 0) key = CKey(); + if ((nDepth == 0 && (nChild != 0 || ReadLE32(fingerprint.data()) != 0)) || code[41] != 0) key = CKey(); } KeyPair::KeyPair(const CKey& key, const uint256* merkle_root) diff --git a/src/key.h b/src/key.h index cd77dcd0ee2..58b8054fa06 100644 --- a/src/key.h +++ b/src/key.h @@ -228,7 +228,7 @@ CKey GenerateRandomKey(bool compressed = true) noexcept; struct CExtKey { unsigned char nDepth; - unsigned char vchFingerprint[4]; + KeyFingerprint fingerprint; unsigned int nChild; ChainCode chaincode; CKey key; @@ -236,16 +236,18 @@ struct CExtKey { friend bool operator==(const CExtKey& a, const CExtKey& b) { return a.nDepth == b.nDepth && - memcmp(a.vchFingerprint, b.vchFingerprint, sizeof(vchFingerprint)) == 0 && + a.fingerprint == b.fingerprint && a.nChild == b.nChild && a.chaincode == b.chaincode && a.key == b.key; } CExtKey() = default; - CExtKey(const CExtPubKey& xpub, const CKey& key_in) : nDepth(xpub.nDepth), nChild(xpub.nChild), chaincode(xpub.chaincode), key(key_in) + CExtKey(const CExtPubKey& xpub, const CKey& key_in) : nDepth(xpub.nDepth), fingerprint(xpub.fingerprint), nChild(xpub.nChild), chaincode(xpub.chaincode), key(key_in) {} + + KeyFingerprint id_key_fingerprint() const { - std::copy(xpub.vchFingerprint, xpub.vchFingerprint + sizeof(xpub.vchFingerprint), vchFingerprint); + return key.GetPubKey().GetID().fingerprint(); } void Encode(unsigned char code[BIP32_EXTKEY_SIZE]) const; diff --git a/src/musig.cpp b/src/musig.cpp index d187ad00133..38a49fa2f37 100644 --- a/src/musig.cpp +++ b/src/musig.cpp @@ -75,7 +75,7 @@ CExtPubKey CreateMuSig2SyntheticXpub(const CPubKey& pubkey) { CExtPubKey extpub; extpub.nDepth = 0; - std::memset(extpub.vchFingerprint, 0, 4); + extpub.fingerprint.fill(0); extpub.nChild = 0; extpub.chaincode = MUSIG_CHAINCODE; extpub.pubkey = pubkey; diff --git a/src/pubkey.cpp b/src/pubkey.cpp index 264f861bc7a..938834960a6 100644 --- a/src/pubkey.cpp +++ b/src/pubkey.cpp @@ -384,7 +384,7 @@ CPubKey EllSwiftPubKey::Decode() const void CExtPubKey::Encode(unsigned char code[BIP32_EXTKEY_SIZE]) const { code[0] = nDepth; - memcpy(code+1, vchFingerprint, 4); + std::ranges::copy(fingerprint, code+1); WriteBE32(code+5, nChild); memcpy(code+9, chaincode.begin(), 32); assert(pubkey.size() == CPubKey::COMPRESSED_SIZE); @@ -393,11 +393,11 @@ void CExtPubKey::Encode(unsigned char code[BIP32_EXTKEY_SIZE]) const { void CExtPubKey::Decode(const unsigned char code[BIP32_EXTKEY_SIZE]) { nDepth = code[0]; - memcpy(vchFingerprint, code+1, 4); + std::copy_n(code + 1, fingerprint.size(), fingerprint.begin()); nChild = ReadBE32(code+5); memcpy(chaincode.begin(), code+9, 32); pubkey.Set(code+41, code+BIP32_EXTKEY_SIZE); - if ((nDepth == 0 && (nChild != 0 || ReadLE32(vchFingerprint) != 0)) || !pubkey.IsFullyValid()) pubkey = CPubKey(); + if ((nDepth == 0 && (nChild != 0 || ReadLE32(fingerprint.data()) != 0)) || !pubkey.IsFullyValid()) pubkey = CPubKey(); } void CExtPubKey::EncodeWithVersion(unsigned char code[BIP32_EXTKEY_WITH_VERSION_SIZE]) const @@ -415,8 +415,7 @@ void CExtPubKey::DecodeWithVersion(const unsigned char code[BIP32_EXTKEY_WITH_VE bool CExtPubKey::Derive(CExtPubKey &out, unsigned int _nChild, uint256* bip32_tweak_out) const { if (nDepth == std::numeric_limits::max()) return false; out.nDepth = nDepth + 1; - CKeyID id = pubkey.GetID(); - memcpy(out.vchFingerprint, &id, 4); + out.fingerprint = id_key_fingerprint(); out.nChild = _nChild; return pubkey.Derive(out.pubkey, out.chaincode, _nChild, chaincode, bip32_tweak_out); } diff --git a/src/pubkey.h b/src/pubkey.h index 0391609ebcc..28dc4a80b56 100644 --- a/src/pubkey.h +++ b/src/pubkey.h @@ -19,12 +19,20 @@ const unsigned int BIP32_EXTKEY_SIZE = 74; const unsigned int BIP32_EXTKEY_WITH_VERSION_SIZE = 78; +using KeyFingerprint = std::array; + /** A reference to a CKey: the Hash160 of its serialized public key */ class CKeyID : public uint160 { public: CKeyID() : uint160() {} explicit CKeyID(const uint160& in) : uint160(in) {} + KeyFingerprint fingerprint() const + { + KeyFingerprint ret; + std::copy_n(begin(), ret.size(), ret.begin()); + return ret; + } }; /** An encapsulated public key. */ @@ -334,7 +342,7 @@ public: struct CExtPubKey { unsigned char version[4]; unsigned char nDepth; - unsigned char vchFingerprint[4]; + KeyFingerprint fingerprint; unsigned int nChild; ChainCode chaincode; CPubKey pubkey; @@ -342,7 +350,7 @@ struct CExtPubKey { friend bool operator==(const CExtPubKey &a, const CExtPubKey &b) { return a.nDepth == b.nDepth && - memcmp(a.vchFingerprint, b.vchFingerprint, sizeof(vchFingerprint)) == 0 && + a.fingerprint == b.fingerprint && a.nChild == b.nChild && a.chaincode == b.chaincode && a.pubkey == b.pubkey; @@ -358,6 +366,11 @@ struct CExtPubKey { return a.chaincode < b.chaincode; } + KeyFingerprint id_key_fingerprint() const + { + return pubkey.GetID().fingerprint(); + } + void Encode(unsigned char code[BIP32_EXTKEY_SIZE]) const; void Decode(const unsigned char code[BIP32_EXTKEY_SIZE]); void EncodeWithVersion(unsigned char code[BIP32_EXTKEY_WITH_VERSION_SIZE]) const; diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 31a877b8f04..78401d2e4b5 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1116,7 +1116,7 @@ static RPCMethod decodepsbt() UniValue keypath(UniValue::VOBJ); keypath.pushKV("xpub", EncodeBase58Check(ser_xpub)); - keypath.pushKV("master_fingerprint", HexStr(std::span(xpub_pair.first.fingerprint, xpub_pair.first.fingerprint + 4))); + keypath.pushKV("master_fingerprint", HexStr(xpub_pair.first.fingerprint)); keypath.pushKV("path", WriteHDKeypath(xpub_pair.first.path)); global_xpubs.push_back(std::move(keypath)); } @@ -1237,7 +1237,7 @@ static RPCMethod decodepsbt() UniValue keypath(UniValue::VOBJ); keypath.pushKV("pubkey", HexStr(entry.first)); - keypath.pushKV("master_fingerprint", strprintf("%08x", ReadBE32(entry.second.fingerprint))); + keypath.pushKV("master_fingerprint", strprintf("%08x", ReadBE32(entry.second.fingerprint.data()))); keypath.pushKV("path", WriteHDKeypath(entry.second.path)); keypaths.push_back(std::move(keypath)); } @@ -1354,7 +1354,7 @@ static RPCMethod decodepsbt() const auto& [leaf_hashes, origin] = leaf_origin; UniValue path_obj(UniValue::VOBJ); path_obj.pushKV("pubkey", HexStr(xonly)); - path_obj.pushKV("master_fingerprint", strprintf("%08x", ReadBE32(origin.fingerprint))); + path_obj.pushKV("master_fingerprint", strprintf("%08x", ReadBE32(origin.fingerprint.data()))); path_obj.pushKV("path", WriteHDKeypath(origin.path)); UniValue leaf_hashes_arr(UniValue::VARR); for (const auto& leaf_hash : leaf_hashes) { @@ -1473,7 +1473,7 @@ static RPCMethod decodepsbt() for (auto entry : output.hd_keypaths) { UniValue keypath(UniValue::VOBJ); keypath.pushKV("pubkey", HexStr(entry.first)); - keypath.pushKV("master_fingerprint", strprintf("%08x", ReadBE32(entry.second.fingerprint))); + keypath.pushKV("master_fingerprint", strprintf("%08x", ReadBE32(entry.second.fingerprint.data()))); keypath.pushKV("path", WriteHDKeypath(entry.second.path)); keypaths.push_back(std::move(keypath)); } @@ -1513,7 +1513,7 @@ static RPCMethod decodepsbt() const auto& [leaf_hashes, origin] = leaf_origin; UniValue path_obj(UniValue::VOBJ); path_obj.pushKV("pubkey", HexStr(xonly)); - path_obj.pushKV("master_fingerprint", strprintf("%08x", ReadBE32(origin.fingerprint))); + path_obj.pushKV("master_fingerprint", strprintf("%08x", ReadBE32(origin.fingerprint.data()))); path_obj.pushKV("path", WriteHDKeypath(origin.path)); UniValue leaf_hashes_arr(UniValue::VARR); for (const auto& leaf_hash : leaf_hashes) { diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp index 3b73a40ccef..82165a1ab8d 100644 --- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -271,7 +271,7 @@ public: Assert(out.pubkeys.contains(pub->GetID())); auto& [pubkey, suborigin] = out.origins[pub->GetID()]; Assert(pubkey == *pub); // m_provider must have a valid origin by this point. - std::copy(std::begin(m_origin.fingerprint), std::end(m_origin.fingerprint), suborigin.fingerprint); + suborigin.fingerprint = m_origin.fingerprint; suborigin.path.insert(suborigin.path.begin(), m_origin.path.begin(), m_origin.path.end()); return pub; } @@ -339,7 +339,7 @@ public: { KeyOriginInfo info; CKeyID keyid = m_pubkey.GetID(); - std::copy(keyid.begin(), keyid.begin() + sizeof(info.fingerprint), info.fingerprint); + info.fingerprint = keyid.fingerprint(); out.origins.emplace(keyid, std::make_pair(m_pubkey, info)); out.pubkeys.emplace(keyid, m_pubkey); return m_pubkey; @@ -404,7 +404,7 @@ class BIP32PubkeyProvider final : public PubkeyProvider CKey key; if (!arg.GetKey(m_root_extkey.pubkey.GetID(), key)) return false; ret.nDepth = m_root_extkey.nDepth; - std::copy(m_root_extkey.vchFingerprint, m_root_extkey.vchFingerprint + sizeof(ret.vchFingerprint), ret.vchFingerprint); + ret.fingerprint = m_root_extkey.fingerprint; ret.nChild = m_root_extkey.nChild; ret.chaincode = m_root_extkey.chaincode; ret.key = key; @@ -441,8 +441,7 @@ public: std::optional GetPubKey(int pos, const SigningProvider& arg, FlatSigningProvider& out, const DescriptorCache* read_cache = nullptr, DescriptorCache* write_cache = nullptr) const override { KeyOriginInfo info; - CKeyID keyid = m_root_extkey.pubkey.GetID(); - std::copy(keyid.begin(), keyid.begin() + sizeof(info.fingerprint), info.fingerprint); + info.fingerprint = m_root_extkey.id_key_fingerprint(); info.path = m_path; if (m_derive == DeriveType::UNHARDENED_RANGED) info.path.push_back((uint32_t)pos); if (m_derive == DeriveType::HARDENED_RANGED) info.path.push_back(((uint32_t)pos) | 0x80000000L); @@ -559,9 +558,7 @@ public: for (; k < (int)m_path.size(); ++k) { end_path.push_back(m_path.at(k)); } - // Get the fingerprint - CKeyID id = m_root_extkey.pubkey.GetID(); - std::copy(id.begin(), id.begin() + 4, origin.fingerprint); + origin.fingerprint = m_root_extkey.id_key_fingerprint(); CExtPubKey xpub; CExtKey lh_xprv; @@ -2164,7 +2161,7 @@ std::vector> ParsePubkey(uint32_t& key_exp_index KeyOriginInfo info; static_assert(sizeof(info.fingerprint) == 4, "Fingerprint must be 4 bytes"); assert(fpr_bytes.size() == 4); - std::copy(fpr_bytes.begin(), fpr_bytes.end(), info.fingerprint); + std::copy_n(fpr_bytes.begin(), info.fingerprint.size(), info.fingerprint.begin()); std::vector path; if (!ParseKeyPath(slash_split, path, apostrophe, error, /*allow_multipath=*/false)) return {}; info.path = path.at(0); diff --git a/src/script/keyorigin.h b/src/script/keyorigin.h index e54133cd12e..190b4f6ba62 100644 --- a/src/script/keyorigin.h +++ b/src/script/keyorigin.h @@ -5,28 +5,23 @@ #ifndef BITCOIN_SCRIPT_KEYORIGIN_H #define BITCOIN_SCRIPT_KEYORIGIN_H +#include #include #include struct KeyOriginInfo { - unsigned char fingerprint[4]; //!< First 32 bits of the Hash160 of the public key at the root of the path + KeyFingerprint fingerprint; //!< First 32 bits of the Hash160 of the public key at the root of the path std::vector path; - friend bool operator==(const KeyOriginInfo& a, const KeyOriginInfo& b) - { - return std::equal(std::begin(a.fingerprint), std::end(a.fingerprint), std::begin(b.fingerprint)) && a.path == b.path; - } + friend bool operator==(const KeyOriginInfo& a, const KeyOriginInfo& b) = default; friend bool operator<(const KeyOriginInfo& a, const KeyOriginInfo& b) { // Compare the fingerprints lexicographically - int fpr_cmp = memcmp(a.fingerprint, b.fingerprint, 4); - if (fpr_cmp < 0) { - return true; - } else if (fpr_cmp > 0) { - return false; - } + if (a.fingerprint < b.fingerprint) return true; + else if (a.fingerprint > b.fingerprint) return false; + // Compare the sizes of the paths, shorter is "less than" if (a.path.size() < b.path.size()) { return true; @@ -41,7 +36,7 @@ struct KeyOriginInfo void clear() { - memset(fingerprint, 0, 4); + fingerprint.fill(0); path.clear(); } }; diff --git a/src/script/sign.cpp b/src/script/sign.cpp index f38ac2bee8e..c4d59de46d5 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -28,6 +28,7 @@ #include #include +#include #include #include #include @@ -312,9 +313,7 @@ static bool SignMuSig2(const BaseSignatureCreator& creator, SignatureData& sigda CPubKey plain_pub = agg_pub; if (XOnlyPubKey(agg_pub) != script_pubkey) { if (agg_info.path.empty()) continue; - // Compute and compare fingerprint - CKeyID keyid = agg_pub.GetID(); - if (!std::equal(agg_info.fingerprint, agg_info.fingerprint + sizeof(agg_info.fingerprint), keyid.data())) { + if (agg_info.fingerprint != agg_pub.GetID().fingerprint()) { continue; } // Get the BIP32 derivation tweaks diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp index 5e4039ad1e1..21b3d20abd6 100644 --- a/src/test/descriptor_tests.cpp +++ b/src/test/descriptor_tests.cpp @@ -159,7 +159,7 @@ std::set> GetKeyOriginData(const FlatSigningPr bytes[0] = 0x02; CPubKey norm_pubkey{bytes}; KeyOriginInfo norm_origin = data.second; - std::fill(std::begin(norm_origin.fingerprint), std::end(norm_origin.fingerprint), 0); // fingerprints don't necessarily match. + norm_origin.fingerprint.fill(0); // fingerprints don't necessarily match. ret.emplace(norm_pubkey, norm_origin); } else { ret.insert(data); @@ -560,7 +560,7 @@ void CheckInferDescriptor(const std::string& script_hex, const std::string& expe std::vector> origin_split = Split(origin_sp, "/"); std::string fpr_str(origin_split[0].begin(), origin_split[0].end()); auto fpr_bytes = ParseHex(fpr_str); - std::copy(fpr_bytes.begin(), fpr_bytes.end(), info.fingerprint); + std::copy_n(fpr_bytes.begin(), info.fingerprint.size(), info.fingerprint.begin()); for (size_t i = 1; i < origin_split.size(); ++i) { std::span elem = origin_split[i]; bool hardened = false; diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 0759e2d5220..009e22101ce 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -421,10 +421,10 @@ bool LegacyDataSPKM::GetKeyOrigin(const CKeyID& keyID, KeyOriginInfo& info) cons meta = it->second; } if (meta.has_key_origin) { - std::copy(meta.key_origin.fingerprint, meta.key_origin.fingerprint + 4, info.fingerprint); + info.fingerprint = meta.key_origin.fingerprint; info.path = meta.key_origin.path; } else { // Single pubkeys get the master fingerprint of themselves - std::copy(keyID.begin(), keyID.begin() + 4, info.fingerprint); + info.fingerprint = keyID.fingerprint(); } return true; }