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

w0xlt · 2025-04-23T23:19:07Z

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.

DrahtBot · 2025-04-23T23:19:10Z

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, rkrux

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.

hodlinator

Was nerd-sniped by this PR decreasing heap usage. Otherwise these kinds of refactorings are a bit frowned upon.

hodlinator · 2025-04-24T07:21:14Z

src/pubkey.cpp

    CPubKey fullpubkey;
    fullpubkey.Set(b, b + 33);
-    out.push_back(fullpubkey.GetID());
+    CKeyID idEvenPubKey = fullpubkey.GetID();


New code should use snake_case for variables according to developer-notes.md.

Done. Thanks.

hodlinator · 2025-04-24T07:22:57Z

src/pubkey.h

     * This is needed for key lookups since keys are indexed by CKeyID.
     */
-    std::vector<CKeyID> GetKeyIDs() const;
+    std::pair<CKeyID, CKeyID> GetKeyIDs() const;


If you instead return std::array<CKeyID, 2> the data is still on the stack and you don't need to touch signingprovider.h.

Done. Thanks.

w0xlt · 2025-04-24T22:27:04Z

@hodlinator yes, this decreases heap usage, but I don't think there are any relevant performance gains.

hodlinator

ACK 4041cbf

hodlinator · 2025-06-16T09:45:12Z

src/pubkey.cpp

@@ -197,21 +197,24 @@ constexpr XOnlyPubKey XOnlyPubKey::NUMS_H{
    []() consteval { return XOnlyPubKey{"50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0"_hex_u8}; }(),
 };

-std::vector<CKeyID> XOnlyPubKey::GetKeyIDs() const
+std::array<CKeyID,2> XOnlyPubKey::GetKeyIDs() const


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

Suggested change

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

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

Done in acf73d3

hodlinator · 2025-06-16T09:47:45Z

src/pubkey.cpp

-    out.push_back(fullpubkey.GetID());
+
+    CPubKey full_pubkey;
+    full_pubkey.Set(b, b + 33);


nit: Could use cbegin/cend here and below:

Suggested change

full_pubkey.Set(b, b + 33);

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

Done in acf73d3

hodlinator · 2025-06-16T09:50:14Z

src/pubkey.h

@@ -282,10 +282,10 @@ class XOnlyPubKey
    /** Construct a Taproot tweaked output point with this point as internal key. */
    std::optional<std::pair<XOnlyPubKey, bool>> CreateTapTweak(const uint256* merkle_root) const;

-    /** Returns a list of CKeyIDs for the CPubKeys that could have been used to create this XOnlyPubKey.
+    /** Returns two CKeyIDs for the CPubKeys that could have been used to create this XOnlyPubKey.


nit: Could be more precise:

Suggested change

/** Returns two CKeyIDs for the CPubKeys that could have been used to create this XOnlyPubKey.

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

Done in acf73d3

hodlinator · 2025-06-18T07:30:36Z

src/pubkey.cpp

+    CKeyID id_odd_pubKey{full_pubkey.GetID()};
+
+    // Return [0] = even-Y, [1] = odd-Y
+    return std::array<CKeyID, 2>{ id_even_pubkey, id_odd_pubKey };


nit - casing inconsistency:

Suggested change

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

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

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

Suggested change

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

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

Done in 1718177

hodlinator

re-ACK 1718177

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

rkrux

ACK 1718177

Usually minor refactoring changes are avoided but I don't seem to mind this change. It does increase the readability marginally as I like seeing an array of size two in the return type instead of vector while going through the callers of GetKeyIDs function.
Both array & vector satisfy the requirements of the CPP Container as well.

achow101 · 2025-07-30T22:34:44Z

-0

I don't think such refactorings are meaningfully useful. While it may improve readability a bit, I don't think the difference is enough to justify making this change. Such refactorings both can cause merge conflicts with other higher priority work, and also make git blame a little bit harder.

DrahtBot · 2025-07-31T22:45:48Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the Refactoring label Apr 23, 2025

DrahtBot mentioned this pull request Apr 24, 2025

OP_CHECKCONTRACTVERIFY #32080

Draft

hodlinator reviewed Apr 24, 2025

View reviewed changes

This was referenced Apr 24, 2025

descriptors: MuSig2 #31244

Merged

wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys #29675

Open

w0xlt force-pushed the xonly_pair_pubkey branch from feca203 to 4041cbf Compare April 24, 2025 20:33

DrahtBot mentioned this pull request Jun 11, 2025

Musig2 tests #32724

Open

hodlinator reviewed Jun 16, 2025

View reviewed changes

w0xlt force-pushed the xonly_pair_pubkey branch from 4041cbf to acf73d3 Compare June 17, 2025 22:55

hodlinator reviewed Jun 18, 2025

View reviewed changes

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

1718177

w0xlt force-pushed the xonly_pair_pubkey branch from acf73d3 to 1718177 Compare June 18, 2025 17:30

hodlinator approved these changes Jun 18, 2025

View reviewed changes

rkrux approved these changes Jul 30, 2025

View reviewed changes

DrahtBot added the Needs rebase label Jul 31, 2025

w0xlt closed this Aug 4, 2025

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

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

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

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

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

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

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

Uh oh!

Conversation

w0xlt commented Apr 23, 2025

Uh oh!

DrahtBot commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w0xlt commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w0xlt Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w0xlt Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hodlinator Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 30, 2025

Uh oh!

DrahtBot commented Jul 31, 2025

Uh oh!

Uh oh!

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

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

DrahtBot commented Apr 23, 2025 •

edited

Loading

w0xlt commented Apr 24, 2025 •

edited

Loading

w0xlt Jun 17, 2025 •

edited

Loading

w0xlt Jun 17, 2025 •

edited

Loading

hodlinator Jun 18, 2025 •

edited

Loading