From f7ee67a8db7aa0d835744e74706495d7472ca364 Mon Sep 17 00:00:00 2001 From: arkpar Date: Mon, 1 Jun 2015 12:43:25 +0200 Subject: [PATCH 1/4] fixed pv61+ hash downloading stalling --- libethereum/EthereumHost.cpp | 98 +++++++++++++----------------------- libethereum/EthereumHost.h | 1 - libethereum/EthereumPeer.cpp | 20 +++----- 3 files changed, 42 insertions(+), 77 deletions(-) diff --git a/libethereum/EthereumHost.cpp b/libethereum/EthereumHost.cpp index a50c1c706..899960c23 100644 --- a/libethereum/EthereumHost.cpp +++ b/libethereum/EthereumHost.cpp @@ -91,7 +91,10 @@ void EthereumHost::doWork() bool netChange = ensureInitialised(); auto h = m_chain.currentHash(); // If we've finished our initial sync (including getting all the blocks into the chain so as to reduce invalid transactions), start trading transactions & blocks - if (!isSyncing() && m_chain.isKnown(m_latestBlockSent)) + bool syncing = false; + DEV_GUARDED(x_sync) + syncing = isSyncing(); + if (syncing && m_chain.isKnown(m_latestBlockSent)) { if (m_newTransactions) { @@ -241,7 +244,6 @@ void EthereumHost::onPeerStatus(EthereumPeer* _peer) _peer->disable("Peer banned for previous bad behaviour."); else { - _peer->m_protocolVersion = EthereumHost::c_oldProtocolVersion; //force V60 for now if (_peer->m_protocolVersion != protocolVersion()) estimatePeerHashes(_peer); else if (_peer->m_latestBlockNumber > m_chain.number()) @@ -283,6 +285,7 @@ void EthereumHost::onPeerHashes(EthereumPeer* _peer, h256s const& _hashes, bool unsigned knowns = 0; unsigned unknowns = 0; h256s neededBlocks; + bool syncByNumber = !m_syncingLatestHash; for (unsigned i = 0; i < _hashes.size(); ++i) { _peer->addRating(1); @@ -290,10 +293,14 @@ void EthereumHost::onPeerHashes(EthereumPeer* _peer, h256s const& _hashes, bool auto status = m_bq.blockStatus(h); if (status == QueueStatus::Importing || status == QueueStatus::Ready || m_chain.isKnown(h)) { - clog(NetMessageSummary) << "block hash ready:" << h << ". Start blocks download..."; - m_hashes += neededBlocks; - onPeerDoneHashes(_peer, true); - return; + clog(NetMessageSummary) << "Block hash already known:" << h; + if (!syncByNumber) + { + m_hashes += neededBlocks; + clog(NetMessageSummary) << "Start blocks download..."; + onPeerDoneHashes(_peer, true); + return; + } } else if (status == QueueStatus::Bad) { @@ -308,65 +315,25 @@ void EthereumHost::onPeerHashes(EthereumPeer* _peer, h256s const& _hashes, bool } else knowns++; - m_syncingLatestHash = h; - } - m_hashes += neededBlocks; - clog(NetMessageSummary) << knowns << "knowns," << unknowns << "unknowns; now at" << m_syncingLatestHash; - if (_complete) - { - m_needSyncBlocks = true; - continueSync(_peer); + if (!syncByNumber) + m_syncingLatestHash = h; } - else if (m_hashes.size() > _peer->m_expectedHashes) + if (syncByNumber) { - _peer->disable("Too many hashes"); - m_hashes.clear(); - m_syncingLatestHash = h256(); - continueSync(); ///Try with some other peer, keep the chain + m_man.appendToChain(neededBlocks); // Append to download manager immediatelly + clog(NetMessageSummary) << knowns << "knowns," << unknowns << "unknowns"; } else - continueSync(_peer); /// Grab next hashes -} - -void EthereumHost::onPeerHashes(EthereumPeer* _peer, unsigned /*_index*/, h256s const& _hashes) -{ - Guard l(x_sync); - assert(_peer->m_asking == Asking::Nothing); - if (_hashes.empty()) { - onPeerDoneHashes(_peer, true); - return; + m_hashes += neededBlocks; // Append to local list + clog(NetMessageSummary) << knowns << "knowns," << unknowns << "unknowns; now at" << m_syncingLatestHash; } - unsigned knowns = 0; - unsigned unknowns = 0; - h256s neededBlocks; - for (unsigned i = 0; i < _hashes.size(); ++i) + if (_complete) { - _peer->addRating(1); - auto h = _hashes[i]; - auto status = m_bq.blockStatus(h); - if (status == QueueStatus::Importing || status == QueueStatus::Ready || m_chain.isKnown(h)) - { - clog(NetWarn) << "block hash already known:" << h; - } - else if (status == QueueStatus::Bad) - { - clog(NetWarn) << "block hash bad!" << h << ". Bailing..."; - _peer->setIdle(); - return; - } - else if (status == QueueStatus::Unknown) - { - unknowns++; - neededBlocks.push_back(h); - } - else - knowns++; + m_needSyncBlocks = true; + continueSync(_peer); } - m_man.appendToChain(neededBlocks); - clog(NetMessageSummary) << knowns << "knowns," << unknowns << "unknowns"; - - if (m_hashMan.isComplete()) + else if (syncByNumber && m_hashMan.isComplete()) { // Done our chain-get. m_needSyncHashes = false; @@ -376,8 +343,15 @@ void EthereumHost::onPeerHashes(EthereumPeer* _peer, unsigned /*_index*/, h256s m_hashMan.reset(m_chain.number() + 1); continueSync(); } + else if (m_hashes.size() > _peer->m_expectedHashes) + { + _peer->disable("Too many hashes"); + m_hashes.clear(); + m_syncingLatestHash = h256(); + continueSync(); ///Try with some other peer, keep the chain + } else - continueSync(_peer); + continueSync(_peer); /// Grab next hashes } void EthereumHost::onPeerDoneHashes(EthereumPeer* _peer, bool _localChain) @@ -470,8 +444,7 @@ void EthereumHost::onPeerBlocks(EthereumPeer* _peer, RLP const& _r) void EthereumHost::onPeerNewHashes(EthereumPeer* _peer, h256s const& _hashes) { - Guard l(x_sync); - if (_peer->m_asking != Asking::Nothing) + if (isSyncing()) { clog(NetMessageSummary) << "Ignoring new hashes since we're already downloading."; return; @@ -483,7 +456,7 @@ void EthereumHost::onPeerNewHashes(EthereumPeer* _peer, h256s const& _hashes) void EthereumHost::onPeerNewBlock(EthereumPeer* _peer, RLP const& _r) { Guard l(x_sync); - if (_peer->m_asking != Asking::Nothing) + if (isSyncing()) { clog(NetMessageSummary) << "Ignoring new blocks since we're already downloading."; return; @@ -525,7 +498,7 @@ void EthereumHost::onPeerNewBlock(EthereumPeer* _peer, RLP const& _r) _peer->m_totalDifficulty = difficulty; m_needSyncHashes = true; m_needSyncBlocks = true; - m_syncingLatestHash = _peer->m_latestHash; + m_syncingLatestHash = h; sync = true; } } @@ -648,7 +621,6 @@ bool EthereumHost::peerShouldGrabChain(EthereumPeer* _peer) const bool EthereumHost::isSyncing() const { - Guard l(x_sync); bool syncing = false; forEachPeer([&](EthereumPeer* _p) { diff --git a/libethereum/EthereumHost.h b/libethereum/EthereumHost.h index 497255034..f8fa79a15 100644 --- a/libethereum/EthereumHost.h +++ b/libethereum/EthereumHost.h @@ -82,7 +82,6 @@ public: void onPeerNewBlock(EthereumPeer* _peer, RLP const& _r); ///< Called by peer once it has new blocks void onPeerNewHashes(EthereumPeer* _peer, h256s const& _hashes); ///< Called by peer once it has new hashes void onPeerHashes(EthereumPeer* _peer, h256s const& _hashes); ///< Called by peer once it has another sequential block of hashes during sync - void onPeerHashes(EthereumPeer* _peer, unsigned _index, h256s const& _hashes); ///< Called by peer once it has a new ordered block of hashes starting with a particular number void onPeerTransactions(EthereumPeer* _peer, RLP const& _r); ///< Called by peer when it has new transactions DownloadMan& downloadMan() { return m_man; } diff --git a/libethereum/EthereumPeer.cpp b/libethereum/EthereumPeer.cpp index aa979e4b8..d6b0b50c3 100644 --- a/libethereum/EthereumPeer.cpp +++ b/libethereum/EthereumPeer.cpp @@ -40,7 +40,6 @@ EthereumPeer::EthereumPeer(Session* _s, HostCapabilityFace* _h, unsigned _i, Cap m_hashSub(host()->hashDownloadMan()), m_peerCapabilityVersion(_cap.second) { - m_peerCapabilityVersion = EthereumHost::c_oldProtocolVersion; m_syncHashNumber = host()->chain().number() + 1; requestStatus(); } @@ -78,7 +77,6 @@ string toString(Asking _a) return "?"; } - void EthereumPeer::setIdle() { m_sub.doneFetch(); @@ -88,8 +86,7 @@ void EthereumPeer::setIdle() void EthereumPeer::requestStatus() { - if (m_asking != Asking::Nothing) - clog(NetWarn) << "Bad state: requesting state should be the first action"; + assert(m_asking == Asking::Nothing); setAsking(Asking::State); RLPStream s; bool latest = m_peerCapabilityVersion == host()->protocolVersion(); @@ -106,22 +103,22 @@ void EthereumPeer::requestStatus() void EthereumPeer::requestHashes() { - if (m_asking == Asking::Blocks) - return; + assert(m_asking == Asking::Nothing); m_syncHashNumber = m_hashSub.nextFetch(c_maxHashesAsk); setAsking(Asking::Hashes); RLPStream s; prep(s, GetBlockHashesByNumberPacket, 2) << m_syncHashNumber << c_maxHashesAsk; + clog(NetMessageDetail) << "Requesting block hashes for numbers " << m_syncHashNumber << "-" << m_syncHashNumber + c_maxHashesAsk - 1; sealAndSend(s); } void EthereumPeer::requestHashes(h256 const& _lastHash) { - if (m_asking == Asking::Blocks) - return; + assert(m_asking == Asking::Nothing); setAsking(Asking::Hashes); RLPStream s; prep(s, GetBlockHashesPacket, 2) << _lastHash << c_maxHashesAsk; + clog(NetMessageDetail) << "Requesting block hashes staring from " << _lastHash; sealAndSend(s); } @@ -212,7 +209,7 @@ bool EthereumPeer::interpret(unsigned _id, RLP const& _r) u256 number256 = _r[0].toInt(); unsigned number = (unsigned) number256; unsigned limit = _r[1].toInt(); - clog(NetMessageSummary) << "GetBlockHashesByNumber (" << number << "-" << number + limit << ")"; + clog(NetMessageSummary) << "GetBlockHashesByNumber (" << number << "-" << number + limit - 1 << ")"; RLPStream s; if (number <= host()->chain().number()) { @@ -248,11 +245,8 @@ bool EthereumPeer::interpret(unsigned _id, RLP const& _r) m_hashSub.noteHash(m_syncHashNumber + i, 1); } - if (m_protocolVersion == host()->protocolVersion()) - host()->onPeerHashes(this, m_syncHashNumber, hashes); // V61+, report hashes by number - else - host()->onPeerHashes(this, hashes); m_syncHashNumber += itemCount; + host()->onPeerHashes(this, hashes); break; } case GetBlocksPacket: From 49d753b3027ddccb29358fa6bb280f6e8ad56c20 Mon Sep 17 00:00:00 2001 From: arkpar Date: Tue, 2 Jun 2015 12:37:45 +0200 Subject: [PATCH 2/4] fixed isSyncing usage --- libethereum/EthereumHost.cpp | 12 +++++------- libethereum/EthereumHost.h | 4 ++-- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/libethereum/EthereumHost.cpp b/libethereum/EthereumHost.cpp index 899960c23..d90bf57e2 100644 --- a/libethereum/EthereumHost.cpp +++ b/libethereum/EthereumHost.cpp @@ -91,10 +91,7 @@ void EthereumHost::doWork() bool netChange = ensureInitialised(); auto h = m_chain.currentHash(); // If we've finished our initial sync (including getting all the blocks into the chain so as to reduce invalid transactions), start trading transactions & blocks - bool syncing = false; - DEV_GUARDED(x_sync) - syncing = isSyncing(); - if (syncing && m_chain.isKnown(m_latestBlockSent)) + if (isSyncing() && m_chain.isKnown(m_latestBlockSent)) { if (m_newTransactions) { @@ -444,7 +441,8 @@ void EthereumHost::onPeerBlocks(EthereumPeer* _peer, RLP const& _r) void EthereumHost::onPeerNewHashes(EthereumPeer* _peer, h256s const& _hashes) { - if (isSyncing()) + Guard l(x_sync); + if (isSyncingInternal()) { clog(NetMessageSummary) << "Ignoring new hashes since we're already downloading."; return; @@ -456,7 +454,7 @@ void EthereumHost::onPeerNewHashes(EthereumPeer* _peer, h256s const& _hashes) void EthereumHost::onPeerNewBlock(EthereumPeer* _peer, RLP const& _r) { Guard l(x_sync); - if (isSyncing()) + if (isSyncingInternal()) { clog(NetMessageSummary) << "Ignoring new blocks since we're already downloading."; return; @@ -619,7 +617,7 @@ bool EthereumHost::peerShouldGrabChain(EthereumPeer* _peer) const } } -bool EthereumHost::isSyncing() const +bool EthereumHost::isSyncingInternal() const { bool syncing = false; forEachPeer([&](EthereumPeer* _p) diff --git a/libethereum/EthereumHost.h b/libethereum/EthereumHost.h index f8fa79a15..ad18bccc7 100644 --- a/libethereum/EthereumHost.h +++ b/libethereum/EthereumHost.h @@ -70,8 +70,7 @@ public: void reset(); DownloadMan const& downloadMan() const { return m_man; } - bool isSyncing() const; - + bool isSyncing() const { Guard l(x_sync); return isSyncingInternal(); } bool isBanned(p2p::NodeId _id) const { return !!m_banned.count(_id); } void noteNewTransactions() { m_newTransactions = true; } @@ -93,6 +92,7 @@ private: std::pair>, std::vector>> randomSelection(unsigned _percent = 25, std::function const& _allow = [](EthereumPeer const*){ return true; }); void forEachPeerPtr(std::function)> const& _f) const; void forEachPeer(std::function const& _f) const; + bool isSyncingInternal() const; /// Sync with the BlockChain. It might contain one of our mined blocks, we might have new candidates from the network. void doWork(); From fdeadf93305f0eb9ceb303d6b221cc7980821ead Mon Sep 17 00:00:00 2001 From: arkpar Date: Tue, 2 Jun 2015 13:41:20 +0200 Subject: [PATCH 3/4] renamed isSyncingInternal --- libethereum/EthereumHost.cpp | 6 +++--- libethereum/EthereumHost.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/libethereum/EthereumHost.cpp b/libethereum/EthereumHost.cpp index d90bf57e2..ddb6bdc52 100644 --- a/libethereum/EthereumHost.cpp +++ b/libethereum/EthereumHost.cpp @@ -442,7 +442,7 @@ void EthereumHost::onPeerBlocks(EthereumPeer* _peer, RLP const& _r) void EthereumHost::onPeerNewHashes(EthereumPeer* _peer, h256s const& _hashes) { Guard l(x_sync); - if (isSyncingInternal()) + if (isSyncing_UNSAFE()) { clog(NetMessageSummary) << "Ignoring new hashes since we're already downloading."; return; @@ -454,7 +454,7 @@ void EthereumHost::onPeerNewHashes(EthereumPeer* _peer, h256s const& _hashes) void EthereumHost::onPeerNewBlock(EthereumPeer* _peer, RLP const& _r) { Guard l(x_sync); - if (isSyncingInternal()) + if (isSyncing_UNSAFE()) { clog(NetMessageSummary) << "Ignoring new blocks since we're already downloading."; return; @@ -617,7 +617,7 @@ bool EthereumHost::peerShouldGrabChain(EthereumPeer* _peer) const } } -bool EthereumHost::isSyncingInternal() const +bool EthereumHost::isSyncing_UNSAFE() const { bool syncing = false; forEachPeer([&](EthereumPeer* _p) diff --git a/libethereum/EthereumHost.h b/libethereum/EthereumHost.h index ad18bccc7..a8ae6dd66 100644 --- a/libethereum/EthereumHost.h +++ b/libethereum/EthereumHost.h @@ -70,7 +70,7 @@ public: void reset(); DownloadMan const& downloadMan() const { return m_man; } - bool isSyncing() const { Guard l(x_sync); return isSyncingInternal(); } + bool isSyncing() const { Guard l(x_sync); return isSyncing_UNSAFE(); } bool isBanned(p2p::NodeId _id) const { return !!m_banned.count(_id); } void noteNewTransactions() { m_newTransactions = true; } @@ -92,7 +92,7 @@ private: std::pair>, std::vector>> randomSelection(unsigned _percent = 25, std::function const& _allow = [](EthereumPeer const*){ return true; }); void forEachPeerPtr(std::function)> const& _f) const; void forEachPeer(std::function const& _f) const; - bool isSyncingInternal() const; + bool isSyncing_UNSAFE() const; /// Sync with the BlockChain. It might contain one of our mined blocks, we might have new candidates from the network. void doWork(); From d3b42295ee1e6bb079ae70993eb1deb2bb5c76cd Mon Sep 17 00:00:00 2001 From: arkpar Date: Tue, 2 Jun 2015 13:47:36 +0200 Subject: [PATCH 4/4] comments for isSyncing --- libethereum/EthereumHost.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libethereum/EthereumHost.cpp b/libethereum/EthereumHost.cpp index ddb6bdc52..564e37315 100644 --- a/libethereum/EthereumHost.cpp +++ b/libethereum/EthereumHost.cpp @@ -619,6 +619,8 @@ bool EthereumHost::peerShouldGrabChain(EthereumPeer* _peer) const bool EthereumHost::isSyncing_UNSAFE() const { + /// We need actual peer information here to handle the case when we are the first ever peer on the network to mine. + /// I.e. on a new private network the first node mining has noone to sync with and should start block propogation immediately. bool syncing = false; forEachPeer([&](EthereumPeer* _p) {