From 70b8c6d77a6aaf90a6e84f3ec6f25bea3b163c40 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Fri, 18 Aug 2017 20:14:23 +0100 Subject: [PATCH] cryptonote_protocol: misc fixes to the new sync algorithm Fix sync wedge corner case: It could happen if a connection went into standby mode, while it was the one which had requested the next span, and that span was still waiting for the data, and that peer is not on the main chain. Other peers can then start asking for that data again and again, but never get it as only that forked peer does. And various other fixes --- src/cryptonote_protocol/block_queue.cpp | 28 ++++- src/cryptonote_protocol/block_queue.h | 3 +- .../cryptonote_protocol_handler.h | 2 + .../cryptonote_protocol_handler.inl | 112 ++++++++++++------ src/p2p/net_node.inl | 2 + 5 files changed, 109 insertions(+), 38 deletions(-) diff --git a/src/cryptonote_protocol/block_queue.cpp b/src/cryptonote_protocol/block_queue.cpp index 94d31404e..02a8e3ec2 100644 --- a/src/cryptonote_protocol/block_queue.cpp +++ b/src/cryptonote_protocol/block_queue.cpp @@ -52,8 +52,11 @@ namespace cryptonote void block_queue::add_blocks(uint64_t height, std::list bcel, const boost::uuids::uuid &connection_id, float rate, size_t size) { boost::unique_lock lock(mutex); - remove_span(height); + std::list hashes; + bool has_hashes = remove_span(height, &hashes); blocks.insert(span(height, std::move(bcel), connection_id, rate, size)); + if (has_hashes) + set_span_hashes(height, connection_id, hashes); } void block_queue::add_blocks(uint64_t height, uint64_t nblocks, const boost::uuids::uuid &connection_id, boost::posix_time::ptime time) @@ -92,17 +95,20 @@ void block_queue::flush_stale_spans(const std::set &live_con } } -void block_queue::remove_span(uint64_t start_block_height) +bool block_queue::remove_span(uint64_t start_block_height, std::list *hashes) { boost::unique_lock lock(mutex); for (block_map::iterator i = blocks.begin(); i != blocks.end(); ++i) { if (i->start_block_height == start_block_height) { + if (hashes) + *hashes = std::move(i->hashes); blocks.erase(i); - return; + return true; } } + return false; } void block_queue::remove_spans(const boost::uuids::uuid &connection_id, uint64_t start_block_height) @@ -278,6 +284,22 @@ bool block_queue::get_next_span(uint64_t &height, std::list lock(mutex); + if (blocks.empty()) + return false; + block_map::const_iterator i = blocks.begin(); + if (is_blockchain_placeholder(*i)) + ++i; + if (i == blocks.end()) + return false; + if (i->connection_id != connection_id) + return false; + filled = !i->blocks.empty(); + return true; +} + size_t block_queue::get_data_size() const { boost::unique_lock lock(mutex); diff --git a/src/cryptonote_protocol/block_queue.h b/src/cryptonote_protocol/block_queue.h index fa1a0f217..13d4619bf 100644 --- a/src/cryptonote_protocol/block_queue.h +++ b/src/cryptonote_protocol/block_queue.h @@ -71,7 +71,7 @@ namespace cryptonote void add_blocks(uint64_t height, uint64_t nblocks, const boost::uuids::uuid &connection_id, boost::posix_time::ptime time = boost::date_time::min_date_time); void flush_spans(const boost::uuids::uuid &connection_id, bool all = false); void flush_stale_spans(const std::set &live_connections); - void remove_span(uint64_t start_block_height); + bool remove_span(uint64_t start_block_height, std::list *hashes = NULL); void remove_spans(const boost::uuids::uuid &connection_id, uint64_t start_block_height); uint64_t get_max_block_height() const; void print() const; @@ -82,6 +82,7 @@ namespace cryptonote std::pair get_next_span_if_scheduled(std::list &hashes, boost::uuids::uuid &connection_id, boost::posix_time::ptime &time) const; void set_span_hashes(uint64_t start_height, const boost::uuids::uuid &connection_id, std::list hashes); bool get_next_span(uint64_t &height, std::list &bcel, boost::uuids::uuid &connection_id, bool filled = true) const; + bool has_next_span(const boost::uuids::uuid &connection_id, bool &filled) const; size_t get_data_size() const; size_t get_num_filled_spans_prefix() const; size_t get_num_filled_spans() const; diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.h b/src/cryptonote_protocol/cryptonote_protocol_handler.h index d94747769..d54687e6a 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler.h +++ b/src/cryptonote_protocol/cryptonote_protocol_handler.h @@ -111,6 +111,7 @@ namespace cryptonote std::list get_connections(); const block_queue &get_block_queue() const { return m_block_queue; } void stop(); + void on_connection_close(cryptonote_connection_context &context); private: //----------------- commands handlers ---------------------------------------------- int handle_notify_new_block(int command, NOTIFY_NEW_BLOCK::request& arg, cryptonote_connection_context& context); @@ -133,6 +134,7 @@ namespace cryptonote bool should_download_next_span(cryptonote_connection_context& context) const; void drop_connection(cryptonote_connection_context &context, bool add_fail, bool flush_all_spans); bool kick_idle_peers(); + int try_add_next_blocks(cryptonote_connection_context &context); t_core& m_core; diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.inl b/src/cryptonote_protocol/cryptonote_protocol_handler.inl index daefe88b7..87690f1fc 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler.inl +++ b/src/cryptonote_protocol/cryptonote_protocol_handler.inl @@ -106,6 +106,11 @@ namespace cryptonote LOG_PRINT_CCONTEXT_L2("-->>NOTIFY_REQUEST_CHAIN: m_block_ids.size()=" << r.block_ids.size() ); post_notify(r, context); } + else if(context.m_state == cryptonote_connection_context::state_standby) + { + context.m_state = cryptonote_connection_context::state_synchronizing; + try_add_next_blocks(context); + } return true; } @@ -819,8 +824,6 @@ namespace cryptonote { MLOG_P2P_MESSAGE("Received NOTIFY_RESPONSE_GET_OBJECTS (" << arg.blocks.size() << " blocks, " << arg.txs.size() << " txes)"); - bool force_next_span = false; - // calculate size of request size_t size = 0; for (const auto &element : arg.txs) size += element.size(); @@ -938,19 +941,34 @@ namespace cryptonote context.m_last_known_hash = cryptonote::get_blob_hash(arg.blocks.back().block); - if (m_core.get_test_drop_download() && m_core.get_test_drop_download_height()) { // DISCARD BLOCKS for testing + if (!m_core.get_test_drop_download() || !m_core.get_test_drop_download_height()) { // DISCARD BLOCKS for testing + return 1; + } + } - // We try to lock the sync lock. If we can, it means no other thread is - // currently adding blocks, so we do that for as long as we can from the - // block queue. Then, we go back to download. - const boost::unique_lock sync{m_sync_lock, boost::try_to_lock}; - if (!sync.owns_lock()) - { - MINFO("Failed to lock m_sync_lock, going back to download"); - goto skip; - } - MDEBUG(context << " lock m_sync_lock, adding blocks to chain..."); +skip: + try_add_next_blocks(context); + return 1; + } + template + int t_cryptonote_protocol_handler::try_add_next_blocks(cryptonote_connection_context& context) + { + bool force_next_span = false; + + { + // We try to lock the sync lock. If we can, it means no other thread is + // currently adding blocks, so we do that for as long as we can from the + // block queue. Then, we go back to download. + const boost::unique_lock sync{m_sync_lock, boost::try_to_lock}; + if (!sync.owns_lock()) + { + MINFO("Failed to lock m_sync_lock, going back to download"); + goto skip; + } + MDEBUG(context << " lock m_sync_lock, adding blocks to chain..."); + + { m_core.pause_mine(); epee::misc_utils::auto_scope_leave_caller scope_exit_handler = epee::misc_utils::create_scope_leave_handler( boost::bind(&t_core::resume_mine, &m_core)); @@ -984,21 +1002,15 @@ namespace cryptonote // - later in an alt chain // - orphan // if it was requested, then it'll be resolved later, otherwise it's an orphan - bool parent_requested = false; - m_p2p->for_each_connection([&](cryptonote_connection_context& context, nodetool::peerid_type peer_id, uint32_t support_flags)->bool{ - if (context.m_requested_objects.find(new_block.prev_id) != context.m_requested_objects.end()) - { - parent_requested = true; - return false; - } - return true; - }); + bool parent_requested = m_block_queue.requested(new_block.prev_id); if (!parent_requested) { - LOG_ERROR_CCONTEXT("Got block with unknown parent which was not requested - dropping connection"); - // in case the peer had dropped beforehand, remove the span anyway so other threads can wake up and get it - m_block_queue.remove_spans(span_connection_id, start_height); - return 1; + // this can happen if a connection was sicced onto a late span, if it did not have those blocks, + // since we don't know that at the sic time + LOG_ERROR_CCONTEXT("Got block with unknown parent which was not requested - querying block hashes"); + context.m_needed_objects.clear(); + context.m_last_response_height = 0; + goto skip; } // parent was requested, so we wait for it to be retrieved @@ -1007,6 +1019,7 @@ namespace cryptonote } const boost::posix_time::ptime start = boost::posix_time::microsec_clock::universal_time(); + context.m_last_request_time = start; m_core.prepare_handle_incoming_blocks(blocks); @@ -1108,7 +1121,7 @@ namespace cryptonote << timing_message); } } - } // if not DISCARD BLOCK + } if (should_download_next_span(context)) { @@ -1179,9 +1192,17 @@ skip: std::list hashes; boost::uuids::uuid span_connection_id; boost::posix_time::ptime request_time; - std::pair span = m_block_queue.get_next_span_if_scheduled(hashes, span_connection_id, request_time); + std::pair span; + + span = m_block_queue.get_start_gap_span(); + if (span.second > 0) + { + MDEBUG(context << " we should download it as there is a gap"); + return true; + } // if the next span is not scheduled (or there is none) + span = m_block_queue.get_next_span_if_scheduled(hashes, span_connection_id, request_time); if (span.second == 0) { // we might be in a weird case where there is a filled next span, @@ -1270,6 +1291,17 @@ skip: first = false; context.m_state = cryptonote_connection_context::state_standby; } + + // this needs doing after we went to standby, so the callback knows what to do + bool filled; + if (m_block_queue.has_next_span(context.m_connection_id, filled) && !filled) + { + MDEBUG(context << " we have the next span, and it is scheduled, resuming"); + ++context.m_callback_request_count; + m_p2p->request_callback(context); + return 1; + } + for (size_t n = 0; n < 50; ++n) { if (m_stopping) @@ -1289,9 +1321,8 @@ skip: size_t count = 0; const size_t count_limit = m_core.get_block_sync_size(m_core.get_current_blockchain_height()); std::pair span = std::make_pair(0, 0); - if (force_next_span) { - MDEBUG(context << " force_next_span is true, trying next span"); + MDEBUG(context << " checking for gap"); span = m_block_queue.get_start_gap_span(); if (span.second > 0) { @@ -1311,6 +1342,9 @@ skip: } MDEBUG(context << " we have the hashes for this gap"); } + } + if (force_next_span) + { if (span.second == 0) { std::list hashes; @@ -1360,7 +1394,12 @@ skip: for (const auto &hash: hashes) { req.blocks.push_back(hash); + ++count; context.m_requested_objects.insert(hash); + // that's atrocious O(n) wise, but this is rare + auto i = std::find(context.m_needed_objects.begin(), context.m_needed_objects.end(), hash); + if (i != context.m_needed_objects.end()) + context.m_needed_objects.erase(i); } } } @@ -1384,14 +1423,12 @@ skip: return false; } - std::list hashes; auto it = context.m_needed_objects.begin(); for (size_t n = 0; n < span.second; ++n) { req.blocks.push_back(*it); ++count; context.m_requested_objects.insert(*it); - hashes.push_back(*it); auto j = it++; context.m_needed_objects.erase(j); } @@ -1399,7 +1436,7 @@ skip: context.m_last_request_time = boost::posix_time::microsec_clock::universal_time(); LOG_PRINT_CCONTEXT_L1("-->>NOTIFY_REQUEST_GET_OBJECTS: blocks.size()=" << req.blocks.size() << ", txs.size()=" << req.txs.size() - << "requested blocks count=" << count << " / " << count_limit << " from " << span.first); + << "requested blocks count=" << count << " / " << count_limit << " from " << span.first << ", first hash " << req.blocks.front()); //epee::net_utils::network_throttle_manager::get_global_throttle_inreq().logger_handle_net("log/dr-monero/net/req-all.data", sec, get_avg_block_size()); post_notify(req, context); @@ -1582,8 +1619,15 @@ skip: { if (add_fail) m_p2p->add_host_fail(context.m_remote_address); + m_p2p->drop_connection(context); + m_block_queue.flush_spans(context.m_connection_id, flush_all_spans); + } + //------------------------------------------------------------------------------------------------------------------------ + template + void t_cryptonote_protocol_handler::on_connection_close(cryptonote_connection_context &context) + { uint64_t target = 0; m_p2p->for_each_connection([&](const connection_context& cntxt, nodetool::peerid_type peer_id, uint32_t support_flags) { if (cntxt.m_state >= cryptonote_connection_context::state_synchronizing && cntxt.m_connection_id != context.m_connection_id) @@ -1597,7 +1641,7 @@ skip: m_core.set_target_blockchain_height(target); } - m_block_queue.flush_spans(context.m_connection_id, flush_all_spans); + m_block_queue.flush_spans(context.m_connection_id, false); } //------------------------------------------------------------------------------------------------------------------------ diff --git a/src/p2p/net_node.inl b/src/p2p/net_node.inl index e179fc14f..4dd7dbf87 100644 --- a/src/p2p/net_node.inl +++ b/src/p2p/net_node.inl @@ -1795,6 +1795,8 @@ namespace nodetool m_peerlist.remove_from_peer_anchor(na); } + m_payload_handler.on_connection_close(context); + MINFO("["<< epee::net_utils::print_connection_context(context) << "] CLOSE CONNECTION"); }