script: qa: Improve `Key::Fingerprint` type safety #35606

davidgumberg commented at 11:59 PM on June 25, 2026: contributor

Extracted from pseudoramdom's work in #35436:

Instead of using c style arrays for key fingerprints, use std::array's whose length can always reasoned about at compile time and for most operations the compiler enforces the size being correct.

using KeyFingerprint = std::array<unsigned char, 4>;

-    unsigned char vchFingerprint[4];
+    KeyFingerprint fingerprint;

This allows the replacement of a lot of raw memcpy + trust-me-bro lengths, with the assignment operator:

-    memcpy(ret.vchFingerprint, vchFingerprint, 4);
+    ret.fingerprint = fingerprint;

This commit also adds two helper functions for

Retrieving the fingerprint of a key identifier (CKeyID)
Retrieving the fingerprint of the key identifier of an XPUB.

DrahtBot added the label Consensus on Jun 25, 2026

davidgumberg renamed this:
~~script: qa: Improve Key::Fingerprint type safety~~
script: qa: Improve `Key::Fingerprint` type safety
on Jun 25, 2026

DrahtBot commented at 11:59 PM on June 25, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35606.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	sedited, pseudoramdom, polespinasa, w0xlt

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

w0xlt commented at 12:09 AM on June 26, 2026: contributor

Concept ACK

script: qa: Improve Key::Fingerprint type safety c9a70f9338

davidgumberg force-pushed on Jun 26, 2026

DrahtBot added the label CI failed on Jun 26, 2026

DrahtBot commented at 12:34 AM on June 26, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/28208162547/job/83563390457</sub> <sub>LLM reason (✨ experimental): CI failed because IWYU reported a header/include change needed in src/script/sign.cpp and was configured to fail when it made edits.</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label CI failed on Jun 26, 2026

andrewtoth commented at 2:18 PM on June 26, 2026: contributor

Instead of using vectors for key fingerprints, use std::array's whose length can be reasoned about at compile time.

-    unsigned char vchFingerprint[4];
+    KeyFingerprint fingerprint;

Just pointing out that before was not a std::vector, but a C-style array. It already has a fixed length that is enforced at compile time via sizeof.

pseudoramdom commented at 4:48 PM on June 26, 2026: contributor

Concept ACK. Thanks for taking care of it.

davidgumberg commented at 12:22 AM on June 27, 2026: contributor

Just pointing out that before was not a std::vector, but a C-style array.

Oops, thanks for catching, fixed.

It already has a fixed length that is enforced at compile time via sizeof.

Yeah, the description was unclearly written

In some cases the length of a c-array is known at compile time, but since it decays to a pointer when passed as an argument, memory copying with a length argument has to be used.

The memcpy() caller could use sizeof() to get a length and pass this as an argument, but as can be seen in the existing code, that's often too inconvenient for people to do in practice, and unlike the assignment operator for a std::array size-correctness is not enforced by the compiler when using memcpy().

sedited approved

sedited commented at 8:31 AM on July 4, 2026: contributor

ACK c9a70f933872158b43a005d149fc41684dadd830

DrahtBot requested review from pseudoramdom on Jul 4, 2026

in src/pubkey.h:22 in c9a70f9338

  18 | @@ -19,12 +19,20 @@
  19 |  const unsigned int BIP32_EXTKEY_SIZE = 74;
  20 |  const unsigned int BIP32_EXTKEY_WITH_VERSION_SIZE = 78;
  21 |  
  22 | +using KeyFingerprint = std::array<unsigned char, 4>;

pseudoramdom commented at 5:16 PM on July 15, 2026:

Should we include <array>

in src/script/sign.cpp:31 in c9a70f9338

  27 | @@ -28,6 +28,7 @@
  28 |  #include <util/vector.h>
  29 |  
  30 |  #include <algorithm>
  31 | +#include <array>

pseudoramdom commented at 5:18 PM on July 15, 2026:

Looks like we're not using <array> here? (Also see warnings for other includes from <functional> to <string>)

in src/rpc/rawtransaction.cpp:1240 in c9a70f9338

1236 | @@ -1237,7 +1237,7 @@ static RPCMethod decodepsbt()
1237 |                  UniValue keypath(UniValue::VOBJ);
1238 |                  keypath.pushKV("pubkey", HexStr(entry.first));
1239 |  
1240 | -                keypath.pushKV("master_fingerprint", strprintf("%08x", ReadBE32(entry.second.fingerprint)));
1241 | +                keypath.pushKV("master_fingerprint", strprintf("%08x", ReadBE32(entry.second.fingerprint.data())));

pseudoramdom commented at 5:31 PM on July 15, 2026:

nit (take it or leave it): since these lines are being touched anyway, the four %08x + ReadBE32 sites in this file could become HexStr matching other fingerprint formatter like in wallet/rpc/addresses.cpp

                keypath.pushKV("master_fingerprint", HexStr(entry.second.fingerprint));

pseudoramdom commented at 5:47 PM on July 15, 2026: contributor

Code review ACK w/ some minor nits c9a70f933872158b43a005d149fc41684dadd830

sedited requested review from polespinasa on Jul 23, 2026

in src/pubkey.cpp:396 in c9a70f9338

 392 | @@ -393,11 +393,11 @@ void CExtPubKey::Encode(unsigned char code[BIP32_EXTKEY_SIZE]) const {
 393 |  
 394 |  void CExtPubKey::Decode(const unsigned char code[BIP32_EXTKEY_SIZE]) {
 395 |      nDepth = code[0];
 396 | -    memcpy(vchFingerprint, code+1, 4);
 397 | +    std::copy_n(code + 1, fingerprint.size(), fingerprint.begin());

polespinasa commented at 11:10 AM on July 27, 2026:

nit:

Encode uses std::ranges::copy and Decode uses std::copy_n, as we are copying the full fingerprint can't Decode just use std::ranges::copy(fingerprint, code+1) ?

in src/script/keyorigin.h:19 in c9a70f9338

  19 | -    {
  20 | -        return std::equal(std::begin(a.fingerprint), std::end(a.fingerprint), std::begin(b.fingerprint)) && a.path == b.path;
  21 | -    }
  22 | +    friend bool operator==(const KeyOriginInfo& a, const KeyOriginInfo& b) = default;
  23 |  
  24 |      friend bool operator<(const KeyOriginInfo& a, const KeyOriginInfo& b)

polespinasa commented at 11:29 AM on July 27, 2026:

Idk if worth adding it, but taking advantage that we are touching this, maybe is worth to increase the scope to <, <=, > and >=:

diff --git a/src/script/keyorigin.h b/src/script/keyorigin.h
index 190b4f6ba6..401614d4ab 100644
--- a/src/script/keyorigin.h
+++ b/src/script/keyorigin.h
@@ -16,20 +16,16 @@ struct KeyOriginInfo
 
     friend bool operator==(const KeyOriginInfo& a, const KeyOriginInfo& b) = default;
 
-    friend bool operator<(const KeyOriginInfo& a, const KeyOriginInfo& b)
+    friend auto operator<=>(const KeyOriginInfo& a, const KeyOriginInfo& b)
     {
         // Compare the fingerprints lexicographically
-        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;
-        } else if (a.path.size() > b.path.size()) {
-            return false;
-        }
-        // Paths same length, compare them lexicographically
-        return a.path < b.path;
+        if (auto cmp = a.fingerprint <=> b.fingerprint; cmp != 0) return cmp;
+
+        // Compare the sizes of the paths
+        if (auto cmp = a.path.size() <=> b.path.size(); cmp != 0) return cmp;
+
+        // Path same length, compare them lexicographically
+        return a.path <=> b.path;
     }
 
     SERIALIZE_METHODS(KeyOriginInfo, obj) { READWRITE(obj.fingerprint, obj.path); }

polespinasa commented at 11:29 AM on July 27, 2026: member

ACK c9a70f933872158b43a005d149fc41684dadd830

left some small comments that can be ignored

w0xlt commented at 1:50 PM on July 27, 2026: contributor

ACK c9a70f933872158b43a005d149fc41684dadd830 Happy to re-ACK if the above suggestions are applied.

sedited merged this on Jul 29, 2026

sedited closed this on Jul 29, 2026

hebasto commented at 11:51 AM on July 30, 2026: member

This PR causes a new -Wmaybe-uninitialized warning when cross-compiling for Windows using GCC 16.1 (either on Fedora or on macOS):

/home/hebasto/dev/bitcoin/src/script/sign.cpp: In function 'bool SignMuSig2(const BaseSignatureCreator&, SignatureData&, const SigningProvider&, std::vector<unsigned char>&, const XOnlyPubKey&, const uint256*, const uint256*, SigVersion)':
/home/hebasto/dev/bitcoin/src/script/sign.cpp:316:13: warning: '*(unsigned int*)((char*)&agg_info + offsetof(KeyOriginInfo, KeyOriginInfo::fingerprint.std::array<unsigned char, 4>::_M_elems[0]))' may be used uninitialized [-Wmaybe-uninitialized]
  316 |             if (agg_info.fingerprint != agg_pub.GetID().fingerprint()) {
      |             ^~
/home/hebasto/dev/bitcoin/src/script/sign.cpp:288:19: note: '*(unsigned int*)((char*)&agg_info + offsetof(KeyOriginInfo, KeyOriginInfo::fingerprint.std::array<unsigned char, 4>::_M_elems[0]))' was declared here
  288 |     KeyOriginInfo agg_info;
      |                   ^~~~~~~~

hebasto commented at 12:17 PM on July 30, 2026: member

This PR causes a new -Wmaybe-uninitialized warning when cross-compiling for Windows using GCC 16.1 (either on Fedora or on macOS):

/home/hebasto/dev/bitcoin/src/script/sign.cpp: In function 'bool SignMuSig2(const BaseSignatureCreator&, SignatureData&, const SigningProvider&, std::vector<unsigned char>&, const XOnlyPubKey&, const uint256*, const uint256*, SigVersion)':
/home/hebasto/dev/bitcoin/src/script/sign.cpp:316:13: warning: '*(unsigned int*)((char*)&agg_info + offsetof(KeyOriginInfo, KeyOriginInfo::fingerprint.std::array<unsigned char, 4>::_M_elems[0]))' may be used uninitialized [-Wmaybe-uninitialized]
  316 |             if (agg_info.fingerprint != agg_pub.GetID().fingerprint()) {
      |             ^~
/home/hebasto/dev/bitcoin/src/script/sign.cpp:288:19: note: '*(unsigned int*)((char*)&agg_info + offsetof(KeyOriginInfo, KeyOriginInfo::fingerprint.std::array<unsigned char, 4>::_M_elems[0]))' was declared here
  288 |     KeyOriginInfo agg_info;
      |                   ^~~~~~~~

Maybe:

--- a/src/key.h
+++ b/src/key.h
@@ -228,7 +228,7 @@ CKey GenerateRandomKey(bool compressed = true) noexcept;
 
 struct CExtKey {
     unsigned char nDepth;
-    KeyFingerprint fingerprint;
+    KeyFingerprint fingerprint{};
     unsigned int nChild;
     ChainCode chaincode;
     CKey key;
--- a/src/pubkey.h
+++ b/src/pubkey.h
@@ -342,7 +342,7 @@ public:
 struct CExtPubKey {
     unsigned char version[4];
     unsigned char nDepth;
-    KeyFingerprint fingerprint;
+    KeyFingerprint fingerprint{};
     unsigned int nChild;
     ChainCode chaincode;
     CPubKey pubkey;
--- a/src/script/keyorigin.h
+++ b/src/script/keyorigin.h
@@ -11,7 +11,7 @@
 
 struct KeyOriginInfo
 {
-    KeyFingerprint fingerprint; //!< 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<uint32_t> path;
 
     friend bool operator==(const KeyOriginInfo& a, const KeyOriginInfo& b) = default;

?

maflcko commented at 1:11 PM on July 30, 2026: member

Maybe:

The general issue with zero-init is that possible logic bugs can no longer be found with valgrind. Maybe this is fine, but then we could enable #18892?

The GCC warning here is a false-positive and and alternative workaround could be:

diff --git a/src/script/sign.cpp b/src/script/sign.cpp
index c4d59de46d..8ba2ac7787 100644
--- a/src/script/sign.cpp
+++ b/src/script/sign.cpp
@@ -289,6 +289,8 @@ static bool SignMuSig2(const BaseSignatureCreator& creator, SignatureData& sigda
     auto misc_pk_it = sigdata.taproot_misc_pubkeys.find(script_pubkey);
     if (misc_pk_it != sigdata.taproot_misc_pubkeys.end()) {
         agg_info = misc_pk_it->second.second;
+    } else {
+        agg_info.clear();
     }
 
     for (const auto& [agg_pub, part_pks] : sigdata.musig2_pubkeys) {

Not sure if applicable here, due to the serialize methods, but generally it would be best to enforce initialization at compile-time (via the constructor) and possibly fall back to std::optional.

Kino1994 referenced this in commit 24fce52175 on Aug 2, 2026

bitcoin deleted a comment on Aug 3, 2026