What the developer wrote
Authored by tobtoht
Merge pull request #11261
9cef02d cryptonote_protocol: reject conflicting span reservations (selsta)
ACKs: jpk68, plowsof
What changed, and why it matters
This change fixes a bug in how Monero nodes reserve chunks ('spans') of blocks to download from peers. Previously, two different peers could be assigned the same starting block height with different expected block hashes, causing confusion about which blocks to request and potentially wasting bandwidth or delaying sync. The fix makes the reservation step check for conflicts before committing, and returns failure if the same height is already reserved by another peer. There is no direct evidence in the commit message that this was publicly disclosed as a security vulnerability.
Treat as a correctness/robustness fix and include in routine release notes. Review whether the duplicate-reservation behavior could be exploited to partition or slow node synchronization, but no immediate emergency response is indicated by the diff alone.
Security signals we found
Conflicting peer span reservations could overwrite hash expectations
Silent duplicate reservation may cause inconsistent block download state
Fix prevents same-height span reservation by different connection IDs
Unit tests added for conflict and prefix-skip behavior
Evidence from the diff
The patch modifies block_queue::reserve_span to call add_blocks once and use its return value (std::set<…>::insert().second) to detect whether a span starting at the same height already exists. Previously, reserve_span called add_blocks unconditionally after computing span_start_height, which could silently insert a duplicate-key span with different hashes. The duplicate insertion would fail at the std::set level (same start height), but the subsequent set_span_hashes call would overwrite the hash list of the existing span, and the function would still return the reserved range to the caller. This could lead to inconsistent span ownership/hash expectations. The new unit tests verify that a second reservation at the same height with different hashes is rejected and that reservations skip already-requested prefixes instead of overlapping them.
Changed components
src/cryptonote_protocol/block_queue.cppsrc/cryptonote_protocol/block_queue.htests/unit_tests/block_queue.cppInspect captured patch +65 / −4
### src/cryptonote_protocol/block_queue.cpp
@@ -62,11 +62,11 @@ void block_queue::add_blocks(uint64_t height, std::vector<cryptonote::block_comp
}
}
-void block_queue::add_blocks(uint64_t height, uint64_t nblocks, const boost::uuids::uuid &connection_id, const epee::net_utils::network_address &addr, boost::posix_time::ptime time)
+bool block_queue::add_blocks(uint64_t height, uint64_t nblocks, const boost::uuids::uuid &connection_id, const epee::net_utils::network_address &addr, boost::posix_time::ptime time)
{
CHECK_AND_ASSERT_THROW_MES(nblocks > 0, "Empty span");
boost::unique_lock<boost::recursive_mutex> lock(mutex);
- blocks.insert(span(height, nblocks, connection_id, addr, time));
+ return blocks.insert(span(height, nblocks, connection_id, addr, time)).second;
}
void block_queue::flush_spans(const boost::uuids::uuid &connection_id, bool all)
@@ -315,8 +315,12 @@ std::pair<uint64_t, uint64_t> block_queue::reserve_span(uint64_t first_block_hei
MDEBUG("span_length 0, cannot reserve");
return std::make_pair(0, 0);
}
+ if (!add_blocks(span_start_height, span_length, connection_id, addr, time))
+ {
+ MDEBUG("Span already starts at height " << span_start_height << ", cannot reserve");
+ return std::make_pair(0, 0);
+ }
MDEBUG("Reserving span " << span_start_height << " - " << (span_start_height + span_length - 1) << " for " << connection_id);
- add_blocks(span_start_height, span_length, connection_id, addr, time);
set_span_hashes(span_start_height, connection_id, hashes);
return std::make_pair(span_start_height, span_length);
}
### src/cryptonote_protocol/block_queue.h
@@ -72,7 +72,7 @@ namespace cryptonote
public:
void add_blocks(uint64_t height, std::vector<cryptonote::block_complete_entry> bcel, const boost::uuids::uuid &connection_id, const epee::net_utils::network_address &addr, float rate, size_t size);
- void add_blocks(uint64_t height, uint64_t nblocks, const boost::uuids::uuid &connection_id, const epee::net_utils::network_address &addr, boost::posix_time::ptime time = boost::date_time::min_date_time);
+ bool add_blocks(uint64_t height, uint64_t nblocks, const boost::uuids::uuid &connection_id, const epee::net_utils::network_address &addr, 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<boost::uuids::uuid> &live_connections);
bool remove_span(uint64_t start_block_height, std::vector<crypto::hash> *hashes = NULL);
### tests/unit_tests/block_queue.cpp
@@ -107,3 +107,60 @@ TEST(block_queue, reserve_does_not_overlap_later_span)
// overlapping the span which is already reserved at height 57.
ASSERT_EQ(bq.reserve_span(1, 100, 36, uuid2(), na, false, 0, 0, 101, hashes), std::make_pair(uint64_t{29}, uint64_t{28}));
}
+
+TEST(block_queue, reserve_span_same_height_different_hashes)
+{
+ const std::vector<std::pair<crypto::hash, uint64_t>> first_hashes{
+ {crypto::rand<crypto::hash>(), 0}, {crypto::rand<crypto::hash>(), 0}};
+ const std::vector<std::pair<crypto::hash, uint64_t>> conflicting_hashes{
+ {crypto::rand<crypto::hash>(), 0}, {crypto::rand<crypto::hash>(), 0}};
+ epee::net_utils::network_address na;
+
+ for (const auto &connection_id: {uuid1(), uuid2()})
+ {
+ cryptonote::block_queue bq;
+ ASSERT_EQ(bq.reserve_span(100, 101, 2, uuid1(), na, false, 0, 0, 1000, first_hashes),
+ std::make_pair(uint64_t(100), uint64_t(2)));
+ ASSERT_EQ(bq.reserve_span(100, 101, 2, connection_id, na, false, 0, 0, 1000, conflicting_hashes),
+ std::make_pair(uint64_t(0), uint64_t(0)));
+
+ std::vector<crypto::hash> hashes;
+ boost::uuids::uuid owner;
+ boost::posix_time::ptime time;
+ ASSERT_EQ(bq.get_next_span_if_scheduled(hashes, owner, time),
+ std::make_pair(uint64_t(100), uint64_t(2)));
+ ASSERT_EQ(owner, uuid1());
+ ASSERT_EQ(hashes, (std::vector<crypto::hash>{first_hashes[0].first, first_hashes[1].first}));
+ for (const auto &entry: first_hashes)
+ ASSERT_TRUE(bq.requested(entry.first));
+ for (const auto &entry: conflicting_hashes)
+ ASSERT_FALSE(bq.requested(entry.first));
+
+ bq.flush_spans(uuid1());
+ ASSERT_EQ(bq.reserve_span(100, 101, 2, connection_id, na, false, 0, 0, 1000, conflicting_hashes),
+ std::make_pair(uint64_t(100), uint64_t(2)));
+ for (const auto &entry: first_hashes)
+ ASSERT_FALSE(bq.requested(entry.first));
+ for (const auto &entry: conflicting_hashes)
+ ASSERT_TRUE(bq.requested(entry.first));
+ }
+}
+
+TEST(block_queue, reserve_span_skips_requested_prefix)
+{
+ const std::vector<std::pair<crypto::hash, uint64_t>> hashes{
+ {crypto::rand<crypto::hash>(), 0}, {crypto::rand<crypto::hash>(), 0},
+ {crypto::rand<crypto::hash>(), 0}, {crypto::rand<crypto::hash>(), 0}};
+ cryptonote::block_queue bq;
+ epee::net_utils::network_address na;
+
+ ASSERT_EQ(bq.reserve_span(100, 103, 2, uuid1(), na, false, 0, 0, 1000, hashes),
+ std::make_pair(uint64_t(100), uint64_t(2)));
+ ASSERT_EQ(bq.reserve_span(100, 103, 2, uuid2(), na, false, 0, 0, 1000, hashes),
+ std::make_pair(uint64_t(102), uint64_t(2)));
+ bq.flush_spans(uuid1());
+ ASSERT_FALSE(bq.requested(hashes[0].first));
+ ASSERT_FALSE(bq.requested(hashes[1].first));
+ ASSERT_TRUE(bq.requested(hashes[2].first));
+ ASSERT_TRUE(bq.requested(hashes[3].first));
+}Why this scored 63/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.