From 16b5c11f7429f55155a1dc1bccc8f2180b00d09d Mon Sep 17 00:00:00 2001 From: Lee *!* Clagett Date: Tue, 8 Oct 2024 20:27:09 -0400 Subject: [PATCH] Some cleanup in span/connection_context + few more checks --- src/cryptonote_basic/connection_context.cpp | 20 +++ src/cryptonote_basic/connection_context.h | 10 +- src/cryptonote_protocol/block_queue.cpp | 16 ++- src/cryptonote_protocol/block_queue.h | 3 +- .../cryptonote_protocol_handler.h | 1 + .../cryptonote_protocol_handler.inl | 116 +++++++++++++----- 6 files changed, 131 insertions(+), 35 deletions(-) diff --git a/src/cryptonote_basic/connection_context.cpp b/src/cryptonote_basic/connection_context.cpp index 4ad5951b30..5803fa74af 100644 --- a/src/cryptonote_basic/connection_context.cpp +++ b/src/cryptonote_basic/connection_context.cpp @@ -29,6 +29,7 @@ #include "connection_context.h" +#include #include "cryptonote_protocol/cryptonote_protocol_defs.h" #include "p2p/p2p_protocol_defs.h" @@ -69,4 +70,23 @@ namespace cryptonote }; return std::numeric_limits::max(); } + + void cryptonote_connection_context::set_state_normal() + { + m_state = state_normal; + m_expected_heights_start = 0; + m_needed_objects.clear(); + m_needed_objects.shrink_to_fit(); + m_expected_heights.clear(); + m_expected_heights.shrink_to_fit(); + m_requested_objects.clear(); + } + + boost::optional cryptonote_connection_context::get_expected_hash(const uint64_t height) const + { + const auto difference = height - m_expected_heights_start; + if (height < m_expected_heights_start || m_expected_heights.size() < difference) + return boost::none; + return m_expected_heights[difference]; + } } // cryptonote diff --git a/src/cryptonote_basic/connection_context.h b/src/cryptonote_basic/connection_context.h index ebd6f55d0b..187e201b68 100644 --- a/src/cryptonote_basic/connection_context.h +++ b/src/cryptonote_basic/connection_context.h @@ -34,6 +34,7 @@ #include #include #include +#include #include "net/net_utils_base.h" #include "crypto/hash.h" @@ -42,7 +43,7 @@ namespace cryptonote struct cryptonote_connection_context: public epee::net_utils::connection_context_base { cryptonote_connection_context(): m_state(state_before_handshake), m_remote_blockchain_height(0), m_last_response_height(0), - m_last_request_time(boost::date_time::not_a_date_time), m_callback_request_count(0), + m_expected_heights_start(0), m_last_request_time(boost::date_time::not_a_date_time), m_callback_request_count(0), m_last_known_hash(crypto::null_hash), m_pruning_seed(0), m_rpc_port(0), m_rpc_credits_per_hash(0), m_anchor(false), m_score(0), m_expect_response(0), m_expect_height(0), m_num_requested(0) {} @@ -92,11 +93,18 @@ namespace cryptonote //! \return Maximum number of bytes permissible for `command`. static size_t get_max_bytes(int command) noexcept; + //! Use this instead of `m_state = state_normal`. + void set_state_normal(); + + boost::optional get_expected_hash(uint64_t height) const; + state m_state; std::vector> m_needed_objects; + std::vector m_expected_heights; std::unordered_set m_requested_objects; uint64_t m_remote_blockchain_height; uint64_t m_last_response_height; + uint64_t m_expected_heights_start; boost::posix_time::ptime m_last_request_time; copyable_atomic m_callback_request_count; //in debug purpose: problem with double callback rise crypto::hash m_last_known_hash; diff --git a/src/cryptonote_protocol/block_queue.cpp b/src/cryptonote_protocol/block_queue.cpp index 2c3e31f593..c5d37e1d08 100644 --- a/src/cryptonote_protocol/block_queue.cpp +++ b/src/cryptonote_protocol/block_queue.cpp @@ -51,10 +51,10 @@ void block_queue::add_blocks(uint64_t height, std::vector lock(mutex); + const auto elem = have_blocks.find(hash); + if (elem == have_blocks.end()) + return std::numeric_limits::max(); + return elem->second; +} + + std::pair block_queue::reserve_span(uint64_t first_block_height, uint64_t last_block_height, uint64_t max_blocks, const boost::uuids::uuid &connection_id, const epee::net_utils::network_address &addr, bool sync_pruned_blocks, uint32_t local_pruning_seed, uint32_t pruning_seed, uint64_t blockchain_height, const std::vector> &block_hashes, boost::posix_time::ptime time) { boost::unique_lock lock(mutex); diff --git a/src/cryptonote_protocol/block_queue.h b/src/cryptonote_protocol/block_queue.h index f897b53e36..97f18c7a9b 100644 --- a/src/cryptonote_protocol/block_queue.h +++ b/src/cryptonote_protocol/block_queue.h @@ -98,6 +98,7 @@ namespace cryptonote bool foreach(std::function f) const; bool requested(const crypto::hash &hash) const; bool have(const crypto::hash &hash) const; + std::uint64_t have_height(const crypto::hash &hash) const; private: void erase_block(block_map::iterator j); @@ -107,6 +108,6 @@ namespace cryptonote block_map blocks; mutable boost::recursive_mutex mutex; std::unordered_set requested_hashes; - std::unordered_set have_blocks; + std::unordered_map have_blocks; }; } diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.h b/src/cryptonote_protocol/cryptonote_protocol_handler.h index d7fe40d225..12a173ec1b 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler.h +++ b/src/cryptonote_protocol/cryptonote_protocol_handler.h @@ -158,6 +158,7 @@ namespace cryptonote bool should_ask_for_pruned_data(cryptonote_connection_context& context, uint64_t first_block_height, uint64_t nblocks, bool check_block_weights) const; void drop_connection(cryptonote_connection_context &context, bool add_fail, bool flush_all_spans); void drop_connection_with_score(cryptonote_connection_context &context, unsigned int score, bool flush_all_spans); + void drop_connection(const boost::uuids::uuid&); void drop_connections(const epee::net_utils::network_address address); bool kick_idle_peers(); bool check_standby_peers(); diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler.inl b/src/cryptonote_protocol/cryptonote_protocol_handler.inl index b4a2fda4f0..56b8cb3b80 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_handler.inl +++ b/src/cryptonote_protocol/cryptonote_protocol_handler.inl @@ -35,6 +35,7 @@ // (may contain code and/or modifications by other developers) // developer rfree: this code is caller of our new network code, and is modded; e.g. for rate limiting +#include #include #include @@ -385,7 +386,7 @@ namespace cryptonote if(m_core.have_block(hshd.top_id)) { - context.m_state = cryptonote_connection_context::state_normal; + context.set_state_normal(); if(is_inital && hshd.current_height >= target && target == m_core.get_current_blockchain_height()) on_connection_synchronized(); return true; @@ -394,7 +395,7 @@ namespace cryptonote // No chain synchronization over hidden networks (tor, i2p, etc.) if(context.m_remote_address.get_zone() != epee::net_utils::zone::public_) { - context.m_state = cryptonote_connection_context::state_normal; + context.set_state_normal(); return true; } @@ -435,7 +436,7 @@ namespace cryptonote if (m_no_sync) { - context.m_state = cryptonote_connection_context::state_normal; + context.set_state_normal(); return true; } @@ -1196,8 +1197,9 @@ namespace cryptonote block_hashes.reserve(arg.blocks.size()); const boost::posix_time::ptime now = boost::posix_time::microsec_clock::universal_time(); uint64_t start_height = std::numeric_limits::max(); + crypto::hash previous{}; cryptonote::block b; - for(const block_complete_entry& block_entry: arg.blocks) + for(std::size_t i = 0; i < arg.blocks.size(); ++i) { if (m_stopping) { @@ -1205,10 +1207,10 @@ namespace cryptonote } crypto::hash block_hash; - if(!parse_and_validate_block_from_blob(block_entry.block, b, block_hash)) + if(!parse_and_validate_block_from_blob(arg.blocks[i].block, b, block_hash)) { LOG_ERROR_CCONTEXT("sent wrong block: failed to parse and validate block: " - << epee::string_tools::buff_to_hex_nodelimer(block_entry.block) << ", dropping connection"); + << epee::string_tools::buff_to_hex_nodelimer(arg.blocks[i].block) << ", dropping connection"); drop_connection(context, false, false); ++m_sync_bad_spans_downloaded; return 1; @@ -1216,14 +1218,25 @@ namespace cryptonote if (b.miner_tx.vin.size() != 1 || b.miner_tx.vin.front().type() != typeid(txin_gen)) { LOG_ERROR_CCONTEXT("sent wrong block: block: miner tx does not have exactly one txin_gen input" - << epee::string_tools::buff_to_hex_nodelimer(block_entry.block) << ", dropping connection"); + << epee::string_tools::buff_to_hex_nodelimer(arg.blocks[i].block) << ", dropping connection"); drop_connection(context, false, false); ++m_sync_bad_spans_downloaded; return 1; } + + const auto this_height = boost::get(b.miner_tx.vin[0]).height; + if (context.get_expected_hash(this_height) != block_hash) + { + LOG_ERROR_CCONTEXT("Sent invalid chain"); + drop_connection(context, false, false); + ++m_sync_bad_spans_downloaded; + return 1; + } + + // if first block if (start_height == std::numeric_limits::max()) { - start_height = boost::get(b.miner_tx.vin[0]).height; + start_height = this_height; if (start_height > context.m_expect_height) { LOG_ERROR_CCONTEXT("sent block ahead of expected height, dropping connection"); @@ -1231,21 +1244,45 @@ namespace cryptonote ++m_sync_bad_spans_downloaded; return 1; } + + if (this_height == 0 || context.get_expected_hash(this_height - 1) != b.prev_id) + { + LOG_ERROR_CCONTEXT("Sent invalid chain"); + drop_connection(context, false, false); + ++m_sync_bad_spans_downloaded; + return 1; + } + } + else if (b.prev_id != previous) + { + LOG_ERROR_CCONTEXT("Sent invalid chain"); + drop_connection(context, false, false); + ++m_sync_bad_spans_downloaded; + return 1; + } + previous = block_hash; + + if (start_height + i != this_height) + { + LOG_ERROR_CCONTEXT("Sent invalid chain"); + drop_connection(context, false, false); + ++m_sync_bad_spans_downloaded; + return 1; } auto req_it = context.m_requested_objects.find(block_hash); if(req_it == context.m_requested_objects.end()) { - LOG_ERROR_CCONTEXT("sent wrong NOTIFY_RESPONSE_GET_OBJECTS: block with id=" << epee::string_tools::pod_to_hex(get_blob_hash(block_entry.block)) + LOG_ERROR_CCONTEXT("sent wrong NOTIFY_RESPONSE_GET_OBJECTS: block with id=" << epee::string_tools::pod_to_hex(get_blob_hash(arg.blocks[i].block)) << " wasn't requested, dropping connection"); drop_connection(context, false, false); ++m_sync_bad_spans_downloaded; return 1; } - if(b.tx_hashes.size() != block_entry.txs.size()) + if(b.tx_hashes.size() != arg.blocks[i].txs.size()) { - LOG_ERROR_CCONTEXT("sent wrong NOTIFY_RESPONSE_GET_OBJECTS: block with id=" << epee::string_tools::pod_to_hex(get_blob_hash(block_entry.block)) - << ", tx_hashes.size()=" << b.tx_hashes.size() << " mismatch with block_complete_entry.m_txs.size()=" << block_entry.txs.size() << ", dropping connection"); + LOG_ERROR_CCONTEXT("sent wrong NOTIFY_RESPONSE_GET_OBJECTS: block with id=" << epee::string_tools::pod_to_hex(get_blob_hash(arg.blocks[i].block)) + << ", tx_hashes.size()=" << b.tx_hashes.size() << " mismatch with block_complete_entry.m_txs.size()=" << arg.blocks[i].txs.size() << ", dropping connection"); drop_connection(context, false, false); ++m_sync_bad_spans_downloaded; return 1; @@ -1463,6 +1500,14 @@ namespace cryptonote bool parent_known = m_core.have_block(new_block.prev_id); if (!parent_known) { + const std::uint64_t confirmed_height = m_block_queue.have_height(new_block.prev_id); + if (confirmed_height != std::numeric_limits::max() && confirmed_height + 1 != start_height) + { + MERROR(context << "Found incorrect height for " << new_block.prev_id << " provided by " << span_connection_id); + drop_connection(span_connection_id); + return 1; + } + // it could be: // - later in the current chain // - later in an alt chain @@ -2091,7 +2136,6 @@ skip: m_block_queue.flush_stale_spans(live_connections); // if we don't need to get next span, and the block queue is full enough, wait a bit - bool start_from_current_chain = false; if (!force_next_span) { do @@ -2114,7 +2158,7 @@ skip: return false; } MDEBUG(context << "Nothing to get from this peer, and it's not ahead of us, all done"); - context.m_state = cryptonote_connection_context::state_normal; + context.set_state_normal(); if (m_core.get_current_blockchain_height() >= m_core.get_target_blockchain_height()) on_connection_synchronized(); return true; @@ -2263,7 +2307,7 @@ skip: return false; } MDEBUG(context << "Nothing to get from this peer, and it's not ahead of us, all done"); - context.m_state = cryptonote_connection_context::state_normal; + context.set_state_normal(); if (m_core.get_current_blockchain_height() >= m_core.get_target_blockchain_height()) on_connection_synchronized(); return true; @@ -2376,7 +2420,7 @@ skip: const uint64_t blockchain_height = m_core.get_current_blockchain_height(); if (std::max(blockchain_height, m_block_queue.get_next_needed_height(blockchain_height)) >= m_core.get_target_blockchain_height()) { - context.m_state = cryptonote_connection_context::state_normal; + context.set_state_normal(); MLOG_PEER_STATE("Nothing to do for now, switching to normal state"); return true; } @@ -2419,14 +2463,11 @@ skip: m_core.get_short_chain_history(r.block_ids); CHECK_AND_ASSERT_MES(!r.block_ids.empty(), false, "Short chain history is empty"); - if (!start_from_current_chain) + // we'll want to start off from where we are on that peer, which may not be added yet + if (context.m_last_known_hash != crypto::null_hash && r.block_ids.front() != context.m_last_known_hash) { - // we'll want to start off from where we are on that peer, which may not be added yet - if (context.m_last_known_hash != crypto::null_hash && r.block_ids.front() != context.m_last_known_hash) - { - context.m_expect_height = std::numeric_limits::max(); - r.block_ids.push_front(context.m_last_known_hash); - } + context.m_expect_height = std::numeric_limits::max(); + r.block_ids.push_front(context.m_last_known_hash); } handler_request_blocks_history( r.block_ids ); // change the limit(?), sleep(?) @@ -2439,7 +2480,7 @@ skip: context.m_last_request_time = boost::posix_time::microsec_clock::universal_time(); context.m_expect_response = NOTIFY_RESPONSE_CHAIN_ENTRY::ID; - MLOG_P2P_MESSAGE("-->>NOTIFY_REQUEST_CHAIN: m_block_ids.size()=" << r.block_ids.size() << ", start_from_current_chain " << start_from_current_chain); + MLOG_P2P_MESSAGE("-->>NOTIFY_REQUEST_CHAIN: m_block_ids.size()=" << r.block_ids.size()); post_notify(r, context); MLOG_PEER_STATE("requesting chain"); }else @@ -2453,7 +2494,7 @@ skip: << "\r\nm_requested_objects.size()=" << context.m_requested_objects.size() << "\r\non connection [" << epee::net_utils::print_connection_context_short(context)<< "]"); - context.m_state = cryptonote_connection_context::state_normal; + context.set_state_normal(); if (context.m_remote_blockchain_height >= m_core.get_target_blockchain_height()) { if (m_core.get_current_blockchain_height() >= m_core.get_target_blockchain_height()) @@ -2626,11 +2667,14 @@ skip: return 1; } + context.m_expected_heights_start = arg.start_height; + + context.m_expected_heights.clear(); + context.m_expected_heights.reserve(arg.m_block_ids.size()); context.m_needed_objects.clear(); context.m_needed_objects.reserve(arg.m_block_ids.size()); uint64_t added = 0; std::unordered_set blocks_found; - bool first = true; bool expect_unknown = false; for (size_t i = 0; i < arg.m_block_ids.size(); ++i) { @@ -2642,9 +2686,10 @@ skip: } int where; const bool have_block = m_core.have_block_unlocked(arg.m_block_ids[i], &where); - if (first) + if (i == 0) { - if (!have_block && !m_block_queue.requested(arg.m_block_ids[i]) && !m_block_queue.have(arg.m_block_ids[i])) + // our outgoing chainlist only has proven blocks (i.e. downloaded) + if (!have_block && m_block_queue.have_height(arg.m_block_ids[i]) != arg.start_height) { LOG_ERROR_CCONTEXT("First block hash is unknown, dropping connection"); drop_connection_with_score(context, 5, false); @@ -2653,7 +2698,7 @@ skip: if (!have_block) expect_unknown = true; } - if (!first) + if (0 < i) { // after the first, blocks may be known or unknown, but if they are known, // they should be at the same height if on the main chain @@ -2694,10 +2739,10 @@ skip: expect_unknown = true; } const uint64_t block_weight = arg.m_block_weights.empty() ? 0 : arg.m_block_weights[i]; + context.m_expected_heights.push_back(arg.m_block_ids[i]); context.m_needed_objects.push_back(std::make_pair(arg.m_block_ids[i], block_weight)); if (++added == n_use_blocks) break; - first = false; } context.m_last_response_height -= arg.m_block_ids.size() - n_use_blocks; @@ -2906,6 +2951,16 @@ skip: } //------------------------------------------------------------------------------------------------------------------------ template + void t_cryptonote_protocol_handler::drop_connection(const boost::uuids::uuid& id) + { + m_p2p->for_connection(id, [this](cryptonote_connection_context& context, nodetool::peerid_type peer_id, uint32_t f)->bool{ + // This _could be_ outside of strand, so careful on actions + drop_connection(context, true, false); + return true; + }); + } + //------------------------------------------------------------------------------------------------------------------------ + template void t_cryptonote_protocol_handler::drop_connections(const epee::net_utils::network_address address) { MWARNING("dropping connections to " << address.str()); @@ -2922,6 +2977,7 @@ skip: { m_block_queue.flush_spans(id, true); m_p2p->for_connection(id, [&](cryptonote_connection_context& context, nodetool::peerid_type peer_id, uint32_t f)->bool{ + // This _could be_ outside of strand, so careful on actions drop_connection(context, true, false); return true; });