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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32080 (OP_CHECKCONTRACTVERIFY by bigspider)
    • #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.

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-05-05 12:12 UTC

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