Disallow automatic conversion between disparate hash types #17938

1. 
   Empact commented at 12:53 AM on January 16, 2020: member

   This bases the script/standard hash types, TxDestination-related and CScriptID on a base template which does not silently convert the underlying uintN type.

   Inspired by and built on #17924. Commits are small and focused to ease review.

   Note some of these changes may be relative to existing bugs of the same sort as #17924. See particularly "Convert CPubKey to WitnessV0KeyHash directly" and "Remove an apparently unnecessary conversion".

2. 
   DrahtBot commented at 2:54 AM on January 16, 2020: 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:

   • #19326 (Simplify hash.h interface using Spans by sipa)
   • #19183 ([WIP DONOTMERGE] Replace boost with C++17 by MarcoFalke)
   • #19114 (scripted-diff: TxoutType C++11 scoped enum class by MarcoFalke)
   • #16841 (Replace GetScriptForWitness with GetScriptForDestination by meshcollider)
   • #15294 ([moveonly] wallet: Extract RipeMd160 by Empact)

   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.

3. DrahtBot added the label GUI on Jan 16, 2020
4. DrahtBot added the label RPC/REST/ZMQ on Jan 16, 2020
5. DrahtBot added the label Tests on Jan 16, 2020
6. DrahtBot added the label Wallet on Jan 16, 2020
7. 
   instagibbs commented at 5:34 PM on January 16, 2020: member

   concept ACK, will review deeper if base is merged

8. Empact force-pushed on Jan 16, 2020
9. fanquake removed the label GUI on Jan 17, 2020
10. fanquake removed the label Tests on Jan 17, 2020
11. 
    Sjors commented at 8:57 AM on January 17, 2020: member

    Code review ACK 730267fd969cded98982b6c6762080dca2332939

12. 
    Empact commented at 6:05 PM on January 17, 2020: member

    @instagibbs rebased now that #17924 is merged

13. 
    Empact commented at 10:48 PM on January 29, 2020: member

    @instagibbs how about a review?

14. 
    laanwj commented at 2:08 PM on February 5, 2020: member

    Concept ACK, I like the idea of using explicit and separate types here to prevent non-sensible conversions, if you can use language features to avoid bugs that's very good.

15. Empact requested review from Sjors on Feb 6, 2020
16. Empact requested review from ryanofsky on Feb 6, 2020
17. Empact requested review from achow101 on Feb 6, 2020
18. Empact requested review from instagibbs on Feb 6, 2020
19. 
    practicalswift commented at 6:46 AM on February 11, 2020: contributor

    Concept ACK: explicit conversion is better (and safer!) than implicit conversion

20. DrahtBot added the label Needs rebase on Feb 25, 2020
21. Empact force-pushed on Feb 28, 2020
22. DrahtBot removed the label Needs rebase on Feb 28, 2020
23. 
    Empact commented at 2:04 AM on February 28, 2020: member

    Rebased, dropped one commit, as #17577 removed the relevant verifymessage conversion.

24. DrahtBot added the label Needs rebase on Mar 9, 2020
25. Empact force-pushed on Mar 10, 2020
26. 
    Empact commented at 11:56 AM on March 10, 2020: member

    Rebased after merge of #18115

27. 
    Sjors commented at 1:31 PM on March 10, 2020: member

    Travis sad.

28. DrahtBot removed the label Needs rebase on Mar 10, 2020
29. Empact force-pushed on Mar 11, 2020
30. 
    laanwj commented at 4:00 PM on March 19, 2020: member

    ACK 4b7de7c402270388794203496728c4baa56c4877

31. 
    instagibbs commented at 6:03 PM on April 14, 2020: member

    ACK https://github.com/bitcoin/bitcoin/pull/17938/commits/4b7de7c402270388794203496728c4baa56c4877

32. achow101 approved
33. 
    achow101 commented at 7:49 PM on June 11, 2020: member

    ACK 4b7de7c402270388794203496728c4baa56c4877

34. 
    meshcollider commented at 8:46 AM on June 17, 2020: contributor

    Code review ACK 4b7de7c402270388794203496728c4baa56c4877

35. 
    meshcollider commented at 8:56 AM on June 17, 2020: contributor

    There is a build error unfortunately

    wallet/scriptpubkeyman.cpp:2054:25: error: no matching function for call to ‘CKeyID::CKeyID(const PKHash&)’
         CKeyID key_id(pkhash);
                             ^
    

36. MarcoFalke added the label Waiting for author on Jun 17, 2020
37. Empact force-pushed on Jun 18, 2020
38. 
    Empact commented at 10:38 PM on June 18, 2020: member

    Rebased, and the fix is in https://github.com/bitcoin/bitcoin/pull/17938/commits/1236555761b79c44f4bc9676260321b8c7095a6f

    I'm not sure what's up with Appveyor, and I'm happy to squash.

39. 
    MarcoFalke commented at 11:47 AM on June 19, 2020: member

    I think it makes sense to squash fixup commits that fix a compile-failure. Otherwise it is not possible to compile earlier commits.

40. Prefer explicit uint160 conversion 0a5ea32ce6
41. Prefer explicit CScriptID construction 3fcc468123
42. Convert CPubKey to WitnessV0KeyHash directly

    The round-tripping through PKHash has no effect, and is
    potentially misleading as such.

    a9e451f144
43. Use explicit conversion from PKHash -> CKeyID

    These types are equivalent, in data etc, so they need only their
    data cast across.
    
    Note a function is used rather than a casting
    operator as CKeyID is defined at a lower level than script/standard

    2c54217f91
44. Use explicit conversion from WitnessV0KeyHash -> CKeyID

    These types are equivalent, in data etc, so they need only their
    data cast across.

    f32c1e07fd
45. Explicitly support conversion between equivalent hash types

    ScriptHash <-> CScriptID
    CKeyID -> PKHash
    PKHash -> WitnessV0KeyHash

    966a22d859
46. Remove an apparently unnecessary conversion

    CScript -> CScriptID -> ScriptHash is unnecessary because
    ScriptHash and CScriptID do the same thing.

    fa9ef2cdbe
47. Disallow automatic conversion between hash types

    A templated BaseHash does not allow for automatic conversion, thus
    conversions much be explicitly allowed / whitelisted, which will
    reduce the risk of unintended conversions.

    4d7369125a
48. Empact force-pushed on Jun 19, 2020
49. 
    Empact commented at 8:04 PM on June 19, 2020: member

    Squashed the fix and rebased.

50. 
    achow101 commented at 10:24 PM on June 19, 2020: member

    ACK 4d7369125a82214ea42b808a32b71b315a5c3c72

    Only change since last is to handle a newly introduced conversion. That's also a good example of the benefits that this PR brings.

51. 
    meshcollider commented at 8:03 AM on June 21, 2020: contributor

    re-utACK 4d7369125a82214ea42b808a32b71b315a5c3c72

52. meshcollider merged this on Jun 21, 2020
53. meshcollider closed this on Jun 21, 2020

    ---

54. Empact deleted the branch on Jun 21, 2020
55. Fabcien referenced this in commit 79bd38319d on Feb 3, 2021
56. MarcoFalke removed the label Waiting for author on Apr 4, 2021
57. DrahtBot locked this on Aug 18, 2022

Review Requested
Sjorsinstagibbsryanofsky