Address PR feedback

Fix some bugs:
* Only shuffle the peers list in randomPeers if a subset
of them are being returned
* Update the hash of the latest block sent in EthereumCapability if
we're too far behind - otherwise we'll always be behind and therefore
send any new blocks or new block hashes.

Also, change the CHANGELOG entry from "added" to "changed" (since this
isn't a new feature but modification of existing behavior)

Author: halfalicious

CHANGELOG.md

@@ -19,7 +19,6 @@
 - Added: [#5701](https://github.com/ethereum/aleth/issues/5701) Outputs ENR text representation in admin.nodeInfo RPC.
 - Added: [#5705](https://github.com/ethereum/aleth/pull/5705) Istanbul support: EIP 1108 Reduce alt_bn128 precompile gas costs.
 - Added: [#5707](https://github.com/ethereum/aleth/pull/5707) Aleth waits for 2 seconds after sending disconnect to peer before closing socket.
-- Added: [#5713](https://github.com/ethereum/aleth/pull/5713) Propagate new blocks after PoW check rather than after import into the blockchain.
 - Changed: [#5532](https://github.com/ethereum/aleth/pull/5532) The leveldb is upgraded to 1.22. This is breaking change on Windows and the old databases are not compatible.
 - Changed: [#5559](https://github.com/ethereum/aleth/pull/5559) Update peer validation error messages.
 - Changed: [#5568](https://github.com/ethereum/aleth/pull/5568) Improve rlpx handshake log messages and create new rlpx log channel.
@@ -35,6 +34,7 @@
 - Changed: [#5675](https://github.com/ethereum/aleth/pull/5675) Disconnect from peer when syncing is disabled for peer.
 - Changed: [#5676](https://github.com/ethereum/aleth/pull/5676) When receiving large batches of new block hashes, process up to 1024 hashes instead of disabling the peer.
 - Changed: [#5719](https://github.com/ethereum/aleth/pull/5719) Enable support for Visual Studio 2017 on Windows.
+- Changed: [#5713](https://github.com/ethereum/aleth/pull/5713) Propagate new blocks after PoW check rather than after import into the blockchain.
 - Removed: [#5631](https://github.com/ethereum/aleth/pull/5631) Removed PARANOID build option.
 - Fixed: [#5562](https://github.com/ethereum/aleth/pull/5562) Don't send header request messages to peers that haven't sent us Status yet.
 - Fixed: [#5581](https://github.com/ethereum/aleth/pull/5581) Fixed finding neighbour nodes in Discovery.

libethereum/Client.cpp

@@ -390,6 +390,8 @@ void Client::syncBlockQueue()
     unsigned count;
     Timer t;
 
+    // The verified blocks list needs to be a shared_ptr since we propagate them on the network
+    // thread and import them into our local chain on the client thread.
     std::shared_ptr<VerifiedBlocks> verifiedBlocks = std::make_shared<VerifiedBlocks>();
     m_bq.drain(*verifiedBlocks, m_syncAmount);
 

libethereum/EthereumCapability.cpp

@@ -436,7 +436,8 @@ void EthereumCapability::reset()
     m_sync->abortSync();
 
     // reset() can be called from RPC handling thread,
-    // but we access m_latestBlockHashSent and m_transactionsSent only from the network thread
+    // but we access m_latestBlockHashSent, m_latestBlockSent, and m_transactionsSent only from the
+    // network thread
     m_host->postWork([this]() {
         m_latestBlockHashSent = {};
         m_latestBlockSent = {};
@@ -511,14 +512,11 @@ std::vector<NodeID> EthereumCapability::randomPeers(
     std::vector<NodeID> const& _peers, size_t _count) const
 {
     std::vector<NodeID> peers{_peers};
-    if (peers.empty())
+    if (peers.empty() || _count >= _peers.size())
         return peers;
 
-    if (_count > peers.size())
-        _count = peers.size();
-    auto itEnd = peers.begin() + _count;
-    std::shuffle(peers.begin(), itEnd, m_urng);
-    return {peers.begin(), itEnd};
+    std::shuffle(peers.begin(), peers.end(), m_urng);
+    return {peers.begin(), peers.begin() + _count};
 }
 
 void EthereumCapability::maintainBlockHashes(h256 const& _currentHash)
@@ -533,7 +531,10 @@ void EthereumCapability::maintainBlockHashes(h256 const& _currentHash)
     // 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)
+    {
+        m_latestBlockHashSent = _currentHash;
         return;
+    }
 
     LOG(m_logger) << "Sending new block hashes (current is " << _currentHash << ", was "
                     << m_latestBlockHashSent << ")";
@@ -950,15 +951,18 @@ void EthereumCapability::propagateNewBlocks(std::shared_ptr<VerifiedBlocks const
     m_host->postWork([this, _newBlocks]() {
         // Verify that we're not too far behind - we perform this check on the network thread to
         // simplify the synchronization story (since otherwise we'd need to synchronize access to
-        // m_latestBlockSent)
+        // m_latestBlockSent).
         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)
             return;
 
         if (diff(detailsFrom.number, detailsTo.number) > c_maxSendNewBlocksCount)
+        {
+            m_latestBlockSent = latestHash;
             return;
+        }
 
         auto const peersWithoutBlock = selectPeers(
             [&](EthereumPeer const& _peer) { return !_peer.isBlockKnown(latestHash); });