Address PR feedback

Fix comment formatting, return bad block hashes in BlockChain::sync() via existing tuple
rather than via out parameter, remove unused
EthereumCapability::randomPartitionPeers, optimize
EthereumCapability::randomPeers

Author: halfalicious

libdevcore/Common.cpp

@@ -136,10 +136,4 @@ bool isFalse(std::string const& _m)
     return _m == "off" || _m == "no" || _m == "false" || _m == "0";
 }
 
-mt19937_64 g_randomGenerator{random_device{}()};
-
-int randomNumber(int _min, int _max)
-{
-    return uniform_int_distribution<int>{_min, _max}(g_randomGenerator);
-}
 }  // namespace dev

libdevcore/Common.h

@@ -317,6 +317,4 @@ class ExitHandler
 
 bool isTrue(std::string const& _m);
 bool isFalse(std::string const& _m);
-
-int randomNumber(int _min = INT_MIN, int _max = INT_MAX);
 }  // namespace dev

libethereum/BlockChain.cpp

@@ -427,19 +427,19 @@ tuple<ImportRoute, bool, unsigned> BlockChain::sync(
     VerifiedBlocks blocks;
     _bq.drain(blocks, _max);
 
-    h256s badBlockHashes;
-    std::tuple<ImportRoute, unsigned> const importResult = sync(blocks, badBlockHashes, _stateDB);
-    bool const moreBlocks = _bq.doneDrain(badBlockHashes);
-    return {std::get<0>(importResult), moreBlocks, std::get<1>(importResult)};
+    std::tuple<ImportRoute, h256s, unsigned> const importResult = sync(blocks, _stateDB);
+    bool const moreBlocks = _bq.doneDrain(std::get<1>(importResult));
+    return {std::get<0>(importResult), moreBlocks, std::get<2>(importResult)};
 }
 
-tuple<ImportRoute, unsigned> BlockChain::sync(
-    VerifiedBlocks const& _blocks, h256s& o_badBlockHashes, OverlayDB const& _stateDB)
+tuple<ImportRoute, h256s, unsigned> BlockChain::sync(
+    VerifiedBlocks const& _blocks, OverlayDB const& _stateDB)
 {
     h256s fresh;
     h256s dead;
     Transactions goodTransactions;
     unsigned count = 0;
+    h256s badBlockHashes;
     for (VerifiedBlock const& block : _blocks)
     {
         do {
@@ -465,7 +465,7 @@ tuple<ImportRoute, unsigned> BlockChain::sync(
                 cwarn << "ODD: Import queue contains block with unknown parent.";// << LogTag::Error << boost::current_exception_diagnostic_information();
                 // NOTE: don't reimport since the queue should guarantee everything in the right order.
                 // Can't continue - chain bad.
-                o_badBlockHashes.push_back(block.verified.info.hash());
+                badBlockHashes.push_back(block.verified.info.hash());
             }
             catch (dev::eth::FutureTime const&)
             {
@@ -485,11 +485,11 @@ tuple<ImportRoute, unsigned> BlockChain::sync(
                     m_onBad(ex);
                 // NOTE: don't reimport since the queue should guarantee everything in the right order.
                 // Can't continue - chain  bad.
-                o_badBlockHashes.push_back(block.verified.info.hash());
+                badBlockHashes.push_back(block.verified.info.hash());
             }
         } while (false);
     }
-    return {ImportRoute{dead, fresh, goodTransactions}, count};
+    return {ImportRoute{dead, fresh, goodTransactions}, badBlockHashes, count};
 }
 
 pair<ImportResult, ImportRoute> BlockChain::attemptImport(bytes const& _block, OverlayDB const& _stateDB, bool _mustBeNew) noexcept

libethereum/BlockChain.h

@@ -108,10 +108,11 @@ class BlockChain
         BlockQueue& _bq, OverlayDB const& _stateDB, unsigned _max);
 
     /// Import the supplied blocks into the chain. Blocks should be processed in order.
-    /// @returns a tuple with two members - the first (ImportRoute) contains fresh blocks, dead
-    /// blocks and imported transactions. The second contains the imported block count.
-    std::tuple<ImportRoute, unsigned> sync(
-        VerifiedBlocks const& _blocks, h256s& o_badBlockHashes, OverlayDB const& _stateDB);
+    /// @returns a tuple with three members - the first (ImportRoute) contains fresh blocks, dead
+    /// blocks and imported transactions. The second contains hashes of bad blocks. The third
+    /// contains the imported block count.
+    std::tuple<ImportRoute, h256s, unsigned> sync(
+        VerifiedBlocks const& _blocks, OverlayDB const& _stateDB);
 
     /// Attempt to import the given block directly into the BlockChain and sync with the state DB.
     /// @returns the block hashes of any blocks that came into/went out of the canonical block chain.

libethereum/Client.cpp

@@ -386,6 +386,7 @@ void Client::syncBlockQueue()
 //  cdebug << "syncBlockQueue()";
 
     ImportRoute ir;
+    h256s badBlockHashes;
     unsigned count;
     Timer t;
 
@@ -398,8 +399,7 @@ void Client::syncBlockQueue()
                 // Client's lifetime
     h->propagateNewBlocks(verifiedBlocks);
 
-    h256s badBlockHashes;
-    std::tie(ir, count) = bc().sync(*verifiedBlocks, badBlockHashes, m_stateDB);
+    std::tie(ir, badBlockHashes, count) = bc().sync(*verifiedBlocks, m_stateDB);
     m_syncBlockQueue = m_bq.doneDrain(badBlockHashes);
 
     double elapsed = t.elapsed();

libethereum/EthereumCapability.cpp

@@ -7,7 +7,6 @@
 #include "BlockChainSync.h"
 #include "BlockQueue.h"
 #include "TransactionQueue.h"
-#include <libdevcore/Common.h>
 #include <libethcore/Exceptions.h>
 #include <libp2p/Common.h>
 #include <libp2p/Host.h>
@@ -212,14 +211,14 @@ class EthereumHostData : public EthereumHostDataFace
                         top = n + step * (numHeadersToSend - 1);
                     }
                     assert(top <= lastBlock && "invalid top block calculated");
-                    blockHash = m_chain.numberHash(static_cast<unsigned>(
-                        top));  // override start block hash with the hash of the top block we have
+                    // override start block hash with the hash of the top block we have
+                    blockHash = m_chain.numberHash(static_cast<unsigned>(top));
                 }
                 else
                     blockHash = {};
             }
         }
-        else  // block id is a number
+        else // block id is a number
         {
             auto n = _blockId.toInt<bigint>();
             cnetlog << "GetBlockHeaders (" << n << " max: " << _maxHeaders << " skip: " << _skip
@@ -239,8 +238,8 @@ class EthereumHostData : public EthereumHostDataFace
                         top = n + step * (numHeadersToSend - 1);
                     }
                     assert(top <= lastBlock && "invalid top block calculated");
-                    blockHash = m_chain.numberHash(static_cast<unsigned>(
-                        top));  // override start block hash with the hash of the top block we have
+                    // override start block hash with the hash of the top block we have
+                    blockHash = m_chain.numberHash(static_cast<unsigned>(top));
                 }
             }
             else if (n <= std::numeric_limits<unsigned>::max())
@@ -253,21 +252,23 @@ class EthereumHostData : public EthereumHostDataFace
             constexpr unsigned c_blockNumberUsageLimit = 1000;
 
             const auto lastBlock = m_chain.number();
+            // find the number of the block below which we don't expect BC changes.
             const auto limitBlock =
                 lastBlock > c_blockNumberUsageLimit ?
                     lastBlock - c_blockNumberUsageLimit :
-                    0;  // find the number of the block below which we don't expect BC changes.
+                    0;
 
-            while (_step)  // parent hash traversal
+            while (_step) // parent hash traversal
             {
                 auto details = m_chain.details(_h);
                 if (details.number < limitBlock)
-                    break;  // stop using parent hash traversal, fallback to using block numbers
+                    // stop using parent hash traversal, fallback to using block numbers
+                    break;
                 _h = details.parent;
                 --_step;
             }
 
-            if (_step)  // still need lower block
+            if (_step) // still need lower block
             {
                 auto n = m_chain.number(_h);
                 if (n >= _step)
@@ -385,23 +386,6 @@ class EthereumHostData : public EthereumHostDataFace
     BlockChain const& m_chain;
     OverlayDB const& m_db;
 };
-
-std::vector<NodeID> randomPeers(std::vector<NodeID> const& _peers, size_t _count)
-{
-    if (_peers.size() <= _count || _peers.empty())
-        return _peers;
-
-    std::vector<NodeID> peers{_peers};
-    std::vector<NodeID> randomPeers;
-    while (_count-- && !peers.empty())
-    {
-        auto const cIter = peers.begin() + randomNumber(0, peers.size() - 1);
-        randomPeers.push_back(*cIter);
-        peers.erase(cIter);
-    }
-
-    return randomPeers;
-}
 }  // namespace
 
 EthereumCapability::EthereumCapability(shared_ptr<p2p::CapabilityHostFace> _host,
@@ -523,32 +507,29 @@ vector<NodeID> EthereumCapability::selectPeers(
     return allowed;
 }
 
-std::pair<std::vector<NodeID>, std::vector<NodeID>> EthereumCapability::randomPartitionPeers(
-    std::vector<NodeID> const& _peers, std::size_t _number) const
+std::vector<NodeID> EthereumCapability::randomPeers(
+    std::vector<NodeID> const& _peers, size_t _count) const
 {
-    vector<NodeID> part1(_peers);
-    vector<NodeID> part2;
-
-    if (_number >= _peers.size())
-        return std::make_pair(part1, part2);
-
-    std::shuffle(part1.begin(), part1.end(), m_urng);
-
-    // Remove elements from the end of the shuffled part1 vector and move them to part2.
-    std::move(part1.begin() + _number, part1.end(), std::back_inserter(part2));
-    part1.erase(part1.begin() + _number, part1.end());
-    return std::make_pair(move(part1), move(part2));
+    std::vector<NodeID> peers{_peers};
+    if (_count > peers.size())
+        _count = peers.size();
+    auto itEnd = peers.begin() + _count;
+    std::shuffle(peers.begin(), itEnd, m_urng);
+    return {peers.begin(), itEnd};
 }
 
 void EthereumCapability::maintainBlockHashes(h256 const& _currentHash)
 {
     // Send any new block hashes
     auto const detailsFrom = m_chain.details(m_latestBlockHashSent);
     auto const detailsTo = m_chain.details(_currentHash);
-    if (detailsFrom.totalDifficulty >= detailsTo.totalDifficulty ||
-        // don't be sending more than c_maxSendNewBlocksCount "new" block hashes. if there are any
-        // more we were probably waaaay behind.
-        diff(detailsFrom.number, detailsTo.number) > c_maxSendNewBlocksCount)
+
+    if (detailsFrom.totalDifficulty >= detailsTo.totalDifficulty)
+        return;
+    
+    // don't be sending more than c_maxSendNewBlocksCount "new" block hashes. if there are any
+    // more we were probably waaaay behind.
+    if (diff(detailsFrom.number, detailsTo.number) > c_maxSendNewBlocksCount)
         return;
 
     LOG(m_logger) << "Sending new block hashes (current is " << _currentHash << ", was "
@@ -970,8 +951,10 @@ void EthereumCapability::propagateNewBlocks(std::shared_ptr<VerifiedBlocks const
         auto const latestHash = _newBlocks->back().verified.info.hash();
         auto const detailsFrom = m_chain.details(m_latestBlockSent);
         auto const detailsTo = m_chain.details(latestHash);
-        if (detailsFrom.totalDifficulty >= detailsTo.totalDifficulty ||
-            diff(detailsFrom.number, detailsTo.number) > c_maxSendNewBlocksCount)
+        if (detailsFrom.totalDifficulty >= detailsTo.totalDifficulty)
+            return;
+
+        if (diff(detailsFrom.number, detailsTo.number) > c_maxSendNewBlocksCount)
             return;
 
         auto const peersWithoutBlock = selectPeers(

libethereum/EthereumCapability.h

@@ -155,8 +155,7 @@ class EthereumCapability : public p2p::CapabilityFace
     std::vector<NodeID> selectPeers(
         std::function<bool(EthereumPeer const&)> const& _predicate) const;
 
-    std::pair<std::vector<NodeID>, std::vector<NodeID>> randomPartitionPeers(
-        std::vector<NodeID> const& _peers, std::size_t _number) const;
+    std::vector<NodeID> randomPeers(std::vector<NodeID> const& _peers, size_t _count) const;
 
     /// Send top transactions (by nonce and gas price) to available peers
     void maintainTransactions();