From 3b2f8f21eaabd1a87fea5d82d9296fa77c1fd16d Mon Sep 17 00:00:00 2001 From: arkpar Date: Fri, 19 Jun 2015 09:49:42 +0200 Subject: [PATCH 1/3] fixed a race condition on peer aborting, style --- libethereum/BlockChainSync.cpp | 41 +++++++++++++++++++++++++--------- libethereum/BlockChainSync.h | 21 +++++++++-------- 2 files changed, 40 insertions(+), 22 deletions(-) diff --git a/libethereum/BlockChainSync.cpp b/libethereum/BlockChainSync.cpp index eb4514731..6b87ce904 100644 --- a/libethereum/BlockChainSync.cpp +++ b/libethereum/BlockChainSync.cpp @@ -52,6 +52,7 @@ BlockChainSync::BlockChainSync(EthereumHost& _host): BlockChainSync::~BlockChainSync() { + RecursiveGuard l(x_sync); abortSync(); } @@ -67,8 +68,10 @@ DownloadMan& BlockChainSync::downloadMan() void BlockChainSync::abortSync() { + DEV_INVARIANT_CHECK; host().foreachPeer([this](EthereumPeer* _p) { onPeerAborting(_p); return true; }); downloadMan().resetToChain(h256s()); + DEV_INVARIANT_CHECK; } void BlockChainSync::onPeerStatus(EthereumPeer* _peer) @@ -87,14 +90,14 @@ void BlockChainSync::onPeerStatus(EthereumPeer* _peer) _peer->disable("Peer banned for previous bad behaviour."); else { - unsigned estimatedHashes = estimateHashes(); - _peer->m_expectedHashes = estimatedHashes; + unsigned hashes = estimatedHashes(); + _peer->m_expectedHashes = hashes; onNewPeer(_peer); } DEV_INVARIANT_CHECK; } -unsigned BlockChainSync::estimateHashes() +unsigned BlockChainSync::estimatedHashes() const { BlockInfo block = host().chain().info(); time_t lastBlockTime = (block.hash() == host().chain().genesisHash()) ? 1428192000 : (time_t)block.timestamp; @@ -113,7 +116,6 @@ void BlockChainSync::requestBlocks(EthereumPeer* _peer) if (host().bq().knownFull()) { clog(NetAllDetail) << "Waiting for block queue before downloading blocks"; - m_lastActiveState = m_state; pauseSync(); _peer->setIdle(); return; @@ -233,10 +235,8 @@ void BlockChainSync::onPeerBlocks(EthereumPeer* _peer, RLP const& _r) { if (downloadMan().isComplete()) completeSync(); - else if (!got) - requestBlocks(_peer); else - peerDoneBlocks(_peer); + requestBlocks(_peer); // Some of the blocks might have been downloaded by helping peers, proceed anyway } DEV_INVARIANT_CHECK; } @@ -478,12 +478,12 @@ void PV60Sync::transition(EthereumPeer* _peer, SyncState _s, bool _force, bool _ clog(NetWarn) << "Invalid state transition:" << EthereumHost::stateName(_s) << "from" << EthereumHost::stateName(m_state) << ", " << (isSyncing(_peer) ? "syncing" : "holding") << (needsSyncing(_peer) ? "& needed" : ""); } -void PV60Sync::resetSyncFor(EthereumPeer* _peer, h256 _latestHash, u256 _td) +void PV60Sync::resetSyncFor(EthereumPeer* _peer, h256 const& _latestHash, u256 const& _td) { setNeedsSyncing(_peer, _latestHash, _td); } -void PV60Sync::setNeedsSyncing(EthereumPeer* _peer, h256 _latestHash, u256 _td) +void PV60Sync::setNeedsSyncing(EthereumPeer* _peer, h256 const& _latestHash, u256 const& _td) { _peer->m_latestHash = _latestHash; _peer->m_totalDifficulty = _td; @@ -636,7 +636,9 @@ void PV60Sync::noteDoneBlocks(EthereumPeer* _peer, bool _clemency) // m_banned.insert(_peer->session()->id()); // We know who you are! // _peer->disable("Peer sent hashes but was unable to provide the blocks."); } + resetSync(); downloadMan().reset(); + transition(_peer, SyncState::Idle); } _peer->m_sub.doneFetch(); } @@ -648,7 +650,7 @@ void PV60Sync::onPeerHashes(EthereumPeer* _peer, h256s const& _hashes) _peer->setIdle(); if (!isSyncing(_peer)) { - clog(NetMessageSummary) << "Ignoring hashes synce not syncing"; + clog(NetMessageSummary) << "Ignoring hashes since not syncing"; return; } if (_hashes.size() == 0) @@ -705,9 +707,10 @@ void PV60Sync::onPeerNewHashes(EthereumPeer* _peer, h256s const& _hashes) clog(NetMessageSummary) << "Ignoring since we're already downloading."; return; } + clog(NetMessageDetail) << "Not syncing and new block hash discovered: syncing without help."; unsigned knowns = 0; unsigned unknowns = 0; - for (auto h: _hashes) + for (auto const& h: _hashes) { _peer->addRating(1); DEV_GUARDED(_peer->x_knownBlocks) @@ -741,6 +744,7 @@ void PV60Sync::onPeerNewHashes(EthereumPeer* _peer, h256s const& _hashes) void PV60Sync::abortSync(EthereumPeer* _peer) { + // Can't check invariants here since the peers is already removed from the list and the state is not updated yet. if (isSyncing(_peer)) { host().foreachPeer([this](EthereumPeer* _p) { _p->setIdle(); return true; }); @@ -751,6 +755,8 @@ void PV60Sync::abortSync(EthereumPeer* _peer) void PV60Sync::onPeerAborting(EthereumPeer* _peer) { + RecursiveGuard l(x_sync); + // Can't check invariants here since the peers is already removed from the list and the state is not updated yet. abortSync(_peer); DEV_INVARIANT_CHECK; } @@ -767,6 +773,10 @@ bool PV60Sync::invariants() const host().foreachPeer([&](EthereumPeer* _p) { if (_p->m_asking == Asking::Hashes) hashes = true; return !hashes; }); if (!hashes) return false; + if (!m_syncingLatestHash) + return false; + if (m_syncingNeededBlocks.empty() != (!m_syncingLastReceivedHash)) + return false; } if (m_state == SyncState::Blocks || m_state == SyncState::NewBlocks) { @@ -777,5 +787,14 @@ bool PV60Sync::invariants() const if (downloadMan().isComplete()) return false; } + if (m_state == SyncState::Idle) + { + bool busy = false; + host().foreachPeer([&](EthereumPeer* _p) { if (_p->m_asking != Asking::Nothing && _p->m_asking != Asking::State) busy = true; return !busy; }); + if (busy) + return false; + } + if (m_state == SyncState::Waiting && !host().bq().isActive()) + return false; return true; } diff --git a/libethereum/BlockChainSync.h b/libethereum/BlockChainSync.h index e981a344e..f47d946e3 100644 --- a/libethereum/BlockChainSync.h +++ b/libethereum/BlockChainSync.h @@ -103,29 +103,27 @@ protected: virtual void pauseSync() = 0; /// Restart sync for given peer - virtual void resetSyncFor(EthereumPeer* _peer, h256 _latestHash, u256 _td) = 0; + virtual void resetSyncFor(EthereumPeer* _peer, h256 const& _latestHash, u256 const& _td) = 0; EthereumHost& host() { return m_host; } EthereumHost const& host() const { return m_host; } /// Estimates max number of hashes peers can give us. - unsigned estimateHashes(); + unsigned estimatedHashes() const; /// Request blocks from peer if needed void requestBlocks(EthereumPeer* _peer); + Handler m_bqRoomAvailable; ///< Triggered once block queue + mutable RecursiveMutex x_sync; + SyncState m_state = SyncState::Idle; ///< Current sync state + unsigned m_estimatedHashes = 0; ///< Number of estimated hashes for the last peer over PV60. Used for status reporting only. + private: static char const* const s_stateNames[static_cast(SyncState::Size)]; bool invariants() const override = 0; EthereumHost& m_host; HashDownloadMan m_hashMan; - -protected: - Handler m_bqRoomAvailable; - mutable RecursiveMutex x_sync; - SyncState m_state = SyncState::Idle; ///< Current sync state - SyncState m_lastActiveState = SyncState::Idle; ///< Saved state before entering waiting queue mode - unsigned m_estimatedHashes = 0; ///< Number of estimated hashes for the last peer over PV60. Used for status reporting only. }; @@ -153,13 +151,14 @@ public: /// @returns Sync status SyncStatus status() const override; +protected: void onNewPeer(EthereumPeer* _peer) override; void continueSync() override; void peerDoneBlocks(EthereumPeer* _peer) override; void restartSync() override; void completeSync() override; void pauseSync() override; - void resetSyncFor(EthereumPeer* _peer, h256 _latestHash, u256 _td) override; + void resetSyncFor(EthereumPeer* _peer, h256 const& _latestHash, u256 const& _td) override; private: /// Transition sync state in a particular direction. @param _peer Peer that is responsible for state tranfer @@ -169,7 +168,7 @@ private: void resetNeedsSyncing(EthereumPeer* _peer) { setNeedsSyncing(_peer, h256(), 0); } /// Update peer syncing requirements state. - void setNeedsSyncing(EthereumPeer* _peer, h256 _latestHash, u256 _td); + void setNeedsSyncing(EthereumPeer* _peer, h256 const& _latestHash, u256 const& _td); /// Do we presently need syncing with this peer? bool needsSyncing(EthereumPeer* _peer) const; From 61e84ea862d5799c35746387cb33edf0c1a719ac Mon Sep 17 00:00:00 2001 From: arkpar Date: Fri, 19 Jun 2015 10:20:23 +0200 Subject: [PATCH 2/3] transitions description --- libethereum/BlockChainSync.cpp | 2 +- libethereum/BlockChainSync.h | 71 ++++++++++++++++++++++++++++++++-- 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/libethereum/BlockChainSync.cpp b/libethereum/BlockChainSync.cpp index 6b87ce904..c3ea67e22 100644 --- a/libethereum/BlockChainSync.cpp +++ b/libethereum/BlockChainSync.cpp @@ -508,7 +508,7 @@ bool PV60Sync::shouldGrabBlocks(EthereumPeer* _peer) const { auto td = _peer->m_totalDifficulty; auto lh = _peer->m_latestHash; - auto ctd = host().chain().details().totalDifficulty; + auto ctd = host().chain().details().totalDifficulty; if (m_syncingNeededBlocks.empty()) return false; diff --git a/libethereum/BlockChainSync.h b/libethereum/BlockChainSync.h index f47d946e3..18e8158e6 100644 --- a/libethereum/BlockChainSync.h +++ b/libethereum/BlockChainSync.h @@ -131,6 +131,70 @@ private: * @brief Syncrhonization over PV60. Selects a single peer and tries to downloading hashes from it. After hash downaload is complete * Syncs to peers and keeps up to date */ + +/** + * Transitions: + * + * Idle->Hashes + * Triggered when: + * * A new peer appears that we can sync to + * * Transtition to Idle, there are peers we can sync to + * Effects: + * * Set chain sync (m_syncingTotalDifficulty, m_syncingLatestHash, m_syncer) + * * Requests hashes from m_syncer + * + * Hashes->Idle + * Triggered when: + * * Received too many hashes + * * Received 0 total hashes from m_syncer + * * m_syncer aborts + * Effects: + * In case of too many hashes sync is reset + * + * Hashes->Blocks + * Triggered when: + * * Received known hash from m_syncer + * * Received 0 hashes from m_syncer and m_syncingTotalBlocks not empty + * Effects: + * * Set up download manager, clear m_syncingTotalBlocks. Set all peers to help with downloading if they can + * + * Blocks->Idle + * Triggered when: + * * m_syncer aborts + * * m_syncer does not have required block + * * All blocks downloaded + * * Block qeueue is full with unknown blocks + * Effects: + * * Download manager is reset + * + * Blocks->Waiting + * Triggered when: + * * Block queue is full with known blocks + * Effects: + * * Stop requesting blocks from peers + * + * Waiting->Blocks + * Triggered when: + * * Block queue has space for new blocks + * Effects: + * * Continue requesting blocks from peers + * + * Idle->NewBlocks + * Triggered when: + * * New block hashes arrive + * Effects: + * * Set up download manager, clear m_syncingTotalBlocks. Download blocks from a single peer. If downloaded blocks have unknown parents, set the peer to sync + * + * NewBlocks->Idle + * Triggered when: + * * m_syncer aborts + * * m_syncer does not have required block + * * All new blocks downloaded + * * Block qeueue is full with unknown blocks + * Effects: + * * Download manager is reset + * + */ class PV60Sync: public BlockChainSync { public: @@ -204,9 +268,10 @@ private: h256s m_syncingNeededBlocks; ///< The blocks that we should download from this peer. h256 m_syncingLastReceivedHash; ///< Hash most recently received from peer. - h256 m_syncingLatestHash; ///< Peer's latest block's hash, as of the current sync. - u256 m_syncingTotalDifficulty; ///< Peer's latest block's total difficulty, as of the current sync. - EthereumPeer* m_syncer = nullptr; // TODO: switch to weak_ptr + h256 m_syncingLatestHash; ///< Latest block's hash of the peer we are syncing to, as of the current sync. + u256 m_syncingTotalDifficulty; ///< Latest block's total difficulty of the peer we aresyncing to, as of the current sync. + // TODO: switch to weak_ptr + EthereumPeer* m_syncer = nullptr; ///< Current }; } } From 997641c035b8dcb96784bf6fbba1a8f1d18f790b Mon Sep 17 00:00:00 2001 From: arkpar Date: Fri, 19 Jun 2015 10:52:00 +0200 Subject: [PATCH 3/3] fixed comment --- libethereum/BlockChainSync.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libethereum/BlockChainSync.h b/libethereum/BlockChainSync.h index 80372d3a2..b946f557e 100644 --- a/libethereum/BlockChainSync.h +++ b/libethereum/BlockChainSync.h @@ -272,7 +272,7 @@ private: h256 m_syncingLatestHash; ///< Latest block's hash of the peer we are syncing to, as of the current sync. u256 m_syncingTotalDifficulty; ///< Latest block's total difficulty of the peer we aresyncing to, as of the current sync. // TODO: switch to weak_ptr - EthereumPeer* m_syncer = nullptr; ///< Current + EthereumPeer* m_syncer = nullptr; ///< Peer we are currently syncing with }; } }