Move SaltedHashers to separate file and add some new ones #19935

pull achow101 wants to merge 3 commits into bitcoin:master from achow101:mv-saltedhash changing 11 files +126 −82
  1. achow101 commented at 3:33 pm on September 10, 2020: member

    There are existing SaltedOutPointHasher and SaltedTxidHasher classes used for std::unordered_map and std::unordered_set that could be useful in other places in the codebase. So we these to their own saltedhash.{cpp/h} file. An existing KeyIDHasher is moved there too. Additionally, ScriptIDHasher, SaltedPubkeyHasher, and SaltedScriptHasher are added so that they can be used in future work.

    KeyIDHasher and ScriptIDHasher are not salted so that equality comparisons of maps and sets keyed by CKeyID and CScriptID will actually work.

    Split from #19602 (and a few other PRs/branches I have).

  2. achow101 force-pushed on Sep 10, 2020
  3. DrahtBot added the label Build system on Sep 10, 2020
  4. DrahtBot added the label Mempool on Sep 10, 2020
  5. DrahtBot added the label UTXO Db and Indexes on Sep 10, 2020
  6. achow101 force-pushed on Sep 10, 2020
  7. in src/Makefile.am:196 in 3f6e8ec8c9 outdated 
    192@@ -193,6 +193,7 @@ BITCOIN_CORE_H = \
193   rpc/request.h \
194   rpc/server.h \
195   rpc/util.h \
196+  saltedhash.h \
    laanwj commented at 12:49 pm on September 11, 2020:
    Maybe place these in util or crypto? I’d prefer not to add files to the source root anymore when possible.
    achow101 commented at 4:36 pm on September 14, 2020:
    Moved to util.
  8. laanwj commented at 12:50 pm on September 11, 2020: member
    Concept ACK
  9. achow101 force-pushed on Sep 14, 2020
  10. DrahtBot commented at 1:40 pm on September 19, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #20788 (net: add RAII socket and use it instead of bare SOCKET by vasild)
    • #20753 (rpc: Allow to ignore specific policy reject reasons by MarcoFalke)
    • #18017 (txmempool: split epoch logic into class by ajtowns)

    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.

  11. promag commented at 10:20 pm on September 26, 2020: member

    Concept ACK.

    Maybe name the file hashers.{h,cpp}? Would be nice to see SaltedScriptHasher and SaltedPubkeyHasher being used - didn’t find them in #19602 either.

    Verified moved code in ea4c9808a5fb883868ffd379e645ef9ba0812d28 and 8ab5135d0df9d76fbfefb3553b3292629bcbd354 with git show --color-moved=dimmed_zebra.

    BTW, should include util/saltedhash.h explicitly - it’s indirectly included in coins.h.

  12. practicalswift commented at 6:22 am on September 27, 2020: contributor

    Concept ACK

    +1 on @promag’s feedback regarding hashers.{cpp,h} and making use of all hashers.

  13. achow101 force-pushed on Sep 28, 2020
  14. achow101 commented at 4:16 pm on September 28, 2020: member

    Maybe name the file hashers.{h,cpp}?

    Done.

    Would be nice to see SaltedScriptHasher and SaltedPubkeyHasher being used - didn’t find them in #19602 either.

    I’m using them in another branch that I haven’t opened a PR for yet.

  15. achow101 force-pushed on Sep 28, 2020
  16. sipa commented at 7:39 pm on September 28, 2020: member
    For the non-optimized ones, it may be better to have a single SaltedSipHasher (or UnsaltedSipHasher) that takes a Span<const unsigned char> as input. That would automatically work for uint256, CPubKey, CScript, CKeyId, and generic ones like std::vector<unsigned char>.
  17. achow101 force-pushed on Sep 28, 2020
  18. achow101 commented at 11:50 pm on September 28, 2020: member

    For the non-optimized ones, it may be better to have a single SaltedSipHasher (or UnsaltedSipHasher) that takes a Span<const unsigned char> as input. That would automatically work for uint256, CPubKey, CScript, CKeyId, and generic ones like std::vector<unsigned char>.

    Good idea. Done.

    Apparently we have other Hashers too, so I’ve added those to util/hasher.

  19. achow101 force-pushed on Sep 28, 2020
  20. DrahtBot added the label Needs rebase on Oct 15, 2020
  21. achow101 force-pushed on Oct 18, 2020
  22. DrahtBot removed the label Needs rebase on Oct 18, 2020
  23. Move Hashers to util/hasher.{cpp/h} 
    Move the hashers that we use for hash tables to a common place.

Moved hashers:
- SaltedTxidHasher
- SaltedOutpointHasher
- FilterHeaderHasher
- SignatureCacheHasher
- BlockHasher
    95e61c1cf2
  24. Add generic SaltedSipHasher 
    SaltedSipHasher is a generic hasher that can be used with most things we
would hash in an unordered container.
    210b693db6
  25. Replace KeyIDHasher with SaltedSipHasher 281fd1a4a0
  26. achow101 force-pushed on Nov 10, 2020
  27. laanwj commented at 3:28 pm on November 23, 2020: member

    Code review ACK 281fd1a4a032cded7f9ea9857e3e99fc793c714b

    I must say that in general I think throwing all hashers of all kinds of different data structures into one grabbag header is not a great subdivision criterion. However in this case it’s all consensus transaction/block related things so I think it’s fine!

    But I wouldn’t want to say, add P2P, wallet or mempool specific data structure hashers to this file.

  28. achow101 commented at 5:13 pm on November 23, 2020: member

    I must say that in general I think throwing all hashers of all kinds of different data structures into one grabbag header is not a great subdivision criterion. However in this case it’s all consensus transaction/block related things so I think it’s fine!

    Right. What I had really needed was TxidHasher and OutpointHasher and it didn’t seem right to be including txmempool.h and sigcache.h in wallet code.

  29. in src/util/hasher.h:39 in 281fd1a4a0 
    34+
35+    /**
36+     * This *must* return size_t. With Boost 1.46 on 32-bit systems the
37+     * unordered_map will behave unpredictably if the custom hasher returns a
38+     * uint64_t, resulting in failures when syncing the chain (#4634).
39+     *
    jnewbery commented at 12:25 pm on December 11, 2020:

    Our minimum boost version is now 1.58, and has been higher than 1.46 since #8920. According to this comment: #4635 (comment), boost versions higher than 1.54 are fine.

    I think we can just delete this part of the comment rather than retaining it.

    achow101 commented at 7:41 pm on January 8, 2021:
    Will remove if I have to push again
  30. jnewbery added the label Review club on Dec 15, 2020
  31. jonatack commented at 4:47 pm on December 16, 2020: member

    ACK 281fd1a4a032cded7f9ea9857e3e99fc793c714b, code review, debug build and ran bitcoind after rebasing to master @ dff0f6f753ea

    src/util/hasher.h could use clang formatting if you retouch

  32. in src/wallet/scriptpubkeyman.h:306 in 281fd1a4a0 
    302@@ -303,7 +303,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
303 
304     /* the HD chain data model (external chain counters) */
305     CHDChain m_hd_chain;
306-    std::unordered_map<CKeyID, CHDChain, KeyIDHasher> m_inactive_hd_chains;
307+    std::unordered_map<CKeyID, CHDChain, SaltedSipHasher> m_inactive_hd_chains;
    mzumsande commented at 4:59 pm on December 16, 2020:
    Looks like KeyIDHasher could be removed since it’s no longer used after this this change.
    achow101 commented at 7:40 pm on January 8, 2021:
    Will remove if I have to push again.
  33. in src/util/hasher.h:11 in 95e61c1cf2 outdated 
     6+#define BITCOIN_UTIL_HASHER_H
 7+
 8+#include <crypto/siphash.h>
 9+#include <primitives/transaction.h>
10+#include <uint256.h>
11+
    fjahr commented at 11:00 am on December 26, 2020:
    I think it would be great if there was a small comment here explaining what belongs into this file and what doesn’t to help future contributors so they don’t have to go back in git history to understand it.
    achow101 commented at 7:41 pm on January 8, 2021:
    Will add if I have to push again.
  34. in src/util/hasher.h:1 in 95e61c1cf2 outdated 
    0@@ -0,0 +1,87 @@
1+// Copyright (c) 2019 The Bitcoin Core developers
    fjahr commented at 11:01 am on December 26, 2020:
    nit: 2020
    achow101 commented at 7:41 pm on January 8, 2021:
    Will fix if I have to push again.
  35. fjahr commented at 11:04 am on December 26, 2020: member

    utACK 281fd1a4a032cded7f9ea9857e3e99fc793c714b

    I agree with @mzumsande that KeyIDHasher seems not longer used and could be removed unless I am missing something.

  36. laanwj merged this on Jan 13, 2021
  37. laanwj closed this on Jan 13, 2021
  38. MarcoFalke commented at 10:55 am on January 13, 2021: member

    Follow ups:

  39. sidhujag referenced this in commit 6a595aaab6 on Jan 13, 2021
  40. sidhujag referenced this in commit 6c60aa4ae4 on Jan 20, 2021
  41. sidhujag referenced this in commit 742e8ed554 on Jan 20, 2021
  42. fanquake referenced this in commit b20ad0eb16 on Aug 24, 2021
  43. sidhujag referenced this in commit b57b963519 on Aug 24, 2021
  44. DrahtBot locked this on Aug 16, 2022


achow101 laanwj DrahtBot promag practicalswift sipa jnewbery jonatack mzumsande fjahr MarcoFalke

Labels
Build system UTXO Db and Indexes Mempool Review club

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