Don’t return an address for invalid pubkeys #4954

1. 
   aalness commented at 11:16 pm on September 20, 2014: contributor

   See: https://github.com/coinbase/toshi/issues/76
2. in src/script/standard.cpp: in a8de87f981 outdated

   202@@ -203,7 +203,11 @@ bool ExtractDestination(const CScript& scriptPubKey, CTxDestination& addressRet)
   203 
   204     if (whichType == TX_PUBKEY)
   205     {
   206-        addressRet = CPubKey(vSolutions[0]).GetID();
   207+        CPubKey pubKey(vSolutions[0]);
   208+        if (!pubKey.size())
   

   ---

   ---

   

   laanwj commented at 10:23 am on September 22, 2014:

   Please use pubkey.IsValid() here
3. in src/script/standard.cpp: in a8de87f981 outdated

   240@@ -237,9 +241,16 @@ bool ExtractDestinations(const CScript& scriptPubKey, txnouttype& typeRet, vecto
   241         nRequiredRet = vSolutions.front()[0];
   242         for (unsigned int i = 1; i < vSolutions.size()-1; i++)
   243         {
   244-            CTxDestination address = CPubKey(vSolutions[i]).GetID();
   245+            CPubKey pubKey(vSolutions[i]);
   246+            if (!pubKey.size())
   

   ---

   ---

   

   laanwj commented at 10:24 am on September 22, 2014:

   Same comment as above.

   Also: should we immediately error out here? Or is it correct to return only part of the destinations?

   ---

   

   aalness commented at 7:01 pm on September 22, 2014:

   Thanks for the review. I think it’s appropriate to show any addresses that can be derived from valid pubkeys in the script.
4. 
   BitcoinPullTester commented at 7:19 pm on September 22, 2014: none

   Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4954_c4c84615ace00c818aa1acc74b225a27d3e65914/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
5. 
   petertodd commented at 7:37 am on September 23, 2014: contributor

   ACK

   The addresses returned were incorrect anyway, as CPubKey() stores all incorrect pubkeys as an empty string, which hashes to 1HT7xU2Ngenf7D4yocz2SAcnNLW7rK8d4E

6. laanwj added the label TX scripts on Sep 23, 2014
7. 
   laanwj commented at 8:36 am on September 25, 2014: member

   To reiterate my question above, which seems to have been lost: In ExtractDestinations, should we immediately error out when we encounter an invalid pubkey? Or is it not confusing to return only part of the destinations as is done now?
8. 
   aalness commented at 9:19 pm on September 25, 2014: contributor

   @laanwj I think it’s a bit subjective. The “addresses” field serves no protocol purpose but is merely informative for the user. IMO, if an address can be derived from the script we should show it to the user and leave it to them to infer what the implication is if the number of signatures required doesn’t match the number of addresses they see. That said, I don’t feel strongly about this. The only thing I do feel strongly about is not including the hash160 of an empty string in the “addresses” array.
9. 
   sipa commented at 11:52 pm on September 25, 2014: member

   IMHO, the fact that the “addresses” is plural is a historic mistake, confusing between addresses (shorthand notations for particular scripts) and addresses (as identifiers for particular public keys).

   Every script by definition only has (at most) one address, though potentially we may recognize the script as involving particular keys.

10. 
    sipa commented at 2:19 am on October 2, 2014: member

    utACK
11. 
    sipa commented at 10:27 pm on October 6, 2014: member

    Can you squash the commits?
12. Don't return an address for invalid pubkeys 9d7cd4c598
13. aalness force-pushed on Oct 7, 2014
14. laanwj merged this on Oct 7, 2014
15. laanwj closed this on Oct 7, 2014

    ---

16. laanwj referenced this in commit 953f16cb4e on Oct 7, 2014
17. MarcoFalke locked this on Sep 8, 2021

Contributors
aalness laanwj BitcoinPullTester petertodd sipa

Labels
Validation