To directly return a CRIPEMD160 hash from data.
Simplifies the call sites.
refactor: Extract RipeMd160 #15294
pull Empact wants to merge 1 commits into bitcoin:master from Empact:ripe-md changing 8 files +27 −20-
Empact commented at 7:21 PM on January 30, 2019: member
- fanquake added the label Refactoring on Jan 30, 2019
-
in src/hash.h:101 in a55fa379a2 outdated
97 | + CRIPEMD160().Write(data, size).Finalize((unsigned char*)&result); 98 | + return result; 99 | +} 100 | + 101 | +/** Compute the 160-bit RIPEMD-160 hash of a vector. */ 102 | +inline uint160 RipeMd160(const std::vector<unsigned char>& vch)
jimpo commented at 3:46 AM on February 1, 2019:Could make this a template function over things with
.data()and.size()so that it works forSpans too. In which case you could also remove the other signature forRipeMd160(ptr, size)and just callRipeMd160(Span<unsigned char>(ptr, size)).
Empact commented at 7:57 AM on February 4, 2019:I have a follow-up here: https://github.com/Empact/bitcoin/tree/base-blob-data that makes
RipeMd160just a template and makesbase_bloband descendants compatible by addingbase_blob::data(). I figure it's best to put that up separately as it's mostly to do withbase_blob.jimpo commented at 3:46 AM on February 1, 2019: contributorConcept ACK
Sjors commented at 6:11 PM on February 1, 2019: memberConcept ACK. I noticed you're not touching the
CRIPEMD160code inscript/interpreter.cpp. I assume that's intentional?This should have wallet tag I think, since it touches IsMine and signing code.
Why
hash.hrather thanripemd160.h? I'm a bit confused by the division of labor between hash.h and the various specific hash function files anyway.Empact renamed this:[moveonly] Extract RipeMd160
[moveonly] wallet: Extract RipeMd160
on Feb 1, 2019Empact force-pushed on Feb 1, 2019Empact force-pushed on Feb 1, 2019Empact force-pushed on Feb 1, 2019in src/crypto/ripemd160.h:12 in d9871b1104 outdated
7 | @@ -8,6 +8,8 @@ 8 | #include <stdint.h> 9 | #include <stdlib.h> 10 | 11 | +#include <uint256.h> // For uint160
Empact commented at 6:26 AM on February 2, 2019:note:
crypto/is pretty conservative wrt inclusion, but seems this is okay becausecrypto/siphash.hincludesuint256.h.Empact force-pushed on Feb 4, 2019DrahtBot commented at 10:56 AM on February 4, 2019: contributor<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
Reviews
See the guideline for information on the review process.
Type Reviewers ACK achow101, theStack, MarcoFalke Concept ACK jimpo Stale ACK Sjors, kiminuo, kristapsk, jonatack, stratospher If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #26840 (refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc by KolbyML)
- #25429 (refactor: Avoid UniValue copies by MarcoFalke)
- #21590 (Safegcd-based modular inverses in MuHash3072 by sipa)
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.
DrahtBot added the label Needs rebase on Apr 10, 2019Empact force-pushed on Apr 14, 2019Empact force-pushed on Apr 14, 2019DrahtBot removed the label Needs rebase on Apr 15, 2019DrahtBot added the label Needs rebase on Jun 21, 2019Empact force-pushed on Jun 22, 2019Empact commented at 5:51 AM on June 22, 2019: memberRebased
DrahtBot removed the label Needs rebase on Jun 22, 2019Empact force-pushed on Oct 9, 2019DrahtBot added the label Needs rebase on Oct 29, 2019Empact force-pushed on Jan 17, 2020DrahtBot removed the label Needs rebase on Jan 18, 2020theStack approvedtheStack commented at 6:41 PM on February 2, 2020: contributorkristapsk approvedkristapsk commented at 12:13 AM on February 3, 2020: contributorACK b29e95c02b5a75681ee3b59d7dd8cceb85d8b27b
Empact requested review from Sjors on Feb 12, 2020in src/script/standard.h:27 in b29e95c02b outdated
23 | @@ -24,6 +24,7 @@ class CScriptID : public uint160 24 | CScriptID() : uint160() {} 25 | explicit CScriptID(const CScript& in); 26 | CScriptID(const uint160& in) : uint160(in) {} 27 | + CScriptID(const uint160&& in) : uint160(std::move(in)) {}
Sjors commented at 12:48 PM on February 13, 2020:Afaik this is the only const rvalue constructor in the codebase. They seem a bit controversial: https://stackoverflow.com/questions/10770181/should-a-move-constructor-take-a-const-or-non-const-rvalue-reference https://codesynthesis.com/~boris/blog/2012/07/24/const-rvalue-references/
It gets called 215 times during the unit test suite, e.g. for descriptor parsing:
CScriptID scriptid{RipeMd160(data[0])};"). When I drop theconstit also gets called 215 times.
sipa commented at 4:10 PM on February 13, 2020:It doesn't do anything here, as it's equivalent to the const lvalue copy constructor.
Empact commented at 9:39 PM on February 13, 2020:Thanks, removed this ineffective attempt at optimization.
$ git diff b29e95c cc91980 diff --git a/src/script/standard.h b/src/script/standard.h index 717892f1d..49a45f3eb 100644 --- a/src/script/standard.h +++ b/src/script/standard.h @@ -24,7 +24,6 @@ public: CScriptID() : uint160() {} explicit CScriptID(const CScript& in); CScriptID(const uint160& in) : uint160(in) {} - CScriptID(const uint160&& in) : uint160(std::move(in)) {} }; /**Sjors approvedSjors commented at 1:07 PM on February 13, 2020: memberCode review ACK b29e95c02b5a75681ee3b59d7dd8cceb85d8b27b
Empact force-pushed on Feb 13, 2020Sjors commented at 8:48 AM on February 14, 2020: memberre-ACK cc91980afd05796230e8ce95c622729accee1785
theStack commented at 5:40 PM on February 16, 2020: contributorre-ACK https://github.com/bitcoin/bitcoin/commit/cc91980afd05796230e8ce95c622729accee1785 (checked that only the const rvalue constructor has been removed since my previous ACK)
Empact requested review from jimpo on Feb 17, 2020DrahtBot added the label Needs rebase on Jun 21, 2020Empact force-pushed on Jun 22, 2020DrahtBot removed the label Needs rebase on Jun 22, 2020DrahtBot added the label Needs rebase on Jun 28, 2020Empact force-pushed on Jul 1, 2020DrahtBot removed the label Needs rebase on Jul 1, 2020kristapsk approvedkristapsk commented at 2:46 AM on November 5, 2020: contributorre-ACK ce38f54e66e8584cdd01b7d0dac76bbf010f0e2b
DrahtBot added the label Needs rebase on Jan 11, 2021Empact force-pushed on Apr 13, 2021Empact force-pushed on Apr 13, 2021DrahtBot removed the label Needs rebase on Apr 13, 2021Empact commented at 4:32 PM on April 13, 2021: memberRebased
kristapsk approvedkristapsk commented at 7:44 PM on April 13, 2021: contributorre-ACK d7432d398846a4fb6a1ebf0a4d45b195c11acada
theStack approvedtheStack commented at 9:15 PM on April 17, 2021: contributorre-ACK d7432d398846a4fb6a1ebf0a4d45b195c11acada
in src/crypto/ripemd160.h:31 in d7432d3988 outdated
26 | @@ -25,4 +27,19 @@ class CRIPEMD160 27 | CRIPEMD160& Reset(); 28 | }; 29 | 30 | +/** Compute the 160-bit RIPEMD-160 hash of an array. */ 31 | +inline uint160 RipeMd160(const unsigned char* data, size_t size)
kiminuo commented at 1:39 PM on May 2, 2021:Nit: I think it would be nicer to have
RipeMd160->RIPEMD160given thatRIPEMDrepresentsRIPE Message DigestandRIPErepresentsRACE Integrity Primitives Evaluation.
Empact commented at 4:58 PM on May 3, 2021:TIL: RIPEMD -> Research and Development in Advanced Communications Technologies in Europe Integrity Primitives Evaluation Message Digest
I could go either way on this one - while all-caps is semantically correct for initialisms, capitalization has distinct meaning in code.
laanwj commented at 12:32 PM on September 9, 2021:Maybe make this take a
Span<>instead. I thint that might even make the second variant below unnecessary, as many containers automatically coerce to a span.kiminuo approvedkiminuo commented at 6:41 AM on May 3, 2021: contributorcrACK d7432d398846a4fb6a1ebf0a4d45b195c11acada
DrahtBot added the label Needs rebase on May 5, 2021Empact force-pushed on Sep 11, 2021dgs2019 approvedDrahtBot removed the label Needs rebase on Sep 11, 2021kristapsk approvedkristapsk commented at 10:10 AM on September 11, 2021: contributorre-ACK cd7fe489df0c84d21c80b9df473e883e8a267f0a
theStack approvedtheStack commented at 7:02 PM on September 11, 2021: contributorre-ACK cd7fe489df0c84d21c80b9df473e883e8a267f0a
Checked via git range-diff that changes since my last ACK are only rebase-related.
Happy to also re-review if you decide to follow laanwj's suggestion of changing the
RipeMd160interface to take aSpan<>instead (https://github.com/bitcoin/bitcoin/pull/15294#discussion_r705283491).in src/crypto/ripemd160.h:40 in c329041bc6 outdated
38 | return result; 39 | } 40 | 41 | -/** Compute the 160-bit RIPEMD-160 hash of an object. */ 42 | -template <typename T> 43 | -inline uint160 RipeMd160(const T& container)
maflcko commented at 11:29 AM on September 12, 2021:Instead of adding code and removing it in the next commit, it might be better to squash the commits. This will reduce review burden for reviewers starting from scratch as well as reduce the git history, blame depth and clone size.
in src/wallet/rpcdump.cpp:914 in c329041bc6 outdated
910 | @@ -910,8 +911,7 @@ static std::string RecurseImportData(const CScript& script, ImportData& import_d 911 | case TxoutType::WITNESS_V0_SCRIPTHASH: { 912 | if (script_ctx == ScriptContext::WITNESS_V0) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Trying to nest P2WSH inside another P2WSH"); 913 | uint256 fullid(solverdata[0]); 914 | - CScriptID id; 915 | - CRIPEMD160().Write(fullid.begin(), fullid.size()).Finalize(id.begin()); 916 | + CScriptID id{RipeMd160(fullid)};
jonatack commented at 1:42 PM on September 13, 2021:suggestion
s/fullid/full_id/, or better yet remove it- uint256 fullid(solverdata[0]); - CScriptID id{RipeMd160(fullid)}; + CScriptID id{RipeMd160(solverdata[0])};in src/wallet/rpcwallet.cpp:3942 in c329041bc6 outdated
3814 | @@ -3816,10 +3815,8 @@ class DescribeWalletAddressVisitor 3815 | { 3816 | UniValue obj(UniValue::VOBJ); 3817 | CScript subscript; 3818 | - CRIPEMD160 hasher; 3819 | - uint160 hash; 3820 | - hasher.Write(id.begin(), 32).Finalize(hash.begin()); 3821 | - if (provider && provider->GetCScript(CScriptID(hash), subscript)) { 3822 | + CScriptID scriptid{RipeMd160(id)}; 3823 | + if (provider && provider->GetCScript(scriptid, subscript)) {
jonatack commented at 1:45 PM on September 13, 2021:suggestion
s/scriptid/script_id/@@ -3814,9 +3814,9 @@ public: UniValue operator()(const WitnessV0ScriptHash& id) const { UniValue obj(UniValue::VOBJ); + CScriptID script_id{RipeMd160(id)}; CScript subscript; - CScriptID scriptid{RipeMd160(id)}; - if (provider && provider->GetCScript(scriptid, subscript)) { + if (provider && provider->GetCScript(script_id, subscript)) { ProcessSubScript(subscript, obj); } return obj;in src/crypto/ripemd160.h:11 in c329041bc6 outdated
7 | @@ -8,6 +8,9 @@ 8 | #include <stdint.h> 9 | #include <stdlib.h> 10 | 11 | +#include <uint256.h> // For uint160 12 | +#include <span.h> // For Span
jonatack commented at 1:45 PM on September 13, 2021:suggestion, sort
+#include <span.h> #include <uint256.h> // For uint160 -#include <span.h> // For Spanjonatack commented at 1:49 PM on September 13, 2021: contributorNice refactoring/cleanup.
Code review ACK cd7fe489df0c84d21c80b9df473e883e8a267f0a and debug build is clean
Some minor suggestions for your consideration if you retouch.
jonatack commented at 1:54 PM on September 13, 2021: contributorPerhaps
s/moveonly/refactoring/in the PR title.DrahtBot added the label Needs rebase on Oct 5, 2021Empact force-pushed on Oct 13, 2021DrahtBot removed the label Needs rebase on Oct 13, 2021sipa commented at 1:41 PM on October 13, 2021: memberI have a slight preference for not having the
crypto/code depend on Bitcoin Core specific code likeuint256.h(for purely aesthetic reasons), and actually going the other direction (also removing it from siphash). I think with the suggestion here that may actually not even be needed (Span seems much "cleaner" to depend on, as it's an almost identical copy of a c++20 stdlib feature).Empact force-pushed on Oct 13, 2021Empact force-pushed on Oct 13, 2021Empact renamed this:[moveonly] wallet: Extract RipeMd160
refactor: Extract RipeMd160
on Oct 13, 2021in src/hash.h:216 in 4779bb4856 outdated
211 | @@ -211,4 +212,12 @@ void BIP32Hash(const ChainCode &chainCode, unsigned int nChild, unsigned char he 212 | */ 213 | CHashWriter TaggedHash(const std::string& tag); 214 | 215 | +/** Compute the 160-bit RIPEMD-160 hash of an array. */ 216 | +inline uint160 RipeMd160(Span<const unsigned char> data)
sipa commented at 2:40 PM on October 13, 2021:Nit (also mentioned earlier): this should be RIPEMD160 I think, as it's an acronym.
Empact force-pushed on Oct 13, 2021Empact force-pushed on Oct 13, 2021Empact force-pushed on Oct 13, 2021in src/script/descriptor.cpp:7 in c2375de502 outdated
3 | @@ -4,10 +4,12 @@ 4 | 5 | #include <script/descriptor.h> 6 | 7 | +#include <hash.h> // For RIPEMD160
fanquake commented at 2:16 AM on October 14, 2021:Please don't add these
For ...commentsEmpact force-pushed on Oct 14, 2021stratospher commented at 3:31 PM on November 13, 2021: contributorACK ca55641. This PR defines RIPEMD160() as an inline function in src/hash.h and makes the function calls more intuitive.
DrahtBot added the label Needs rebase on Dec 3, 2021Empact force-pushed on Feb 28, 2022DrahtBot removed the label Needs rebase on Feb 28, 2022DrahtBot added the label Needs rebase on Oct 20, 2022achow101 commented at 9:55 PM on December 14, 2022: memberAre you still working on this?
Empact force-pushed on Dec 16, 2022DrahtBot removed the label Needs rebase on Dec 16, 2022achow101 commented at 10:03 PM on January 17, 2023: memberTwo more places in
src/wallet/rpc/coins.cppcould be updated:diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index 82642194c20..250b580c37c 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -679,8 +679,7 @@ RPCHelpMan listunspent() CHECK_NONFATAL(extracted); // Also return the witness script const WitnessV0ScriptHash& whash = std::get<WitnessV0ScriptHash>(witness_destination); - CScriptID id; - CRIPEMD160().Write(whash.begin(), whash.size()).Finalize(id.begin()); + CScriptID id{RIPEMD160(whash)}; CScript witnessScript; if (provider->GetCScript(id, witnessScript)) { entry.pushKV("witnessScript", HexStr(witnessScript)); @@ -689,8 +688,7 @@ RPCHelpMan listunspent() } } else if (scriptPubKey.IsPayToWitnessScriptHash()) { const WitnessV0ScriptHash& whash = std::get<WitnessV0ScriptHash>(address); - CScriptID id; - CRIPEMD160().Write(whash.begin(), whash.size()).Finalize(id.begin()); + CScriptID id{RIPEMD160(whash)}; CScript witnessScript; if (provider->GetCScript(id, witnessScript)) { entry.pushKV("witnessScript", HexStr(witnessScript));Empact force-pushed on Jan 26, 20236879be691brefactor: Extract RIPEMD160
To directly return a CRIPEMD160 hash from data. Incidentally, decoding this acronym: * RIPEMD -> RIPE Message Digest * RIPE -> RACE Integrity Primitives Evaluation * RACE -> Research and Development in Advanced Communications Technologies in Europe
Empact force-pushed on Jan 26, 2023achow101 commented at 6:32 PM on January 27, 2023: memberACK 6879be691bf636a53208ef058f2ebe18bfa8017c
theStack approvedtheStack commented at 3:44 PM on January 29, 2023: contributorre-ACK 6879be691bf636a53208ef058f2ebe18bfa8017c
maflcko approvedmaflcko commented at 8:48 AM on January 30, 2023: memberreview ACK 6879be691bf636a53208ef058f2ebe18bfa8017c 🏔
<details><summary>Show signature</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 review ACK 6879be691bf636a53208ef058f2ebe18bfa8017c 🏔 -----BEGIN PGP SIGNATURE----- iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p pUiT1wv+JeOoQOIiy2NUd4vV1htnx+kXGz/gFFoDnqCY0M9sj6LdXfmwLf3EYpQA fKhnS86I49ms/985+Z9jviAiwZ6iA9ViVZjmoLhcfPqUi8l/cDInfml8UVwLI2dj 0aXCAqXRex56hieA3q8zHCQ3L12UPyD/pJvdVGIDim18llMB5lxld7gT7eC3fhyO i+jo6x86YDdxo7+OgwMXjSb/59IPK4bR/B10lBS64AGLKD1tSCpU2X/hQkUnNsut Q/RCJipWDBL5cepJwCR/h1XO4C2KFzYEw998cYm0ZnLLivKTKimrHeDgQfJ3akfZ m3YRylDN8OeJ/Fnwr7JuPisTBvisVm7wfFIF30RW8AgPF/5WFR2iddF7Awlf83Qr AMnEcCnM6mMCO1xxrxR92NjnEqXsqlTqf6f0ygNuCiFIA5xyADUvT3B+dMiT0eg5 0M6xp/ppnrP5AA8Nc9m+FZ9tEDOwiCh/9cYeZAZyyruEhw1JgjZyUAgU7qMD2cr4 Ev+BKUFf =XLKt -----END PGP SIGNATURE-----</details>
maflcko merged this on Jan 30, 2023maflcko closed this on Jan 30, 2023sidhujag referenced this in commit 8df7da787a on Jan 30, 2023Empact deleted the branch on Jan 31, 2023maflcko referenced this in commit 8fc3bcf93d on Feb 1, 2023sidhujag referenced this in commit 5d9550900b on Feb 1, 2023bitcoin locked this on Jan 31, 2024
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-04-27 00:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me
