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

pull davidgumberg wants to merge 1 commits into bitcoin:master from davidgumberg:2026-06-25-fingerprint changing 11 files +56 −52
  1. 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

  2. DrahtBot added the label Consensus on Jun 25, 2026
  3. davidgumberg renamed this:
    script: qa: Improve Key::Fingerprint type safety
    script: qa: Improve `Key::Fingerprint` type safety
    on Jun 25, 2026
  4. DrahtBot commented at 11:59 PM on June 25, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK w0xlt, pseudoramdom

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #34697 (descriptor: fix musig duplicate checks and origin handling by shuv-amp)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

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

    Concept ACK

  6. script: qa: Improve Key::Fingerprint type safety c9a70f9338
  7. davidgumberg force-pushed on Jun 26, 2026
  8. DrahtBot added the label CI failed on Jun 26, 2026
  9. DrahtBot commented at 12:34 AM on June 26, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 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>

  10. DrahtBot removed the label CI failed on Jun 26, 2026
  11. 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.

  12. pseudoramdom commented at 4:48 PM on June 26, 2026: none

    Concept ACK. Thanks for taking care of it.

  13. 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().


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-07-02 09:51 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me