Disallow automatic conversion between disparate hash types #17938

Empact · 2020-01-16T00:53:52Z

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".

DrahtBot · 2020-01-16T02:54:16Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Simplify hash.h interface using Spans #19326 (Simplify hash.h interface using Spans by sipa)
[WIP DONOTMERGE] Replace boost with C++17 (std::shared_mutex) #19183 ([WIP DONOTMERGE] Replace boost with C++17 by MarcoFalke)
scripted-diff: TxoutType C++11 scoped enum class #19114 (scripted-diff: TxoutType C++11 scoped enum class by MarcoFalke)
Replace GetScriptForWitness with GetScriptForDestination #16841 (Replace GetScriptForWitness with GetScriptForDestination by meshcollider)
refactor: Extract RipeMd160 #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.

instagibbs · 2020-01-16T17:34:38Z

concept ACK, will review deeper if base is merged

Sjors · 2020-01-17T08:57:03Z

Code review ACK 730267f

Empact · 2020-01-17T18:05:14Z

@instagibbs rebased now that #17924 is merged

Empact · 2020-01-29T22:48:15Z

@instagibbs how about a review?

laanwj · 2020-02-05T14:08:10Z

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.

practicalswift · 2020-02-11T06:46:35Z

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

achow101

ACK 4b7de7c

meshcollider

Code review ACK 4b7de7c

meshcollider · 2020-06-17T08:56:11Z

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);
                         ^

Empact · 2020-06-18T22:38:19Z

Rebased, and the fix is in 1236555

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

maflcko · 2020-06-19T11:47:39Z

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

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

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

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

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

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

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

Empact · 2020-06-19T20:04:35Z

Squashed the fix and rebased.

achow101 · 2020-06-19T22:24:01Z

ACK 4d73691

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

meshcollider · 2020-06-21T08:03:43Z

re-utACK 4d73691

Summary: ``` 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". ``` Backport of core [[bitcoin/bitcoin#17938 | PR17938]]. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9140

Empact mentioned this pull request Jan 16, 2020

Bug: IsUsedDestination shouldn't use key id as script id for ScriptHash #17924

Merged

DrahtBot added GUI RPC/REST/ZMQ Tests Wallet labels Jan 16, 2020

Empact force-pushed the hash-conversion branch from 35e585a to 730267f Compare January 16, 2020 21:41

fanquake removed GUI Tests labels Jan 17, 2020

Empact requested review from Sjors, ryanofsky, achow101 and instagibbs February 6, 2020 12:38

This was referenced Feb 21, 2020

External signer support - Wallet Box edition #16546

Merged

UI external signer support (e.g. hardware wallet) #16549

Closed

refactor: consolidate sendmany and sendtoaddress code #18202

Merged

DrahtBot added the Needs rebase label Feb 25, 2020

DrahtBot mentioned this pull request Jun 6, 2020

[WIP DONOTMERGE] Replace boost with C++17 (std::shared_mutex) #19183

Closed

achow101 approved these changes Jun 11, 2020

View reviewed changes

DrahtBot mentioned this pull request Jun 16, 2020

Replace boost::filesystem with std::filesystem #19245

Closed

5 tasks

meshcollider reviewed Jun 17, 2020

View reviewed changes

maflcko added the Waiting for author label Jun 17, 2020

Empact force-pushed the hash-conversion branch from 4b7de7c to 1236555 Compare June 18, 2020 22:09

DrahtBot mentioned this pull request Jun 19, 2020

Simplify hash.h interface using Spans #19326

Merged

Empact added 8 commits June 19, 2020 12:14

Prefer explicit uint160 conversion

0a5ea32

Prefer explicit CScriptID construction

3fcc468

Convert CPubKey to WitnessV0KeyHash directly

a9e451f

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

Use explicit conversion from PKHash -> CKeyID

2c54217

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

Use explicit conversion from WitnessV0KeyHash -> CKeyID

f32c1e0

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

Explicitly support conversion between equivalent hash types

966a22d

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

Remove an apparently unnecessary conversion

fa9ef2c

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

Disallow automatic conversion between hash types

4d73691

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

Empact force-pushed the hash-conversion branch from 1236555 to 4d73691 Compare June 19, 2020 20:02

meshcollider merged commit bd331bd into bitcoin:master Jun 21, 2020

Empact deleted the hash-conversion branch June 21, 2020 08:35

ryanofsky mentioned this pull request Jul 7, 2020

Multiprocess bitcoin #10102

Draft

maflcko removed the Waiting for author label Apr 4, 2021

kwvg mentioned this pull request May 19, 2021

Merge #19660, #19373, #19841, #13862, #13866, #17280, #17682 and partial #19326, #14978: Auxiliary Backports dashpay/dash#4164

Merged

furszy mentioned this pull request Sep 7, 2021

[Refactor] Differentiate destinations from lookup keys and disallow automatic conversion between them PIVX-Project/PIVX#2543

Closed

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Disallow automatic conversion between disparate hash types #17938

Disallow automatic conversion between disparate hash types #17938

Uh oh!

Empact commented Jan 16, 2020

Uh oh!

DrahtBot commented Jan 16, 2020 •

edited

Loading

Uh oh!

instagibbs commented Jan 16, 2020

Uh oh!

Sjors commented Jan 17, 2020

Uh oh!

Empact commented Jan 17, 2020

Uh oh!

Empact commented Jan 29, 2020

Uh oh!

laanwj commented Feb 5, 2020

Uh oh!

practicalswift commented Feb 11, 2020

Uh oh!

achow101 left a comment

Uh oh!

meshcollider left a comment

Uh oh!

meshcollider commented Jun 17, 2020

Uh oh!

Empact commented Jun 18, 2020

Uh oh!

maflcko commented Jun 19, 2020

Uh oh!

Empact commented Jun 19, 2020

Uh oh!

achow101 commented Jun 19, 2020

Uh oh!

meshcollider commented Jun 21, 2020

Uh oh!

Uh oh!

Disallow automatic conversion between disparate hash types #17938

Disallow automatic conversion between disparate hash types #17938

Uh oh!

Conversation

Empact commented Jan 16, 2020

Uh oh!

DrahtBot commented Jan 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

instagibbs commented Jan 16, 2020

Uh oh!

Sjors commented Jan 17, 2020

Uh oh!

Empact commented Jan 17, 2020

Uh oh!

Empact commented Jan 29, 2020

Uh oh!

laanwj commented Feb 5, 2020

Uh oh!

practicalswift commented Feb 11, 2020

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Jun 17, 2020

Uh oh!

Empact commented Jun 18, 2020

Uh oh!

maflcko commented Jun 19, 2020

Uh oh!

Empact commented Jun 19, 2020

Uh oh!

achow101 commented Jun 19, 2020

Uh oh!

meshcollider commented Jun 21, 2020

Uh oh!

Uh oh!

DrahtBot commented Jan 16, 2020 •

edited

Loading