refactor: Extract RipeMd160 #15294

pull Empact wants to merge 1 commits into bitcoin:master from Empact:ripe-md changing 8 files +27 −20
  1. Empact commented at 7:21 pm on January 30, 2019: member

    To directly return a CRIPEMD160 hash from data.

    Simplifies the call sites.

  2. fanquake added the label Refactoring on Jan 30, 2019
  3. 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 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)).
    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 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.
  4. jimpo commented at 3:46 am on February 1, 2019: contributor
    Concept ACK
  5. Sjors commented at 6:11 pm on February 1, 2019: member

    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.

  6. Empact renamed this:
    [moveonly] Extract RipeMd160
    [moveonly] wallet: Extract RipeMd160
    on Feb 1, 2019
  7. Empact force-pushed on Feb 1, 2019
  8. Empact force-pushed on Feb 1, 2019
  9. Empact force-pushed on Feb 1, 2019
  10. in 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 because crypto/siphash.h includes uint256.h.
  11. Empact force-pushed on Feb 4, 2019
  12. Empact commented at 8:02 am on February 4, 2019: member
    @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.
  13. DrahtBot commented at 10:56 am on February 4, 2019: contributor

    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.

  14. DrahtBot added the label Needs rebase on Apr 10, 2019
  15. Empact force-pushed on Apr 14, 2019
  16. Empact commented at 2:09 am on April 14, 2019: member
    Rebased for #15638
  17. Empact force-pushed on Apr 14, 2019
  18. DrahtBot removed the label Needs rebase on Apr 15, 2019
  19. DrahtBot added the label Needs rebase on Jun 21, 2019
  20. Empact force-pushed on Jun 22, 2019
  21. Empact commented at 5:51 am on June 22, 2019: member
    Rebased
  22. DrahtBot removed the label Needs rebase on Jun 22, 2019
  23. Empact force-pushed on Oct 9, 2019
  24. DrahtBot added the label Needs rebase on Oct 29, 2019
  25. Empact force-pushed on Jan 17, 2020
  26. DrahtBot removed the label Needs rebase on Jan 18, 2020
  27. theStack approved
  29. kristapsk approved
  30. kristapsk commented at 0:13 am on February 3, 2020: contributor
    ACK b29e95c02b5a75681ee3b59d7dd8cceb85d8b27b
  31. Empact requested review from Sjors on Feb 12, 2020
  32. in 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 the const it 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.

     0$ git diff b29e95c cc91980
 1diff --git a/src/script/standard.h b/src/script/standard.h
 2index 717892f1d..49a45f3eb 100644
 3--- a/src/script/standard.h
 4+++ b/src/script/standard.h
 5@@ -24,7 +24,6 @@ public:
 6     CScriptID() : uint160() {}
 7     explicit CScriptID(const CScript& in);
 8     CScriptID(const uint160& in) : uint160(in) {}
 9-    CScriptID(const uint160&& in) : uint160(std::move(in)) {}
10 };
11 
12 /**
  33. Sjors approved
  34. Sjors commented at 1:07 pm on February 13, 2020: member
    Code review ACK b29e95c02b5a75681ee3b59d7dd8cceb85d8b27b
  35. Empact force-pushed on Feb 13, 2020
  36. Sjors commented at 8:48 am on February 14, 2020: member
    re-ACK cc91980afd05796230e8ce95c622729accee1785
  37. theStack commented at 5:40 pm on February 16, 2020: contributor
    re-ACK https://github.com/bitcoin/bitcoin/commit/cc91980afd05796230e8ce95c622729accee1785 (checked that only the const rvalue constructor has been removed since my previous ACK)
  38. Empact requested review from jimpo on Feb 17, 2020
  39. DrahtBot added the label Needs rebase on Jun 21, 2020
  40. Empact force-pushed on Jun 22, 2020
  41. DrahtBot removed the label Needs rebase on Jun 22, 2020
  42. DrahtBot added the label Needs rebase on Jun 28, 2020
  43. Empact force-pushed on Jul 1, 2020
  44. DrahtBot removed the label Needs rebase on Jul 1, 2020
  45. kristapsk approved
  46. kristapsk commented at 2:46 am on November 5, 2020: contributor
    re-ACK ce38f54e66e8584cdd01b7d0dac76bbf010f0e2b
  47. DrahtBot added the label Needs rebase on Jan 11, 2021
  48. Empact force-pushed on Apr 13, 2021
  49. Empact force-pushed on Apr 13, 2021
  50. DrahtBot removed the label Needs rebase on Apr 13, 2021
  51. Empact commented at 4:32 pm on April 13, 2021: member
    Rebased
  52. kristapsk approved
  53. kristapsk commented at 7:44 pm on April 13, 2021: contributor
    re-ACK d7432d398846a4fb6a1ebf0a4d45b195c11acada
  54. theStack approved
  55. theStack commented at 9:15 pm on April 17, 2021: contributor
    re-ACK d7432d398846a4fb6a1ebf0a4d45b195c11acada
  56. 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 -> RIPEMD160 given that

    • RIPEMD represents RIPE Message Digest and
    • RIPE represents RACE 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.
  57. kiminuo approved
  58. kiminuo commented at 6:41 am on May 3, 2021: contributor
    crACK d7432d398846a4fb6a1ebf0a4d45b195c11acada
  59. DrahtBot added the label Needs rebase on May 5, 2021
  60. Empact force-pushed on Sep 11, 2021
  61. dgs2019 approved
  62. DrahtBot removed the label Needs rebase on Sep 11, 2021
  63. kristapsk approved
  64. kristapsk commented at 10:10 am on September 11, 2021: contributor
    re-ACK cd7fe489df0c84d21c80b9df473e883e8a267f0a
  65. theStack approved
  66. theStack commented at 7:02 pm on September 11, 2021: contributor

    re-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 RipeMd160 interface to take a Span<> instead (https://github.com/bitcoin/bitcoin/pull/15294#discussion_r705283491).

  67. 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.
  68. 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

    0-        uint256 fullid(solverdata[0]);
1-        CScriptID id{RipeMd160(fullid)};
2+        CScriptID id{RipeMd160(solverdata[0])};
  69. 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/

     0@@ -3814,9 +3814,9 @@ public:
 1     UniValue operator()(const WitnessV0ScriptHash& id) const
 2     {
 3         UniValue obj(UniValue::VOBJ);
 4+        CScriptID script_id{RipeMd160(id)};
 5         CScript subscript;
 6-        CScriptID scriptid{RipeMd160(id)};
 7-        if (provider && provider->GetCScript(scriptid, subscript)) {
 8+        if (provider && provider->GetCScript(script_id, subscript)) {
 9             ProcessSubScript(subscript, obj);
10         }
11         return obj;
  70. 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

    0+#include <span.h>
1 #include <uint256.h> // For uint160
2-#include <span.h> // For Span
  71. jonatack commented at 1:49 pm on September 13, 2021: contributor

    Nice refactoring/cleanup.

    Code review ACK cd7fe489df0c84d21c80b9df473e883e8a267f0a and debug build is clean

    Some minor suggestions for your consideration if you retouch.

  72. jonatack commented at 1:54 pm on September 13, 2021: contributor
    Perhaps s/moveonly/refactoring/ in the PR title.
  73. DrahtBot added the label Needs rebase on Oct 5, 2021
  74. Empact force-pushed on Oct 13, 2021
  75. DrahtBot removed the label Needs rebase on Oct 13, 2021
  76. sipa commented at 1:41 pm on October 13, 2021: member
    I have a slight preference for not having the crypto/ code depend on Bitcoin Core specific code like uint256.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).
  77. Empact force-pushed on Oct 13, 2021
  78. Empact force-pushed on Oct 13, 2021
  79. Empact renamed this:
    [moveonly] wallet: Extract RipeMd160
    refactor: Extract RipeMd160
    on Oct 13, 2021
  80. Empact commented at 2:31 pm on October 13, 2021: member
    @sipa makes sense - I’ve moved the new code to hash.h
  81. in 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.
  82. Empact force-pushed on Oct 13, 2021
  83. Empact force-pushed on Oct 13, 2021
  84. Empact force-pushed on Oct 13, 2021
  85. in 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 ... comments
  86. Empact force-pushed on Oct 14, 2021
  87. stratospher commented at 3:31 pm on November 13, 2021: contributor
    ACK ca55641. This PR defines RIPEMD160() as an inline function in src/hash.h and makes the function calls more intuitive.
  88. DrahtBot added the label Needs rebase on Dec 3, 2021
  89. Empact force-pushed on Feb 28, 2022
  90. DrahtBot removed the label Needs rebase on Feb 28, 2022
  91. DrahtBot added the label Needs rebase on Oct 20, 2022
  92. achow101 commented at 9:55 pm on December 14, 2022: member
    Are you still working on this?
  93. Empact force-pushed on Dec 16, 2022
  94. Empact commented at 8:21 pm on December 16, 2022: member
    @achow101 Rebased to address conflict with #26158
  95. DrahtBot removed the label Needs rebase on Dec 16, 2022
  96. achow101 commented at 10:03 pm on January 17, 2023: member

    Two more places in src/wallet/rpc/coins.cpp could be updated:

     0diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
 1index 82642194c20..250b580c37c 100644
 2--- a/src/wallet/rpc/coins.cpp
 3+++ b/src/wallet/rpc/coins.cpp
 4@@ -679,8 +679,7 @@ RPCHelpMan listunspent()
 5                             CHECK_NONFATAL(extracted);
 6                             // Also return the witness script
 7                             const WitnessV0ScriptHash& whash = std::get<WitnessV0ScriptHash>(witness_destination);
 8-                            CScriptID id;
 9-                            CRIPEMD160().Write(whash.begin(), whash.size()).Finalize(id.begin());
10+                            CScriptID id{RIPEMD160(whash)};
11                             CScript witnessScript;
12                             if (provider->GetCScript(id, witnessScript)) {
13                                 entry.pushKV("witnessScript", HexStr(witnessScript));
14@@ -689,8 +688,7 @@ RPCHelpMan listunspent()
15                     }
16                 } else if (scriptPubKey.IsPayToWitnessScriptHash()) {
17                     const WitnessV0ScriptHash& whash = std::get<WitnessV0ScriptHash>(address);
18-                    CScriptID id;
19-                    CRIPEMD160().Write(whash.begin(), whash.size()).Finalize(id.begin());
20+                    CScriptID id{RIPEMD160(whash)};
21                     CScript witnessScript;
22                     if (provider->GetCScript(id, witnessScript)) {
23                         entry.pushKV("witnessScript", HexStr(witnessScript));
  97. Empact force-pushed on Jan 26, 2023
  98. refactor: 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
    6879be691b
  99. Empact force-pushed on Jan 26, 2023
  100. achow101 commented at 6:32 pm on January 27, 2023: member
    ACK 6879be691bf636a53208ef058f2ebe18bfa8017c
  101. theStack approved
  102. theStack commented at 3:44 pm on January 29, 2023: contributor
    re-ACK 6879be691bf636a53208ef058f2ebe18bfa8017c
  103. maflcko approved
  104. maflcko commented at 8:48 am on January 30, 2023: member

    review ACK 6879be691bf636a53208ef058f2ebe18bfa8017c 🏔

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 6879be691bf636a53208ef058f2ebe18bfa8017c  🏔
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiT1wv+JeOoQOIiy2NUd4vV1htnx+kXGz/gFFoDnqCY0M9sj6LdXfmwLf3EYpQA
 8fKhnS86I49ms/985+Z9jviAiwZ6iA9ViVZjmoLhcfPqUi8l/cDInfml8UVwLI2dj
 90aXCAqXRex56hieA3q8zHCQ3L12UPyD/pJvdVGIDim18llMB5lxld7gT7eC3fhyO
10i+jo6x86YDdxo7+OgwMXjSb/59IPK4bR/B10lBS64AGLKD1tSCpU2X/hQkUnNsut
11Q/RCJipWDBL5cepJwCR/h1XO4C2KFzYEw998cYm0ZnLLivKTKimrHeDgQfJ3akfZ
12m3YRylDN8OeJ/Fnwr7JuPisTBvisVm7wfFIF30RW8AgPF/5WFR2iddF7Awlf83Qr
13AMnEcCnM6mMCO1xxrxR92NjnEqXsqlTqf6f0ygNuCiFIA5xyADUvT3B+dMiT0eg5
140M6xp/ppnrP5AA8Nc9m+FZ9tEDOwiCh/9cYeZAZyyruEhw1JgjZyUAgU7qMD2cr4
15Ev+BKUFf
16=XLKt
17-----END PGP SIGNATURE-----
  105. maflcko merged this on Jan 30, 2023
  106. maflcko closed this on Jan 30, 2023
  107. sidhujag referenced this in commit 8df7da787a on Jan 30, 2023
  108. Empact deleted the branch on Jan 31, 2023
  109. maflcko referenced this in commit 8fc3bcf93d on Feb 1, 2023
  110. sidhujag referenced this in commit 5d9550900b on Feb 1, 2023
  111. bitcoin locked this on Jan 31, 2024


Empact jimpo Sjors DrahtBot theStack kristapsk sipa kiminuo laanwj dgs2019 maflcko jonatack fanquake stratospher achow101


jimpo

Labels
Refactoring

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: 2024-11-17 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me