refactor: Update XOnlyPubKey::GetKeyIDs() to return a pair of pubkeys #32332

pull w0xlt wants to merge 1 commits into bitcoin:master from w0xlt:xonly_pair_pubkey changing 2 files +13 −10
  1. w0xlt commented at 11:19 pm on April 23, 2025: contributor

    Currently, XOnlyPubKey::GetKeyIDs() has vector as return type, but always returns a pair of public key IDs.

    This PR changes the code for readability reasons. There may be negligible efficiency gains, but the goal is to make the code more readable and to make the intent of the function clear. There is no change in behavior.

  2. DrahtBot commented at 11:19 pm on April 23, 2025: 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/32332.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hodlinator

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32724 (Musig2 tests by w0xlt)
    • #31244 (descriptors: MuSig2 by achow101)
    • #29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys by achow101)

    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.

  3. DrahtBot added the label Refactoring on Apr 23, 2025
  4. in src/pubkey.cpp:210 in feca2031b3 outdated
    208     std::copy(m_keydata.begin(), m_keydata.end(), b + 1);
    209+
    210     CPubKey fullpubkey;
    211     fullpubkey.Set(b, b + 33);
    212-    out.push_back(fullpubkey.GetID());
    213+    CKeyID idEvenPubKey = fullpubkey.GetID();
    


    hodlinator commented at 7:21 am on April 24, 2025:
    New code should use snake_case for variables according to developer-notes.md.

    w0xlt commented at 8:34 pm on April 24, 2025:
    Done. Thanks.
  5. in src/pubkey.h:288 in feca2031b3 outdated
    281@@ -282,10 +282,10 @@ class XOnlyPubKey
    282     /** Construct a Taproot tweaked output point with this point as internal key. */
    283     std::optional<std::pair<XOnlyPubKey, bool>> CreateTapTweak(const uint256* merkle_root) const;
    284 
    285-    /** Returns a list of CKeyIDs for the CPubKeys that could have been used to create this XOnlyPubKey.
    286+    /** Returns a pair of CKeyIDs for the CPubKeys that could have been used to create this XOnlyPubKey.
    287      * This is needed for key lookups since keys are indexed by CKeyID.
    288      */
    289-    std::vector<CKeyID> GetKeyIDs() const;
    290+    std::pair<CKeyID, CKeyID> GetKeyIDs() const;
    


    hodlinator commented at 7:22 am on April 24, 2025:
    If you instead return std::array<CKeyID, 2> the data is still on the stack and you don’t need to touch signingprovider.h.

    w0xlt commented at 8:34 pm on April 24, 2025:
    Done. Thanks.
  6. hodlinator commented at 7:23 am on April 24, 2025: contributor
    Was nerd-sniped by this PR decreasing heap usage. Otherwise these kinds of refactorings are a bit frowned upon.
  7. w0xlt force-pushed on Apr 24, 2025
  8. w0xlt commented at 10:27 pm on April 24, 2025: contributor
    @hodlinator yes, this decreases heap usage, but I don’t think there are any relevant performance gains.
  9. in src/pubkey.cpp:200 in 4041cbf3c6 outdated
    196@@ -197,21 +197,24 @@ constexpr XOnlyPubKey XOnlyPubKey::NUMS_H{
    197     []() consteval { return XOnlyPubKey{"50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0"_hex_u8}; }(),
    198 };
    199 
    200-std::vector<CKeyID> XOnlyPubKey::GetKeyIDs() const
    201+std::array<CKeyID,2> XOnlyPubKey::GetKeyIDs() const
    


    hodlinator commented at 9:45 am on June 16, 2025:

    nit: Grepped for std::array<\w+,\w+ and only found 2 other occurrences (vs 89 for std::array<\w+, \w+).

    0std::array<CKeyID, 2> XOnlyPubKey::GetKeyIDs() const
    

  10. in src/pubkey.cpp:209 in 4041cbf3c6 outdated
    209-    CPubKey fullpubkey;
    210-    fullpubkey.Set(b, b + 33);
    211-    out.push_back(fullpubkey.GetID());
    212+
    213+    CPubKey full_pubkey;
    214+    full_pubkey.Set(b, b + 33);
    


    hodlinator commented at 9:47 am on June 16, 2025:

    nit: Could use cbegin/cend here and below:

    0    full_pubkey.Set(std::cbegin(b), std::cend(b));
    

  11. in src/pubkey.h:285 in 4041cbf3c6 outdated
    281@@ -282,10 +282,10 @@ class XOnlyPubKey
    282     /** Construct a Taproot tweaked output point with this point as internal key. */
    283     std::optional<std::pair<XOnlyPubKey, bool>> CreateTapTweak(const uint256* merkle_root) const;
    284 
    285-    /** Returns a list of CKeyIDs for the CPubKeys that could have been used to create this XOnlyPubKey.
    286+    /** Returns two CKeyIDs for the CPubKeys that could have been used to create this XOnlyPubKey.
    


    hodlinator commented at 9:50 am on June 16, 2025:

    nit: Could be more precise:

    0    /** Returns CKeyIDs for the even and odd CPubKeys that could have been used to create this XOnlyPubKey.
    

  12. hodlinator commented at 10:09 am on June 16, 2025: contributor
    ACK 4041cbf3c6ce1dfafc4e4c2d1fe834ce52bbc879
  13. w0xlt force-pushed on Jun 17, 2025
  14. in src/pubkey.cpp:217 in acf73d3525 outdated
    220-    return out;
    221+    full_pubkey.Set(std::cbegin(b), std::cend(b));
    222+    CKeyID id_odd_pubKey{full_pubkey.GetID()};
    223+
    224+    // Return [0] = even-Y, [1] = odd-Y
    225+    return std::array<CKeyID, 2>{ id_even_pubkey, id_odd_pubKey };
    


    hodlinator commented at 7:30 am on June 18, 2025:

    nit - casing inconsistency:

    0    return std::array<CKeyID, 2>{ id_even_pubkey, id_odd_pubkey };
    

    Edit: could just drop _pub[kK]ey as we are in the XOnlyPubKey:

    0    return std::array<CKeyID, 2>{ id_even, id_odd };
    

  15. refactor: Update XOnlyPubKey::GetKeyIDs() to return a pair of pubkeys 1718177a30
  16. w0xlt force-pushed on Jun 18, 2025
  17. hodlinator approved
  18. hodlinator commented at 9:02 pm on June 18, 2025: contributor

    re-ACK 1718177a3033db4033329d215811c8eca71fc51a

    Tiny refactor that clarifies the XOnlyPubKey::GetKeyIDs() interface by explicitly returning 2 ids (making heap-usage less likely as a bonus). Also documents even/odd Y values.

    Only trivial changes since first ACK (please resolve).


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: 2025-07-06 21:13 UTC

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