Consolidate XOnlyPubKey lookup hack #22512

1. 
   achow101 commented at 2:12 am on July 21, 2021: member

   The places where we need to lookup information for a XOnlyPubKey currently implement a hack which makes both serializations of the full pubkey in order to try the CKeyIDs for the lookup functions. Instead of duplicating this everywhere it is needed, we can consolidate the CKeyID generation into a function, and then have wrappers around GetPubKey, GetKey, and GetKeyOrigin which takes the XOnlyPubKey, retrieves all of the CKeyIDs (using the new GetKeyIDs() function in XOnlyPubKey), and tries their respective underlying lookup function.

   Split from #22364

2. DrahtBot added the label Descriptors on Jul 21, 2021
3. 
   DrahtBot commented at 10:33 am on July 21, 2021: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   No conflicts as of last run.

4. 
   tryphe commented at 4:52 am on July 30, 2021: contributor

   Concept ACK

   nit: Maybe put the convenience function code blocks in signingprovider.cpp? But it doesn’t exist yet because the existing functions are virtual. Gah.

5. 
   theStack commented at 12:08 pm on August 20, 2021: member

   Concept ACK
6. DrahtBot added the label Needs rebase on Aug 23, 2021
7. in src/pubkey.cpp:185 in 4e64c5232c outdated

   179@@ -180,6 +180,23 @@ XOnlyPubKey::XOnlyPubKey(Span<const unsigned char> bytes)
   180     std::copy(bytes.begin(), bytes.end(), m_keydata.begin());
   181 }
   182 
   183+std::vector<CKeyID> XOnlyPubKey::GetKeyIDs() const
   184+{
   185+    std::vector<CKeyID> out;
   

   ---

   ---

   

   S3RK commented at 7:48 am on August 23, 2021:

   nit: maybe use a fixed size array here? 🤷‍♂️

   ---

   

   achow101 commented at 4:52 pm on August 23, 2021:

   I want to leave open the possibility for future keyids, so std::vector seemed better for that.

   ---

   

   S3RK commented at 8:58 pm on August 23, 2021:

   I’m not sure what you are hinting at. Can you give me an hypothetical example?
8. S3RK approved
9. 
   S3RK commented at 7:49 am on August 23, 2021: member

   Code Review ACK. Need rebase though
10. 
    Zero-1729 commented at 7:54 am on August 23, 2021: contributor

    Concept ACK
11. 
    achow101 commented at 4:53 pm on August 23, 2021: member

    Rebased
12. achow101 force-pushed on Aug 23, 2021
13. DrahtBot removed the label Needs rebase on Aug 23, 2021
14. theStack approved
15. 
    theStack commented at 5:30 pm on August 23, 2021: member

    Code-review ACK 48811023372bbeb32305b3039c0cc1c3b0fd4cc8 🔑
16. 
    Zero-1729 commented at 6:34 pm on August 23, 2021: contributor

    crACK 48811023372bbeb32305b3039c0cc1c3b0fd4cc8
17. in src/pubkey.cpp:186 in 4881102337 outdated

    179@@ -180,6 +180,23 @@ XOnlyPubKey::XOnlyPubKey(Span<const unsigned char> bytes)
    180     std::copy(bytes.begin(), bytes.end(), m_keydata.begin());
    181 }
    182 
    183+std::vector<CKeyID> XOnlyPubKey::GetKeyIDs() const
    184+{
    185+    std::vector<CKeyID> out;
    186+    // For now, use the old full pubkey-based key derivation logic. As it indexed by
    

    ---

    ---

    

    S3RK commented at 8:53 pm on August 23, 2021:

    0    // For now, use the old full pubkey-based key derivation logic. As it is indexed by
    

    ---

    

    S3RK commented at 8:54 pm on August 23, 2021:

    You lost the fix with the rebase

    ---

    

    achow101 commented at 1:39 am on August 24, 2021:

    Oops, done.
18. 
    S3RK commented at 8:56 pm on August 23, 2021: member

    Code review ACK 4881102
19. Consolidate XOnlyPubKey lookup hack

    The places where we need to lookup information for a XOnlyPubKey
    currently implement a hack which makes both serializations of the full
    pubkey in order to try the CKeyIDs for the lookup functions. Instead of
    duplicating this everywhere it is needed, we can consolidate the CKeyID
    generation into a function, and then have wrappers around GetPubKey,
    GetKey, and GetKeyOrigin which takes the XOnlyPubKey, retrieves all of
    the CKeyIDs (using the new GetKeyIDs() function in XOnlyPubKey), and
    tries their respective underlying lookup function.

    d9d3ec07cf
20. achow101 force-pushed on Aug 24, 2021
21. 
    S3RK commented at 6:36 am on August 24, 2021: member

    Code Review reACK d9d3ec0
22. Zero-1729 approved
23. 
    Zero-1729 commented at 7:55 am on August 24, 2021: contributor

    re-crACK d9d3ec0
24. theStack approved
25. 
    theStack commented at 8:01 am on August 24, 2021: member

    re-ACK d9d3ec07cfe45cfa55028cc879dc8a55aecb4d3c
26. 
    meshcollider commented at 1:31 am on September 2, 2021: contributor

    Code review + functional test run ACK d9d3ec07cfe45cfa55028cc879dc8a55aecb4d3c
27. meshcollider merged this on Sep 2, 2021
28. meshcollider closed this on Sep 2, 2021

    ---

29. sidhujag referenced this in commit b39bda7377 on Sep 2, 2021
30. DrahtBot locked this on Sep 2, 2022

Contributors
achow101 DrahtBot tryphe theStack S3RK Zero-1729 meshcollider

Labels
Descriptors