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

achow101 · 2020-09-10T15:33:43Z

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

laanwj · 2020-09-11T12:49:54Z

src/Makefile.am

@@ -193,6 +193,7 @@ BITCOIN_CORE_H = \
  rpc/request.h \
  rpc/server.h \
  rpc/util.h \
+  saltedhash.h \


Maybe place these in util or crypto? I'd prefer not to add files to the source root anymore when possible.

Moved to util.

laanwj · 2020-09-11T12:50:38Z

Concept ACK

DrahtBot · 2020-09-19T13:40:30Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

net: add RAII socket and use it instead of bare SOCKET #20788 (net: add RAII socket and use it instead of bare SOCKET by vasild)
rpc: Allow to ignore specific policy reject reasons #20753 (rpc: Allow to ignore specific policy reject reasons by MarcoFalke)
txmempool: split epoch logic into class #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.

promag

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 ea4c980 and 8ab5135 with git show --color-moved=dimmed_zebra.

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

practicalswift · 2020-09-27T06:22:53Z

Concept ACK

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

achow101 · 2020-09-28T16:16:10Z

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.

sipa · 2020-09-28T19:39:50Z

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

achow101 · 2020-09-28T23:50:14Z

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.

Move the hashers that we use for hash tables to a common place. Moved hashers: - SaltedTxidHasher - SaltedOutpointHasher - FilterHeaderHasher - SignatureCacheHasher - BlockHasher

SaltedSipHasher is a generic hasher that can be used with most things we would hash in an unordered container.

laanwj · 2020-11-23T15:28:02Z

Code review ACK 281fd1a

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.

achow101 · 2020-11-23T17:13:34Z

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.

jnewbery · 2020-12-11T12:25:36Z

src/util/hasher.h

+     * This *must* return size_t. With Boost 1.46 on 32-bit systems the
+     * unordered_map will behave unpredictably if the custom hasher returns a
+     * uint64_t, resulting in failures when syncing the chain (#4634).
+     *


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.

Will remove if I have to push again

jonatack

ACK 281fd1a, code review, debug build and ran bitcoind after rebasing to master @ dff0f6f

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

mzumsande · 2020-12-16T16:59:02Z

src/wallet/scriptpubkeyman.h

@@ -303,7 +303,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv

    /* the HD chain data model (external chain counters) */
    CHDChain m_hd_chain;
-    std::unordered_map<CKeyID, CHDChain, KeyIDHasher> m_inactive_hd_chains;
+    std::unordered_map<CKeyID, CHDChain, SaltedSipHasher> m_inactive_hd_chains;


Looks like KeyIDHasher could be removed since it's no longer used after this this change.

Will remove if I have to push again.

fjahr

utACK 281fd1a

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

fjahr · 2020-12-26T11:00:17Z

src/util/hasher.h

+#include <crypto/siphash.h>
+#include <primitives/transaction.h>
+#include <uint256.h>
+


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.

Will add if I have to push again.

fjahr · 2020-12-26T11:01:04Z

src/util/hasher.h

@@ -0,0 +1,87 @@
+// Copyright (c) 2019 The Bitcoin Core developers


Will fix if I have to push again.

maflcko · 2021-01-13T10:55:34Z

Follow ups:

… new ones 281fd1a Replace KeyIDHasher with SaltedSipHasher (Andrew Chow) 210b693 Add generic SaltedSipHasher (Andrew Chow) 95e61c1 Move Hashers to util/hasher.{cpp/h} (Andrew Chow) Pull request description: 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 bitcoin#19602 (and a few other PRs/branches I have). ACKs for top commit: laanwj: Code review ACK 281fd1a jonatack: ACK 281fd1a, code review, debug build and ran bitcoind after rebasing to master @ dff0f6f fjahr: utACK 281fd1a Tree-SHA512: bb03b231ccf3c9ecefc997b8da9c3770af4819f9be5b0a72997a103864e84046a2ac39b8eadf0dc9247bdccd53f86f433642e3a098882e6748341a9e7736271b

…add some new ones" This reverts commit 6c60aa4.

…9935) 4c69571 doc: remove outdated comment (Martin Zumsande) 16652a9 refactor: Remove unused KeyIDHasher (Martin Zumsande) Pull request description: Small follow-ups to #19935: - Removal of unused `KeyIDHasher` class ([comment in 19935](bitcoin/bitcoin#19935 (comment))) - Removal of an outdated comment, which referred to an old problem with the no longer supported Boost 1.46 and `boost::unordered_map`, now replaced by `std::unordered_map`. ([comment in 19935](bitcoin/bitcoin#19935 (comment))) ACKs for top commit: Saviour1001: Tested ACK <code>[4c69571](https://github.com/bitcoin/bitcoin/commit/4c69571e6eeae2c03d59045ea102baa5fd1c3816)</code> Zero-1729: ACK 4c69571 theStack: ACK 4c69571 🆗 Tree-SHA512: 243fda2120bfac6c40a268ca2c0f34482ce27e71fbc50005c0d13c2ad5db9ee72a037f9937c37cc50ed0f9f6f11ee6afee4ac50e5031d6876ec942f41f38dadf

4c69571 doc: remove outdated comment (Martin Zumsande) 16652a9 refactor: Remove unused KeyIDHasher (Martin Zumsande) Pull request description: Small follow-ups to bitcoin#19935: - Removal of unused `KeyIDHasher` class ([comment in 19935](bitcoin#19935 (comment))) - Removal of an outdated comment, which referred to an old problem with the no longer supported Boost 1.46 and `boost::unordered_map`, now replaced by `std::unordered_map`. ([comment in 19935](bitcoin#19935 (comment))) ACKs for top commit: Saviour1001: Tested ACK <code>[4c69571](https://github.com/bitcoin/bitcoin/commit/4c69571e6eeae2c03d59045ea102baa5fd1c3816)</code> Zero-1729: ACK 4c69571 theStack: ACK 4c69571 🆗 Tree-SHA512: 243fda2120bfac6c40a268ca2c0f34482ce27e71fbc50005c0d13c2ad5db9ee72a037f9937c37cc50ed0f9f6f11ee6afee4ac50e5031d6876ec942f41f38dadf

achow101 force-pushed the mv-saltedhash branch from a2b50c9 to 7e755b7 Compare September 10, 2020 15:54

DrahtBot added Build system Mempool UTXO Db and Indexes labels Sep 10, 2020

achow101 force-pushed the mv-saltedhash branch from 7e755b7 to 3f6e8ec Compare September 10, 2020 16:54

laanwj reviewed Sep 11, 2020

View reviewed changes

achow101 force-pushed the mv-saltedhash branch from 3f6e8ec to b645eff Compare September 14, 2020 16:36

achow101 mentioned this pull request Sep 15, 2020

wallet: Migrate legacy wallets to descriptor wallets #19602

Merged

DrahtBot mentioned this pull request Sep 19, 2020

txmempool: split epoch logic into class #18017

Merged

promag reviewed Sep 26, 2020

View reviewed changes

achow101 force-pushed the mv-saltedhash branch from b645eff to 4e0ce3e Compare September 28, 2020 16:15

achow101 force-pushed the mv-saltedhash branch from 4e0ce3e to 97fde1a Compare September 28, 2020 16:51

achow101 force-pushed the mv-saltedhash branch from 97fde1a to 4ed62a4 Compare September 28, 2020 23:49

achow101 force-pushed the mv-saltedhash branch from 4ed62a4 to 2d5c130 Compare September 28, 2020 23:55

DrahtBot added the Needs rebase label Oct 15, 2020

achow101 force-pushed the mv-saltedhash branch from 2d5c130 to 134a904 Compare October 18, 2020 18:06

DrahtBot removed the Needs rebase label Oct 18, 2020

achow101 added 2 commits November 10, 2020 14:33

Move Hashers to util/hasher.{cpp/h}

95e61c1

Move the hashers that we use for hash tables to a common place. Moved hashers: - SaltedTxidHasher - SaltedOutpointHasher - FilterHeaderHasher - SignatureCacheHasher - BlockHasher

Add generic SaltedSipHasher

210b693

SaltedSipHasher is a generic hasher that can be used with most things we would hash in an unordered container.

Replace KeyIDHasher with SaltedSipHasher

281fd1a

achow101 force-pushed the mv-saltedhash branch from 134a904 to 281fd1a Compare November 10, 2020 19:50

jnewbery reviewed Dec 11, 2020

View reviewed changes

jnewbery added the Review club label Dec 15, 2020

jonatack reviewed Dec 16, 2020

View reviewed changes

mzumsande reviewed Dec 16, 2020

View reviewed changes

DrahtBot mentioned this pull request Dec 23, 2020

rpc: Allow to ignore specific policy reject reasons #20753

Closed

fjahr reviewed Dec 26, 2020

View reviewed changes

DrahtBot mentioned this pull request Jan 8, 2021

net: add RAII socket and use it instead of bare SOCKET #20788

Merged

laanwj merged commit 8ffaf5c into bitcoin:master Jan 13, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 20, 2021

Revert "Merge bitcoin#19935: Move SaltedHashers to separate file and …

742e8ed

…add some new ones" This reverts commit 6c60aa4.

mzumsande mentioned this pull request Aug 22, 2021

refactor: hasher cleanup (follow-up to 19935) #22772

Merged

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

		@@ -0,0 +1,87 @@
		// Copyright (c) 2019 The Bitcoin Core developers

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

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

Uh oh!

Conversation

achow101 commented Sep 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 11, 2020

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Sep 27, 2020

Uh oh!

achow101 commented Sep 28, 2020

Uh oh!

sipa commented Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Sep 28, 2020

Uh oh!

laanwj commented Nov 23, 2020

Uh oh!

achow101 commented Nov 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjahr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 13, 2021

Uh oh!

Uh oh!

achow101 commented Sep 10, 2020 •

edited

Loading

laanwj Sep 11, 2020 •

edited

Loading

DrahtBot commented Sep 19, 2020 •

edited

Loading

sipa commented Sep 28, 2020 •

edited

Loading

fjahr left a comment •

edited

Loading