Replace CScriptID and CKeyID in CTxDestination with dedicated types #15452

pull instagibbs wants to merge 2 commits into bitcoin:master from instagibbs:key_script_dest changing 32 files +184 −163
  1. instagibbs commented at 10:01 PM on February 20, 2019: member

    The current usage seems to be an overloading of meanings. CScriptID is used in the wallet as a lookup key, as well as a destination, and CKeyID likewise. Instead, have all destinations be dedicated types.

    New types: CScriptID->ScriptHash CKeyID->PKHash

  2. MarcoFalke added the label Refactoring on Feb 20, 2019
  3. MarcoFalke added the label Wallet on Feb 20, 2019
  4. DrahtBot commented at 11:34 PM on February 20, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #15093 (rpc: Change importwallet to return additional errors by marcinja)
    • #12578 (gui: Add transaction record type Fee by promag)

    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.

  5. in src/script/standard.h:84 in 07fb59957c outdated 
      79 | +    explicit PKHash(const uint160& hash) : uint160(hash) {}
  80 | +    explicit PKHash(const CPubKey& pubkey);
  81 | +    using uint160::uint160;
  82 | +};
  83 | +
  84 | +struct SHash : public uint160
    laanwj commented at 9:46 AM on February 21, 2019:

    Concept ACK—I dislike the name SHash though, it sounds too general. I'd prefer a longer name that makes it more clear what is being hashed, maybe ScriptHash?

    instagibbs commented at 2:06 AM on February 22, 2019:

    done

  6. instagibbs force-pushed on Feb 22, 2019
  7. sipa commented at 2:17 AM on February 22, 2019: member

    Concept ACK. I believe @ryanofsky suggested something like this before.

  8. in src/script/standard.h:88 in 3330398ab5 outdated 
      83 | +
  84 | +struct ScriptHash : public uint160
  85 | +{
  86 | +    ScriptHash() : uint160() {}
  87 | +    explicit ScriptHash(const uint160& hash) : uint160(hash) {}
  88 | +    explicit ScriptHash(const CScript& script);
    practicalswift commented at 11:36 PM on February 25, 2019:

    Nit: Make sure parameter names match between declaration and definition :-)

  9. instagibbs commented at 2:14 PM on February 26, 2019: member

    Not sure how to read appveyor error tbh

  10. MarcoFalke commented at 2:34 PM on February 26, 2019: member

    The appveyor sync failure was fixed earlier this week

  11. Sjors commented at 8:16 AM on February 27, 2019: member

    Concept ACK. Maybe enumerate the new types in the PR description?

    ~Shouldn't CKeyID GetID() in pubkey.h also return a PKHash ? That breaks a few other things from the looks of it, but otherwise it seems inconsistent.~ (no actually that's the point :-)

  12. in src/script/standard.h:80 in 3330398ab5 outdated 
      72 | @@ -73,6 +73,22 @@ class CNoDestination {
  73 |      friend bool operator<(const CNoDestination &a, const CNoDestination &b) { return true; }
  74 |  };
  75 |  
  76 | +struct PKHash : public uint160
  77 | +{
  78 | +    PKHash() : uint160() {}
  79 | +    explicit PKHash(const uint160& hash) : uint160(hash) {}
  80 | +    explicit PKHash(const CPubKey& pubkey);
    Sjors commented at 8:28 AM on February 27, 2019:

    Inline definition might be more clear but I don't want to upset any strong c++ preferences out there.

    instagibbs commented at 2:09 PM on February 27, 2019:

    It's a mixed bag here(definition for other constructors are in .cpp), I'll leave as-is unless someone else chimes in

  13. stevenroose commented at 3:08 PM on March 14, 2019: contributor

    utACK 3330398ab5df6f67859307339c0bb71c6b687220

  14. DrahtBot added the label Needs rebase on Mar 28, 2019
  15. instagibbs force-pushed on Mar 28, 2019
  16. instagibbs commented at 4:13 PM on March 28, 2019: member

    rebased

  17. DrahtBot removed the label Needs rebase on Mar 28, 2019
  18. DrahtBot added the label Needs rebase on Apr 16, 2019
  19. instagibbs force-pushed on Apr 23, 2019
  20. instagibbs commented at 1:54 PM on April 23, 2019: member

    rebased and added description of new destination types in OP @Sjors

  21. DrahtBot removed the label Needs rebase on Apr 23, 2019
  22. in src/wallet/wallet.cpp:3771 in 51482131c6 outdated 
    3767 | @@ -3768,7 +3768,7 @@ void CWallet::GetKeyBirthTimes(interfaces::Chain::Lock& locked_chain, std::map<C
3768 |      // get birth times for keys with metadata
3769 |      for (const auto& entry : mapKeyMetadata) {
3770 |          if (entry.second.nCreateTime) {
3771 | -            mapKeyBirth[entry.first] = entry.second.nCreateTime;
3772 | +            mapKeyBirth[PKHash(entry.first)] = entry.second.nCreateTime;
    ryanofsky commented at 3:48 PM on April 23, 2019:

    This change and the 2 changes below will work but don't really make sense logically. Would suggest reverting these and just changing the map key type to:

    boost::variant<CKeyID, CScriptID>

    or maybe

    CKeyID

    if you wanted to drop the "TODO: include scripts in GetKeyBirthTimes() output instead of separate" comment.

    instagibbs commented at 4:50 PM on April 23, 2019:

    done

  23. ryanofsky approved
  24. ryanofsky commented at 4:12 PM on April 23, 2019: member

    Conditional utACK 51482131c645ad565ad032a57bd105ede65f9b86 if fixes are added for travis failures. I'm guessing this will just require rebasing and adding some more CKeyID/CScriptID replacements.

    This change was also suggested a while ago in: #11403 (review) and I think it could be simplified a little by taking the other suggestion in that comment and making ScriptHash inherit from CScriptID and PKHash inherit from CKeyID. This would avoid the need to add explicit conversions several places in this PR. I think it's important for conversions from CScriptID to ScriptHash and CKeyID to PKHash to be explicit, but it seems fine for conversions in the opposite direction to be implicit.

  25. instagibbs force-pushed on Apr 23, 2019
  26. instagibbs commented at 4:51 PM on April 23, 2019: member

    fixed a single dangling test case that was expecting a PKHash, and added a commit to change GetKeyBirthTimes to returning CKeyID instead of CTxDestination.

  27. ryanofsky approved
  28. ryanofsky commented at 6:32 PM on April 23, 2019: member

    utACK ddc21f2f2360a3d7e7acdddd4478822d47c44f7a. Changes since last review are fixing test case and switching GetKeyBirthTimes away from CTxDestination.

  29. Sjors commented at 10:39 AM on April 24, 2019: member

    tACK ddc21f2. Compiled, opened a wallet and ran test suite and bench on macOS 10.14.4. Not sure what else to test here.

  30. in src/rpc/rawtransaction.cpp:574 in d6dc183ae8 outdated 
     570 | @@ -571,7 +571,7 @@ static UniValue decodescript(const JSONRPCRequest& request)
 571 |      if (type.isStr() && type.get_str() != "scripthash") {
 572 |          // P2SH cannot be wrapped in a P2SH. If this script is already a P2SH,
 573 |          // don't return the address for a P2SH of the P2SH.
 574 | -        r.pushKV("p2sh", EncodeDestination(CScriptID(script)));
 575 | +        r.pushKV("p2sh", EncodeDestination(ScriptHash(CScriptID(script))));
    meshcollider commented at 3:32 AM on April 27, 2019:

    ScriptHash(script)? Same on line 601

    instagibbs commented at 1:13 PM on April 29, 2019:

    fixed

  31. in src/test/script_standard_tests.cpp:493 in d6dc183ae8 outdated 
     489 | @@ -490,8 +490,8 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine)
 490 |      {
 491 |          CBasicKeyStore keystore;
 492 |  
 493 | -        CScript redeemscript = GetScriptForDestination(pubkeys[0].GetID());
 494 | -        CScript witnessscript = GetScriptForDestination(CScriptID(redeemscript));
 495 | +        CScript redeemscript = GetScriptForDestination(ScriptHash(PKHash(pubkeys[0])));
    meshcollider commented at 3:37 AM on April 27, 2019:

    Why is there ScriptHash here?

    instagibbs commented at 1:07 PM on April 29, 2019:

    hmm seems I broke this test case, thanks

  32. in src/test/script_standard_tests.cpp:601 in d6dc183ae8 outdated 
     597 | @@ -598,7 +598,7 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine)
 598 |          BOOST_CHECK(keystore.AddKey(keys[1]));
 599 |  
 600 |          CScript redeemScript = GetScriptForMultisig(2, {uncompressedPubkey, pubkeys[1]});
 601 | -        scriptPubKey = GetScriptForDestination(CScriptID(redeemScript));
 602 | +        scriptPubKey = GetScriptForDestination(ScriptHash(CScriptID(redeemScript)));
    meshcollider commented at 3:38 AM on April 27, 2019:

    ScriptHash(redeemScript) again?

    instagibbs commented at 1:13 PM on April 29, 2019:

    fixed

    Sjors commented at 2:58 PM on May 8, 2019:

    A followup PR that would make this particular mistake fail the test would be nice though.

  33. instagibbs force-pushed on Apr 29, 2019
  34. Replace CScriptID and CKeyID in CTxDestination with dedicated types 70946e7fee
  35. GetKeyBirthTimes should return key ids, not destinations 78e407ad0c
  36. instagibbs force-pushed on Apr 29, 2019
  37. instagibbs commented at 2:16 PM on April 29, 2019: member

    all comments addressed

  38. ryanofsky approved
  39. ryanofsky commented at 3:19 PM on April 29, 2019: member

    utACK 78e407ad0c26190a22de1bc8ed900164a44a36c3. Only changes are removing extra CScriptID()s and fixing the test case.

  40. fanquake requested review from Sjors on May 8, 2019
  41. fanquake requested review from meshcollider on May 8, 2019
  42. Sjors approved
  43. Sjors commented at 2:57 PM on May 8, 2019: member

    utACK 78e407a

  44. meshcollider approved
  45. meshcollider commented at 11:31 AM on May 9, 2019: contributor

    utACK https://github.com/bitcoin/bitcoin/pull/15452/commits/78e407ad0c26190a22de1bc8ed900164a44a36c3

    By removal of the // TODO: include scripts in GetKeyBirthTimes() output instead of separate comment, I assume you think that shouldn't be done, rather than because you implemented it.

  46. laanwj merged this on May 9, 2019
  47. laanwj closed this on May 9, 2019
  48. laanwj referenced this in commit de5af41e35 on May 9, 2019
  49. instagibbs commented at 5:00 PM on May 9, 2019: member

    @meshcollider payoff was very low, and descriptor wallets would have changed whatever someone was going to do

  50. sidhujag referenced this in commit 4add681637 on May 10, 2019
  51. domob1812 referenced this in commit bc7e710f26 on May 13, 2019
  52. domob1812 referenced this in commit 0f9424ed42 on May 13, 2019
  53. deadalnix referenced this in commit f8e75cb4a8 on Jun 6, 2020
  54. deadalnix referenced this in commit 323a3af1e7 on Jun 6, 2020
  55. dzutto referenced this in commit 6e2d52d5cf on Dec 10, 2021
  56. DrahtBot locked this on Dec 16, 2021
Contributors
instagibbsDrahtBotlaanwjsipapracticalswiftMarcoFalkeSjorsstevenrooseryanofskymeshcollider
Labels
RefactoringWallet
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: 2026-05-02 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me