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

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. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #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. refactor: Update XOnlyPubKey::GetKeyIDs() to return a pair of pubkeys 4041cbf3c6
8. w0xlt force-pushed on Apr 24, 2025
9. 
   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.

Contributors
w0xlt DrahtBot hodlinator

Labels
Refactoring