From d0724b5ee14a369fa68ac63f1428dff64933d2db Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 30 Nov 2018 18:21:03 +0100 Subject: [PATCH 1/2] Merge #13258: uint256: Remove unnecessary crypto/common.h dependency bf2e01097 uint256: Remove unnecessary crypto/common.h use (Karl-Johan Alm) Pull request description: This is an alternative to #13242 which keeps the `ReadLE64` part, but moves the `crypto/common.h` dependency into `crypto/common.h` as a function outside of `uint256`. **Reason:** this change will remove dependencies for `uint256` to `crypto/common.h`, `compat/endian.h`, and `compat/byteswap.h`. This PR removes the need to update tests to be endian-aware/-independent, but keeps the (arguably dubious) `ReadLE64` part (which was only introduced to fix the tests, not for any functionality). Tree-SHA512: 78b35123cdb185b3b3ec59aba5ca8a5db72624d147f2d6a5484ffa5ce626a72f782a01dc6893fc8f5619b03e2eae7b5a03b0df5d43460f3bda428e719e188aec --- src/addrman.cpp | 10 +++++----- src/evo/deterministicmns.h | 11 +++++++++-- src/hash.h | 10 ++++++++++ src/llmq/chainlocks.h | 9 +++++++-- src/llmq/instantsend.cpp | 12 ++++++------ src/llmq/instantsend.h | 4 ++-- src/llmq/signing_shares.cpp | 2 +- src/spork.h | 3 ++- src/txdb.cpp | 2 +- src/txdb.h | 6 ------ src/uint256.h | 21 --------------------- src/validation.h | 6 +++++- src/wallet/wallet.cpp | 3 ++- 13 files changed, 50 insertions(+), 49 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index cfb36cdd7a..df8b131857 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -12,8 +12,8 @@ int CAddrInfo::GetTriedBucket(const uint256& nKey, const std::vector &asmap) const { - uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << GetKey()).GetHash().GetCheapHash(); - uint64_t hash2 = (CHashWriter(SER_GETHASH, 0) << nKey << GetGroup(asmap) << (hash1 % ADDRMAN_TRIED_BUCKETS_PER_GROUP)).GetHash().GetCheapHash(); + uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << GetKey()).GetCheapHash(); + uint64_t hash2 = (CHashWriter(SER_GETHASH, 0) << nKey << GetGroup(asmap) << (hash1 % ADDRMAN_TRIED_BUCKETS_PER_GROUP)).GetCheapHash(); int tried_bucket = hash2 % ADDRMAN_TRIED_BUCKET_COUNT; uint32_t mapped_as = GetMappedAS(asmap); LogPrint(BCLog::NET, "IP %s mapped to AS%i belongs to tried bucket %i\n", ToStringIP(), mapped_as, tried_bucket); @@ -23,8 +23,8 @@ int CAddrInfo::GetTriedBucket(const uint256& nKey, const std::vector &asma int CAddrInfo::GetNewBucket(const uint256& nKey, const CNetAddr& src, const std::vector &asmap) const { std::vector vchSourceGroupKey = src.GetGroup(asmap); - uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << GetGroup(asmap) << vchSourceGroupKey).GetHash().GetCheapHash(); - uint64_t hash2 = (CHashWriter(SER_GETHASH, 0) << nKey << vchSourceGroupKey << (hash1 % ADDRMAN_NEW_BUCKETS_PER_SOURCE_GROUP)).GetHash().GetCheapHash(); + uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << GetGroup(asmap) << vchSourceGroupKey).GetCheapHash(); + uint64_t hash2 = (CHashWriter(SER_GETHASH, 0) << nKey << vchSourceGroupKey << (hash1 % ADDRMAN_NEW_BUCKETS_PER_SOURCE_GROUP)).GetCheapHash(); int new_bucket = hash2 % ADDRMAN_NEW_BUCKET_COUNT; uint32_t mapped_as = GetMappedAS(asmap); LogPrint(BCLog::NET, "IP %s mapped to AS%i belongs to new bucket %i\n", ToStringIP(), mapped_as, new_bucket); @@ -33,7 +33,7 @@ int CAddrInfo::GetNewBucket(const uint256& nKey, const CNetAddr& src, const std: int CAddrInfo::GetBucketPosition(const uint256 &nKey, bool fNew, int nBucket) const { - uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << (fNew ? 'N' : 'K') << nBucket << GetKey()).GetHash().GetCheapHash(); + uint64_t hash1 = (CHashWriter(SER_GETHASH, 0) << nKey << (fNew ? 'N' : 'K') << nBucket << GetKey()).GetCheapHash(); return hash1 % ADDRMAN_BUCKET_SIZE; } diff --git a/src/evo/deterministicmns.h b/src/evo/deterministicmns.h index b7b9ceb9ee..cccc80d736 100644 --- a/src/evo/deterministicmns.h +++ b/src/evo/deterministicmns.h @@ -6,6 +6,7 @@ #define BITCOIN_EVO_DETERMINISTICMNS_H #include +#include #include #include #include @@ -300,10 +301,16 @@ inline void SerReadWrite(Stream& s, immer::map& obj, CSerActi class CDeterministicMNList { +private: + struct ImmerHasher + { + size_t operator()(const uint256& hash) const { return ReadLE64(hash.begin()); } + }; + public: - using MnMap = immer::map; + using MnMap = immer::map; using MnInternalIdMap = immer::map; - using MnUniquePropertyMap = immer::map >; + using MnUniquePropertyMap = immer::map, ImmerHasher>; private: uint256 blockHash; diff --git a/src/hash.h b/src/hash.h index d860aeee1c..dd6499fdb5 100644 --- a/src/hash.h +++ b/src/hash.h @@ -7,6 +7,7 @@ #ifndef BITCOIN_HASH_H #define BITCOIN_HASH_H +#include #include #include #include @@ -208,6 +209,15 @@ public: return result; } + /** + * Returns the first 64 bits from the resulting hash. + */ + inline uint64_t GetCheapHash() { + unsigned char result[CHash256::OUTPUT_SIZE]; + ctx.Finalize(result); + return ReadLE64(result); + } + template CHashWriter& operator<<(const T& obj) { // Serialize to this stream diff --git a/src/llmq/chainlocks.h b/src/llmq/chainlocks.h index 50bbcb4f04..a190a716c1 100644 --- a/src/llmq/chainlocks.h +++ b/src/llmq/chainlocks.h @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -70,9 +71,13 @@ private: uint256 lastSignedMsgHash GUARDED_BY(cs); // We keep track of txids from recently received blocks so that we can check if all TXs got islocked - using BlockTxs = std::unordered_map>>; + struct BlockHasher + { + size_t operator()(const uint256& hash) const { return ReadLE64(hash.begin()); } + }; + using BlockTxs = std::unordered_map>, BlockHasher>; BlockTxs blockTxs GUARDED_BY(cs); - std::unordered_map txFirstSeenTime GUARDED_BY(cs); + std::unordered_map txFirstSeenTime GUARDED_BY(cs); std::map seenChainLocks GUARDED_BY(cs); diff --git a/src/llmq/instantsend.cpp b/src/llmq/instantsend.cpp index 7e40cb95ae..fe3dff1daf 100644 --- a/src/llmq/instantsend.cpp +++ b/src/llmq/instantsend.cpp @@ -169,7 +169,7 @@ void CInstantSendDb::WriteInstantSendLockArchived(CDBBatch& batch, const uint256 batch.Write(std::make_tuple(DB_ARCHIVED_BY_HASH, hash), true); } -std::unordered_map CInstantSendDb::RemoveConfirmedInstantSendLocks(int nUntilHeight) +std::unordered_map CInstantSendDb::RemoveConfirmedInstantSendLocks(int nUntilHeight) { LOCK(cs_db); if (nUntilHeight <= best_confirmed_height) { @@ -186,7 +186,7 @@ std::unordered_map CInstantSendDb::RemoveConfirmed it->Seek(firstKey); CDBBatch batch(*db); - std::unordered_map ret; + std::unordered_map ret; while (it->Valid()) { decltype(firstKey) curKey; if (!it->GetKey(curKey) || std::get<0>(curKey) != DB_MINED_BY_HEIGHT_AND_HASH) { @@ -900,12 +900,12 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks() return fMoreWork; } -std::unordered_set CInstantSendManager::ProcessPendingInstantSendLocks(int signOffset, const std::unordered_map, StaticSaltedHasher>& pend, bool ban) +std::unordered_set CInstantSendManager::ProcessPendingInstantSendLocks(int signOffset, const std::unordered_map, StaticSaltedHasher>& pend, bool ban) { auto llmqType = Params().GetConsensus().llmqTypeInstantSend; CBLSBatchVerifier batchVerifier(false, true, 8); - std::unordered_map recSigs; + std::unordered_map recSigs; size_t verifyCount = 0; size_t alreadyVerified = 0; @@ -977,7 +977,7 @@ std::unordered_set CInstantSendManager::ProcessPendingInstantSendLocks( LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- verified locks. count=%d, alreadyVerified=%d, vt=%d, nodes=%d\n", __func__, verifyCount, alreadyVerified, verifyTimer.count(), batchVerifier.GetUniqueSourceCount()); - std::unordered_set badISLocks; + std::unordered_set badISLocks; if (ban && !batchVerifier.badSources.empty()) { LOCK(cs_main); @@ -1335,7 +1335,7 @@ void CInstantSendManager::HandleFullyConfirmedBlock(const CBlockIndex* pindex) void CInstantSendManager::RemoveMempoolConflictsForLock(const uint256& hash, const CInstantSendLock& islock) { - std::unordered_map toDelete; + std::unordered_map toDelete; { LOCK(mempool.cs); diff --git a/src/llmq/instantsend.h b/src/llmq/instantsend.h index 4ba6a63a8b..4fe7d8f68e 100644 --- a/src/llmq/instantsend.h +++ b/src/llmq/instantsend.h @@ -117,7 +117,7 @@ public: * @param nUntilHeight Removes all IS Locks confirmed up until nUntilHeight * @return returns an unordered_map of the hash of the IS Locks and a pointer object to the IS Locks for all IS Locks which were removed */ - std::unordered_map RemoveConfirmedInstantSendLocks(int nUntilHeight); + std::unordered_map RemoveConfirmedInstantSendLocks(int nUntilHeight); /** * Removes IS Locks from the archive if the tx was confirmed 100 blocks before nUntilHeight * @param nUntilHeight the height from which to base the remove of archive IS Locks @@ -230,7 +230,7 @@ private: void ProcessMessageInstantSendLock(const CNode* pfrom, const CInstantSendLockPtr& islock) LOCKS_EXCLUDED(cs); static bool PreVerifyInstantSendLock(const CInstantSendLock& islock); bool ProcessPendingInstantSendLocks(); - std::unordered_set ProcessPendingInstantSendLocks(int signOffset, const std::unordered_map, StaticSaltedHasher>& pend, bool ban); + std::unordered_set ProcessPendingInstantSendLocks(int signOffset, const std::unordered_map, StaticSaltedHasher>& pend, bool ban); void ProcessInstantSendLock(NodeId from, const uint256& hash, const CInstantSendLockPtr& islock); void AddNonLockedTx(const CTransactionRef& tx, const CBlockIndex* pindexMined); diff --git a/src/llmq/signing_shares.cpp b/src/llmq/signing_shares.cpp index ab1a3c4d9b..a5a08e9fee 100644 --- a/src/llmq/signing_shares.cpp +++ b/src/llmq/signing_shares.cpp @@ -991,7 +991,7 @@ void CSigSharesManager::CollectSigSharesToSendConcentrated(std::unordered_map proTxToNode; + std::unordered_map proTxToNode; for (const auto& pnode : vNodes) { auto verifiedProRegTxHash = pnode->GetVerifiedProRegTxHash(); if (verifiedProRegTxHash.IsNull()) { diff --git a/src/spork.h b/src/spork.h index 5f41463eb0..381af253e2 100644 --- a/src/spork.h +++ b/src/spork.h @@ -7,6 +7,7 @@ #include #include +#include #include #include @@ -162,7 +163,7 @@ private: mutable std::unordered_map mapSporksCachedActive GUARDED_BY(cs); mutable std::unordered_map mapSporksCachedValues GUARDED_BY(cs); - std::unordered_map mapSporksByHash GUARDED_BY(cs); + std::unordered_map mapSporksByHash GUARDED_BY(cs); std::unordered_map > mapSporksActive GUARDED_BY(cs); std::set setSporkPubKeyIDs GUARDED_BY(cs); diff --git a/src/txdb.cpp b/src/txdb.cpp index bd1f65d8b2..ad50b4e55d 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -139,7 +139,7 @@ size_t CCoinsViewDB::EstimateSize() const return db.EstimateSize(DB_COIN, (char)(DB_COIN+1)); } -CBlockTreeDB::CBlockTreeDB(size_t nCacheSize, bool fMemory, bool fWipe) : CDBWrapper(gArgs.IsArgSet("-blocksdir") ? GetDataDir() / "blocks" / "index" : GetBlocksDir() / "index", nCacheSize, fMemory, fWipe), mapHasTxIndexCache(10000, 20000) { +CBlockTreeDB::CBlockTreeDB(size_t nCacheSize, bool fMemory, bool fWipe) : CDBWrapper(gArgs.IsArgSet("-blocksdir") ? GetDataDir() / "blocks" / "index" : GetBlocksDir() / "index", nCacheSize, fMemory, fWipe) { } bool CBlockTreeDB::ReadBlockFileInfo(int nFile, CBlockFileInfo &info) { diff --git a/src/txdb.h b/src/txdb.h index 6f25537e90..21f7a5962d 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -9,10 +9,8 @@ #include #include #include -#include #include #include -#include #include #include @@ -91,10 +89,6 @@ private: /** Access to the block database (blocks/index/) */ class CBlockTreeDB : public CDBWrapper { -private: - CCriticalSection cs; - unordered_limitedmap mapHasTxIndexCache; - public: explicit CBlockTreeDB(size_t nCacheSize, bool fMemory = false, bool fWipe = false); diff --git a/src/uint256.h b/src/uint256.h index cfb00635bf..e533bcf79e 100644 --- a/src/uint256.h +++ b/src/uint256.h @@ -13,7 +13,6 @@ #include #include #include -#include /** Template base class for fixed-sized opaque blobs. */ template @@ -127,16 +126,6 @@ class uint256 : public base_blob<256> { public: uint256() {} explicit uint256(const std::vector& vch) : base_blob<256>(vch) {} - - /** A cheap hash function that just returns 64 bits from the result, it can be - * used when the contents are considered uniformly random. It is not appropriate - * when the value can easily be influenced from outside as e.g. a network adversary could - * provide values to trigger worst-case behavior. - */ - uint64_t GetCheapHash() const - { - return ReadLE64(m_data); - } }; /* uint256 from const char *. @@ -175,15 +164,5 @@ public: } }; -namespace std { - template <> - struct hash - { - std::size_t operator()(const uint256& k) const - { - return (std::size_t)k.GetCheapHash(); - } - }; -} #endif // BITCOIN_UINT256_H diff --git a/src/validation.h b/src/validation.h index 6f06e0660a..cd3b32bcdb 100644 --- a/src/validation.h +++ b/src/validation.h @@ -13,6 +13,7 @@ #include #include +#include // for ReadLE64 #include #include // For CMessageHeader::MessageStartChars #include @@ -114,7 +115,10 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 945 * 1024 * 1024; struct BlockHasher { - size_t operator()(const uint256& hash) const { return hash.GetCheapHash(); } + // this used to call `GetCheapHash()` in uint256, which was later moved; the + // cheap hash function simply calls ReadLE64() however, so the end result is + // identical + size_t operator()(const uint256& hash) const { return ReadLE64(hash.begin()); } }; extern CCriticalSection cs_main; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e3312a4ffa..290d7ca439 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -1643,7 +1644,7 @@ bool CWallet::IsFullyMixed(const COutPoint& outpoint) const ss << outpoint << nCoinJoinSalt; uint256 nHash; CSHA256().Write((const unsigned char*)ss.data(), ss.size()).Finalize(nHash.begin()); - if (nHash.GetCheapHash() % 2 == 0) { + if (ReadLE64(nHash.begin()) % 2 == 0) { return false; } } From b5bc27805ba37f97eca8410c072e43b549f630db Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 14 Dec 2018 13:02:07 -0500 Subject: [PATCH 2/2] Merge #14951: Revert "tests: Support calling add_nodes more than once" fa4b8c90d3 test: add_nodes can only be called once after set_test_params (MarcoFalke) faa831102a Revert "tests: Support calling add_nodes more than once" (MarcoFalke) Pull request description: Writing tests should be straightforward and with little side-effects as possible. I don't see how this is needed and can not be achieved with `self.num_nodes` (and `self.extra_args` et al.) Tree-SHA512: 83a67f2cba9d97e21d80847ff405a4633fcb0d5674486efa57ee1813e46efe8709ae0fb462b8339a01ebeca5c4f2d29ecb1807d648b8fd9ee8ce336b08d580a8 --- .../test_framework/test_framework.py | 24 ++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 671bc18238..5000427785 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -357,7 +357,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): # Public helper methods. These can be accessed by the subclass test scripts. def add_nodes(self, num_nodes, extra_args=None, *, rpchost=None, binary=None): - """Instantiate TestNode objects""" + """Instantiate TestNode objects. + + Should only be called once after the nodes have been specified in + set_test_params().""" if self.bind_to_localhost_only: extra_confs = [["bind=127.0.0.1"]] * num_nodes else: @@ -369,9 +372,24 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): assert_equal(len(extra_confs), num_nodes) assert_equal(len(extra_args), num_nodes) assert_equal(len(binary), num_nodes) + old_num_nodes = len(self.nodes) for i in range(num_nodes): - numnode = len(self.nodes) - self.nodes.append(TestNode(numnode, get_datadir_path(self.options.tmpdir, numnode), self.extra_args_from_options, chain=self.chain, rpchost=rpchost, timewait=self.rpc_timeout, bitcoind=binary[i], bitcoin_cli=self.options.bitcoincli, mocktime=self.mocktime, coverage_dir=self.options.coveragedir, extra_conf=extra_confs[i], extra_args=extra_args[i], use_cli=self.options.usecli, start_perf=self.options.perf)) + self.nodes.append(TestNode( + old_num_nodes + i, + get_datadir_path(self.options.tmpdir, old_num_nodes + i), + self.extra_args_from_options, + chain=self.chain, + rpchost=rpchost, + timewait=self.rpc_timeout, + bitcoind=binary[i], + bitcoin_cli=self.options.bitcoincli, + mocktime=self.mocktime, + coverage_dir=self.options.coveragedir, + extra_conf=extra_confs[i], + extra_args=extra_args[i], + use_cli=self.options.usecli, + start_perf=self.options.perf, + )) def start_node(self, i, *args, **kwargs): """Start a dashd"""