refactor: Extract RipeMd160 #15294

Empact · 2019-01-30T19:21:30Z

To directly return a CRIPEMD160 hash from data.

Simplifies the call sites.

jimpo

Concept ACK

jimpo · 2019-02-01T03:46:21Z

src/hash.h

+}
+
+/** Compute the 160-bit RIPEMD-160 hash of a vector. */
+inline uint160 RipeMd160(const std::vector<unsigned char>& vch)


Could make this a template function over things with .data() and .size() so that it works for Spans too. In which case you could also remove the other signature for RipeMd160(ptr, size) and just call RipeMd160(Span<unsigned char>(ptr, size)).

I have a follow-up here: https://github.com/Empact/bitcoin/tree/base-blob-data that makes RipeMd160 just a template and makes base_blob and descendants compatible by adding base_blob::data(). I figure it's best to put that up separately as it's mostly to do with base_blob.

Sjors · 2019-02-01T18:11:52Z

Concept ACK. I noticed you're not touching the CRIPEMD160 code in script/interpreter.cpp. I assume that's intentional?

This should have wallet tag I think, since it touches IsMine and signing code.

Why hash.h rather than ripemd160.h? I'm a bit confused by the division of labor between hash.h and the various specific hash function files anyway.

Empact · 2019-02-02T06:26:52Z

src/crypto/ripemd160.h

@@ -8,6 +8,8 @@
 #include <stdint.h>
 #include <stdlib.h>

+#include <uint256.h> // For uint160


note: crypto/ is pretty conservative wrt inclusion, but seems this is okay because crypto/siphash.h includes uint256.h.

Empact · 2019-02-04T08:02:43Z

@Sjors addressed your comments - now in ripemd160.h. Didn't touch script/interpreter.cpp both because the call pattern didn't fit, and to avoid touching consensus code.

DrahtBot · 2019-02-04T10:56:55Z

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

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.

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.

Empact · 2019-04-14T02:09:42Z

Rebased for #15638

Empact · 2019-06-22T05:51:10Z

Rebased

theStack

ACK b29e95c

kristapsk

ACK b29e95c

Sjors

Code review ACK b29e95c

src/script/standard.h

achow101 · 2022-12-14T21:55:51Z

Are you still working on this?

Empact · 2022-12-16T20:21:50Z

@achow101 Rebased to address conflict with #26158

achow101 · 2023-01-17T22:03:25Z

Two more places in src/wallet/rpc/coins.cpp could 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));

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

achow101 · 2023-01-27T18:32:24Z

ACK 6879be6

theStack

re-ACK 6879be6

maflcko

review ACK 6879be6 🏔

Show signature

Signature:

…ions 87f11ef refactor: use `Hash` helper for double-SHA256 calculations (Sebastian Falbesoner) Pull request description: We have two helper templates `Hash(const T& in1)` and `Hash(const T& in1, const T& in2)` available for calculating the double-SHA256 hash of one object or two concatenated objects, respectively: https://github.com/bitcoin/bitcoin/blob/b5868f4b1f884e8d6612f34ca4005fe3a992053d/src/hash.h#L74-L89 This PR uses them in order to increase readability and simplify the code. As in #15294 (which inspired this PR, doing the same for RIPEMD160), the helper is not utilized in validation.cpp and script/interpreter.cpp to avoid touching consensus-relevant code. ACKs for top commit: john-moffett: ACK 87f11ef stickies-v: ACK 87f11ef MarcoFalke: review ACK 87f11ef 😬 Tree-SHA512: 11d7e3d00c89685107784010fbffb33ccafb4d1b6a76c4dceb937b29bb234ef4d54581b16bd0737c8d2994a90cf4fe10a9738c7cc5b6d085c6a819f06176dab9

…alculations 87f11ef refactor: use `Hash` helper for double-SHA256 calculations (Sebastian Falbesoner) Pull request description: We have two helper templates `Hash(const T& in1)` and `Hash(const T& in1, const T& in2)` available for calculating the double-SHA256 hash of one object or two concatenated objects, respectively: https://github.com/bitcoin/bitcoin/blob/b5868f4b1f884e8d6612f34ca4005fe3a992053d/src/hash.h#L74-L89 This PR uses them in order to increase readability and simplify the code. As in bitcoin#15294 (which inspired this PR, doing the same for RIPEMD160), the helper is not utilized in validation.cpp and script/interpreter.cpp to avoid touching consensus-relevant code. ACKs for top commit: john-moffett: ACK 87f11ef stickies-v: ACK 87f11ef MarcoFalke: review ACK 87f11ef 😬 Tree-SHA512: 11d7e3d00c89685107784010fbffb33ccafb4d1b6a76c4dceb937b29bb234ef4d54581b16bd0737c8d2994a90cf4fe10a9738c7cc5b6d085c6a819f06176dab9

fanquake added the Refactoring label Jan 30, 2019

jimpo reviewed Feb 1, 2019

View reviewed changes

Empact changed the title ~~[moveonly] Extract RipeMd160~~ [moveonly] wallet: Extract RipeMd160 Feb 1, 2019

Empact force-pushed the ripe-md branch 3 times, most recently from 3a380b4 to d9871b1 Compare February 1, 2019 21:25

Empact commented Feb 2, 2019

View reviewed changes

Empact force-pushed the ripe-md branch from d9871b1 to 56ded3e Compare February 4, 2019 07:52

DrahtBot added the Needs rebase label Apr 10, 2019

Empact force-pushed the ripe-md branch from 56ded3e to 97c73c2 Compare April 14, 2019 02:09

Empact force-pushed the ripe-md branch from 97c73c2 to df6eacf Compare April 14, 2019 03:52

DrahtBot removed the Needs rebase label Apr 15, 2019

DrahtBot added the Needs rebase label Jun 21, 2019

Empact force-pushed the ripe-md branch from df6eacf to 9bef940 Compare June 22, 2019 05:51

DrahtBot removed the Needs rebase label Jun 22, 2019

Empact force-pushed the ripe-md branch from 9bef940 to 87454b3 Compare October 9, 2019 15:04

DrahtBot added the Needs rebase label Oct 29, 2019

Empact force-pushed the ripe-md branch from 87454b3 to b29e95c Compare January 17, 2020 23:14

DrahtBot removed the Needs rebase label Jan 18, 2020

theStack approved these changes Feb 2, 2020

View reviewed changes

kristapsk approved these changes Feb 3, 2020

View reviewed changes

This was referenced Feb 11, 2020

refactor: Abstract boost::variant out #17953

Closed

Disallow automatic conversion between disparate hash types #17938

Merged

Empact requested a review from Sjors February 12, 2020 20:02

Sjors approved these changes Feb 13, 2020

View reviewed changes

src/script/standard.h Outdated Show resolved Hide resolved

This was referenced Nov 27, 2021

wallet: Split stuff from rpcwallet #23602

Merged

MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet #23647

Closed

DrahtBot added the Needs rebase label Dec 3, 2021

Empact force-pushed the ripe-md branch from ca55641 to c3f015c Compare February 28, 2022 16:33

DrahtBot removed the Needs rebase label Feb 28, 2022

DrahtBot mentioned this pull request Aug 23, 2022

Use static member functions from class instead of instances #25903

Closed

DrahtBot mentioned this pull request Sep 23, 2022

bench: add "priority level" to the benchmark framework #26158

Merged

This was referenced Oct 11, 2022

refactor: move interfaces/* to common/interfaces/* #26293

Closed

refactor: Split util/system into exception, shell, and fs-specific files #25152

Closed

DrahtBot added the Needs rebase label Oct 20, 2022

Empact force-pushed the ripe-md branch from c3f015c to 7860371 Compare December 16, 2022 20:21

DrahtBot removed the Needs rebase label Dec 16, 2022

DrahtBot mentioned this pull request Dec 22, 2022

refactor: Avoid UniValue copies #25429

Closed

DrahtBot mentioned this pull request Jan 7, 2023

refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc #26840

Closed

Empact force-pushed the ripe-md branch from 7860371 to 4391e97 Compare January 26, 2023 21:43

refactor: Extract RIPEMD160

6879be6

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 the ripe-md branch from 4391e97 to 6879be6 Compare January 26, 2023 21:49

theStack approved these changes Jan 29, 2023

View reviewed changes

maflcko approved these changes Jan 30, 2023

View reviewed changes

maflcko merged commit 1c8b80f into bitcoin:master Jan 30, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 30, 2023

Merge bitcoin#15294: refactor: Extract RipeMd160

8df7da7

Empact deleted the ripe-md branch January 31, 2023 04:12

theStack mentioned this pull request Jan 31, 2023

refactor: use Hash helpers for double-SHA256 calculations #27010

Merged

bitcoin locked and limited conversation to collaborators Jan 31, 2024

refactor: Extract RipeMd160 #15294

refactor: Extract RipeMd160 #15294

Uh oh!

Conversation

Empact commented Jan 30, 2019

Uh oh!

jimpo left a comment

Choose a reason for hiding this comment

Uh oh!

jimpo Feb 1, 2019

Choose a reason for hiding this comment

Uh oh!

Empact Feb 4, 2019

Choose a reason for hiding this comment

Uh oh!

Sjors commented Feb 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact Feb 2, 2019

Choose a reason for hiding this comment

Uh oh!

Empact commented Feb 4, 2019

Uh oh!

DrahtBot commented Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

Empact commented Apr 14, 2019

Uh oh!

Empact commented Jun 22, 2019

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Dec 14, 2022

Uh oh!

Empact commented Dec 16, 2022

Uh oh!

achow101 commented Jan 17, 2023

Uh oh!

achow101 commented Jan 27, 2023

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Feb 1, 2019 •

edited

Loading

DrahtBot commented Feb 4, 2019 •

edited

Loading