XOnlyPubKey::GetKeyIDs() to return a pair of pubkeys
#32332
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32332.
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.
Reviewers, this pull request conflicts with the following ones:
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.
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();
snake_case for variables according to developer-notes.md.
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;
std::array<CKeyID, 2> the data is still on the stack and you don’t need to touch signingprovider.h.
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
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
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);
nit: Could use cbegin/cend here and below:
0 full_pubkey.Set(std::cbegin(b), std::cend(b));
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.
nit: Could be more precise:
0 /** Returns CKeyIDs for the even and odd CPubKeys that could have been used to create this XOnlyPubKey.
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 };
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 };
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).
ACK 1718177a3033db4033329d215811c8eca71fc51a
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.
-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.