From 6dbb2ed27b15871b50a2a62cc8044a0bc1961d53 Mon Sep 17 00:00:00 2001 From: arkpar Date: Sun, 19 Jul 2015 13:45:22 +0200 Subject: [PATCH 1/6] Minor blockchain sync fixes --- libethereum/BlockChainSync.cpp | 33 +++++++++++++++++++++------------ libethereum/BlockChainSync.h | 1 + libethereum/EthereumPeer.cpp | 4 ++-- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/libethereum/BlockChainSync.cpp b/libethereum/BlockChainSync.cpp index 64d1bb197..0c8f9cc68 100644 --- a/libethereum/BlockChainSync.cpp +++ b/libethereum/BlockChainSync.cpp @@ -300,7 +300,7 @@ void BlockChainSync::onPeerNewBlock(std::shared_ptr _peer, RLP con u256 totalDifficulty = _r[1].toInt(); if (totalDifficulty > _peer->m_totalDifficulty) { - clog(NetMessageDetail) << "Received block with no known parent. Resyncing..."; + clog(NetMessageDetail) << "Received block with no known parent. Peer needs syncing..."; resetSyncFor(_peer, h, totalDifficulty); } break; @@ -649,11 +649,8 @@ void PV60Sync::noteDoneBlocks(std::shared_ptr _peer, bool _clemenc else { // Done our chain-get. - clog(NetWarn) << "Chain download failed. Peer with blocks didn't have them all. This peer is bad and should be punished."; - clog(NetWarn) << downloadMan().remaining(); - clog(NetWarn) << "WOULD BAN."; -// m_banned.insert(_peer->session()->id()); // We know who you are! -// _peer->disable("Peer sent hashes but was unable to provide the blocks."); + clog(NetNote) << "Peer does not have required blocks"; + resetNeedsSyncing(_peer); } resetSync(); downloadMan().reset(); @@ -747,7 +744,7 @@ void PV60Sync::onPeerNewHashes(std::shared_ptr _peer, h256s const& RecursiveGuard l(x_sync); if (isSyncing() && (m_state != SyncState::NewBlocks || isSyncing(_peer))) { - clog(NetMessageSummary) << "Ignoring since we're already downloading."; + clog(NetMessageDetail) << "Ignoring new hashes since we're already downloading."; return; } clog(NetMessageDetail) << "Not syncing and new block hash discovered: syncing without help."; @@ -838,7 +835,7 @@ void PV60Sync::onPeerAborting() // Can't check invariants here since the peers is already removed from the list and the state is not updated yet. if (m_syncer.expired() && m_state != SyncState::Idle) { - clog(NetWarn) << "Syncing peer disconnected, restarting sync"; + clog(NetNote) << "Syncing peer disconnected"; m_syncer.reset(); abortSync(); } @@ -971,9 +968,6 @@ void PV61Sync::completeSubchain(std::shared_ptr _peer, unsigned _n m_syncingNeededBlocks.clear(); for (auto h = m_completeChainMap.rbegin(); h != m_completeChainMap.rend(); ++h) m_syncingNeededBlocks.insert(m_syncingNeededBlocks.end(), h->second.hashes.begin(), h->second.hashes.end()); - m_completeChainMap.clear(); - m_knownHashes.clear(); - m_syncingBlockNumber = 0; transition(syncer, SyncState::Blocks); } else @@ -1007,6 +1001,7 @@ void PV61Sync::onPeerHashes(std::shared_ptr _peer, h256s const& _h { // End of hash chain, add last chunk to download m_readyChainMap.insert(make_pair(m_syncingBlockNumber, SubChain{ h256s{ _peer->m_latestHash }, _peer->m_latestHash })); + m_knownHashes.insert(_peer->m_latestHash); m_hashScanComplete = true; _peer->m_syncHashNumber = 0; requestSubchain(_peer); @@ -1032,7 +1027,13 @@ void PV61Sync::onPeerHashes(std::shared_ptr _peer, h256s const& _h if (isSyncing(_peer) && _peer->m_syncHashNumber == m_syncingBlockNumber) { // Got new subchain marker - assert(_hashes.size() == 1); + if(_hashes.size() != 1) + { + clog(NetWarn) << "Peers gives to much hashes"; + _peer->disable("Too many hashes"); + restartSync(); + return; + } m_knownHashes.insert(_hashes[0]); m_readyChainMap.insert(make_pair(m_syncingBlockNumber, SubChain{ h256s{ _hashes[0] }, _hashes[0] })); if ((m_readyChainMap.size() + m_downloadingChainMap.size() + m_completeChainMap.size()) * c_hashSubchainSize > _peer->m_expectedHashes) @@ -1186,6 +1187,14 @@ bool PV61Sync::isPV61Syncing() const return m_syncingBlockNumber != 0; } +void PV61Sync::completeSync() +{ + m_completeChainMap.clear(); + m_knownHashes.clear(); + m_syncingBlockNumber = 0; + PV60Sync::completeSync(); +} + bool PV61Sync::invariants() const { if (m_state == SyncState::Hashes) diff --git a/libethereum/BlockChainSync.h b/libethereum/BlockChainSync.h index 23c122bff..4e328d5ba 100644 --- a/libethereum/BlockChainSync.h +++ b/libethereum/BlockChainSync.h @@ -292,6 +292,7 @@ public: protected: void restartSync() override; + void completeSync() override; void requestSubchain(std::shared_ptr _peer) override; void syncHashes(std::shared_ptr _peer) override; void onPeerHashes(std::shared_ptr _peer, h256s const& _hashes) override; diff --git a/libethereum/EthereumPeer.cpp b/libethereum/EthereumPeer.cpp index 3b7a72b63..9a2aa3c64 100644 --- a/libethereum/EthereumPeer.cpp +++ b/libethereum/EthereumPeer.cpp @@ -145,7 +145,7 @@ void EthereumPeer::requestHashes(u256 _number, unsigned _count) setAsking(Asking::Hashes); RLPStream s; prep(s, GetBlockHashesByNumberPacket, 2) << m_syncHashNumber << _count; - clog(NetMessageDetail) << "Requesting block hashes for numbers " << m_syncHashNumber << "-" << m_syncHashNumber + c_maxHashesAsk - 1; + clog(NetMessageDetail) << "Requesting block hashes for numbers " << m_syncHashNumber << "-" << m_syncHashNumber + _count - 1; sealAndSend(s); } @@ -298,7 +298,7 @@ bool EthereumPeer::interpret(unsigned _id, RLP const& _r) if (m_asking != Asking::Hashes) { - clog(NetWarn) << "Peer giving us hashes when we didn't ask for them."; + clog(NetAllDetail) << "Peer giving us hashes when we didn't ask for them."; break; } setIdle(); From 03216e8749041c1f97966ae33248828e295bdf26 Mon Sep 17 00:00:00 2001 From: arkpar Date: Sun, 19 Jul 2015 23:03:37 +0200 Subject: [PATCH 2/6] prevent re-downloading of already queued blocks --- libethereum/BlockChainSync.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/libethereum/BlockChainSync.cpp b/libethereum/BlockChainSync.cpp index 0c8f9cc68..6cf84b588 100644 --- a/libethereum/BlockChainSync.cpp +++ b/libethereum/BlockChainSync.cpp @@ -967,7 +967,15 @@ void PV61Sync::completeSubchain(std::shared_ptr _peer, unsigned _n //Done chain-get m_syncingNeededBlocks.clear(); for (auto h = m_completeChainMap.rbegin(); h != m_completeChainMap.rend(); ++h) - m_syncingNeededBlocks.insert(m_syncingNeededBlocks.end(), h->second.hashes.begin(), h->second.hashes.end()); + if (!host().chain().isKnown(h->second.hashes.front()) && !host().chain().isKnown(h->second.hashes.back())) + { + if (host().bq().blockStatus(h->second.hashes.front()) == QueueStatus::Unknown || host().bq().blockStatus(h->second.hashes.back()) == QueueStatus::Unknown) + m_syncingNeededBlocks.insert(m_syncingNeededBlocks.end(), h->second.hashes.begin(), h->second.hashes.end()); + else + for (h256 const& hash: h->second.hashes) + if (!host().chain().isKnown(hash) && host().bq().blockStatus(hash) == QueueStatus::Unknown) + m_syncingNeededBlocks.insert(m_syncingNeededBlocks.end(), hash); + } transition(syncer, SyncState::Blocks); } else From de047149534b667dee51f3ed58baee08577232f7 Mon Sep 17 00:00:00 2001 From: arkpar Date: Sun, 19 Jul 2015 23:36:57 +0200 Subject: [PATCH 3/6] comments --- libethereum/BlockChainSync.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libethereum/BlockChainSync.cpp b/libethereum/BlockChainSync.cpp index 6cf84b588..f0c183254 100644 --- a/libethereum/BlockChainSync.cpp +++ b/libethereum/BlockChainSync.cpp @@ -648,7 +648,8 @@ void PV60Sync::noteDoneBlocks(std::shared_ptr _peer, bool _clemenc clog(NetNote) << "Chain download failed. Aborted while incomplete."; else { - // Done our chain-get. + // This can happen when the leading peer aborts and the one that is selected instead does not have all the blocks. + // Just stop syncing to this peer. Sync will restart if there are no more peers to sync with. clog(NetNote) << "Peer does not have required blocks"; resetNeedsSyncing(_peer); } @@ -966,6 +967,7 @@ void PV61Sync::completeSubchain(std::shared_ptr _peer, unsigned _n { //Done chain-get m_syncingNeededBlocks.clear(); + // Add hashes to download skipping onces that are already downloaded for (auto h = m_completeChainMap.rbegin(); h != m_completeChainMap.rend(); ++h) if (!host().chain().isKnown(h->second.hashes.front()) && !host().chain().isKnown(h->second.hashes.back())) { From 2daad53d8c78c78b0336297effbd83c3e4ed7205 Mon Sep 17 00:00:00 2001 From: arkpar Date: Sun, 19 Jul 2015 23:37:39 +0200 Subject: [PATCH 4/6] Fixed a potential deadlock when peer is released in forEachPeer --- libethereum/EthereumHost.cpp | 20 ++++++++++---------- libethereum/EthereumHost.h | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/libethereum/EthereumHost.cpp b/libethereum/EthereumHost.cpp index f8ad742f8..cd5073b10 100644 --- a/libethereum/EthereumHost.cpp +++ b/libethereum/EthereumHost.cpp @@ -84,7 +84,7 @@ bool EthereumHost::ensureInitialised() void EthereumHost::reset() { - Guard l(x_sync); + RecursiveGuard l(x_sync); if (m_sync) m_sync->abortSync(); m_sync.reset(); @@ -118,7 +118,7 @@ void EthereumHost::doWork() if (m_syncStart) { - DEV_GUARDED(x_sync) + DEV_RECURSIVE_GUARDED(x_sync) if (!m_sync) { time_t now = std::chrono::system_clock::to_time_t(chrono::system_clock::now()); @@ -288,35 +288,35 @@ BlockChainSync* EthereumHost::sync() void EthereumHost::onPeerStatus(std::shared_ptr _peer) { - Guard l(x_sync); + RecursiveGuard l(x_sync); if (sync()) sync()->onPeerStatus(_peer); } void EthereumHost::onPeerHashes(std::shared_ptr _peer, h256s const& _hashes) { - Guard l(x_sync); + RecursiveGuard l(x_sync); if (sync()) sync()->onPeerHashes(_peer, _hashes); } void EthereumHost::onPeerBlocks(std::shared_ptr _peer, RLP const& _r) { - Guard l(x_sync); + RecursiveGuard l(x_sync); if (sync()) sync()->onPeerBlocks(_peer, _r); } void EthereumHost::onPeerNewHashes(std::shared_ptr _peer, h256s const& _hashes) { - Guard l(x_sync); + RecursiveGuard l(x_sync); if (sync()) sync()->onPeerNewHashes(_peer, _hashes); } void EthereumHost::onPeerNewBlock(std::shared_ptr _peer, RLP const& _r) { - Guard l(x_sync); + RecursiveGuard l(x_sync); if (sync()) sync()->onPeerNewBlock(_peer, _r); } @@ -335,7 +335,7 @@ void EthereumHost::onPeerTransactions(std::shared_ptr _peer, RLP c void EthereumHost::onPeerAborting() { - Guard l(x_sync); + RecursiveGuard l(x_sync); try { if (m_sync) @@ -349,7 +349,7 @@ void EthereumHost::onPeerAborting() bool EthereumHost::isSyncing() const { - Guard l(x_sync); + RecursiveGuard l(x_sync); if (!m_sync) return false; return m_sync->isSyncing(); @@ -357,7 +357,7 @@ bool EthereumHost::isSyncing() const SyncStatus EthereumHost::status() const { - Guard l(x_sync); + RecursiveGuard l(x_sync); if (!m_sync) return SyncStatus(); return m_sync->status(); diff --git a/libethereum/EthereumHost.h b/libethereum/EthereumHost.h index 9afaa8413..34f3c1948 100644 --- a/libethereum/EthereumHost.h +++ b/libethereum/EthereumHost.h @@ -134,7 +134,7 @@ private: bool m_newTransactions = false; bool m_newBlocks = false; - mutable Mutex x_sync; + mutable RecursiveMutex x_sync; mutable Mutex x_transactions; DownloadMan m_man; std::unique_ptr m_sync; From 7f696850b4164da703cd005a4981a78394caeac6 Mon Sep 17 00:00:00 2001 From: arkpar Date: Mon, 20 Jul 2015 13:07:31 +0200 Subject: [PATCH 5/6] get transactions from syncing peer --- libethereum/EthereumHost.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/libethereum/EthereumHost.cpp b/libethereum/EthereumHost.cpp index cd5073b10..d973c4801 100644 --- a/libethereum/EthereumHost.cpp +++ b/libethereum/EthereumHost.cpp @@ -323,11 +323,6 @@ void EthereumHost::onPeerNewBlock(std::shared_ptr _peer, RLP const void EthereumHost::onPeerTransactions(std::shared_ptr _peer, RLP const& _r) { - if (_peer->isCriticalSyncing()) - { - clog(EthereumHostTrace) << "Ignoring transaction from peer we are syncing with"; - return; - } unsigned itemCount = _r.itemCount(); clog(EthereumHostTrace) << "Transactions (" << dec << itemCount << "entries)"; m_tq.enqueue(_r, _peer->session()->id()); From f8869313c845d607e0e7096f1647add9007e17a8 Mon Sep 17 00:00:00 2001 From: arkpar Date: Tue, 21 Jul 2015 11:19:43 +0200 Subject: [PATCH 6/6] style --- libethereum/BlockChainSync.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libethereum/BlockChainSync.cpp b/libethereum/BlockChainSync.cpp index f0c183254..a665405c7 100644 --- a/libethereum/BlockChainSync.cpp +++ b/libethereum/BlockChainSync.cpp @@ -1037,9 +1037,9 @@ void PV61Sync::onPeerHashes(std::shared_ptr _peer, h256s const& _h if (isSyncing(_peer) && _peer->m_syncHashNumber == m_syncingBlockNumber) { // Got new subchain marker - if(_hashes.size() != 1) + if (_hashes.size() != 1) { - clog(NetWarn) << "Peers gives to much hashes"; + clog(NetWarn) << "Peer sent too many hashes"; _peer->disable("Too many hashes"); restartSync(); return;