From 72155d9f5e940125e57f77a3ba56d263a1730a03 Mon Sep 17 00:00:00 2001
From: Roy Keene <rkeene@users.noreply.github.com>
Date: Wed, 25 Jul 2018 11:12:44 -0500
Subject: [PATCH] Make "bulk_pull" with a start block inclusive of that start
 block (#985)

* Fixed a test that was relying on incorrect semantics for "bulk_pull"

* Make "bulk_pull" with a start block inclusive of that start block
---
 rai/core_test/network.cpp | 23 ++++++++++++++++++++-
 rai/node/bootstrap.cpp    | 43 ++++++++++++++++++++++++++++++++++++++-
 rai/node/bootstrap.hpp    |  1 +
 3 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/rai/core_test/network.cpp b/rai/core_test/network.cpp
index a0b4bbd4..7a2c909f 100644
--- a/rai/core_test/network.cpp
+++ b/rai/core_test/network.cpp
@@ -473,7 +473,7 @@ TEST (bulk_pull, none)
 	auto connection (std::make_shared<rai::bootstrap_server> (nullptr, system.nodes[0]));
 	rai::genesis genesis;
 	std::unique_ptr<rai::bulk_pull> req (new rai::bulk_pull{});
-	req->start = genesis.hash ();
+	req->start = rai::test_genesis_key.pub;
 	req->end = genesis.hash ();
 	connection->requests.push (std::unique_ptr<rai::message>{});
 	auto request (std::make_shared<rai::bulk_pull_server> (connection, std::move (req)));
@@ -510,6 +510,27 @@ TEST (bulk_pull, by_block)
 	auto block (request->get_next ());
 	ASSERT_NE (nullptr, block);
 	ASSERT_EQ (block->hash (), genesis.hash ());
+
+	block = request->get_next ();
+	ASSERT_EQ (nullptr, block);
+}
+
+TEST (bulk_pull, by_block_single)
+{
+	rai::system system (24000, 1);
+	auto connection (std::make_shared<rai::bootstrap_server> (nullptr, system.nodes[0]));
+	rai::genesis genesis;
+	std::unique_ptr<rai::bulk_pull> req (new rai::bulk_pull{});
+	req->start = genesis.hash ();
+	req->end = genesis.hash ();
+	connection->requests.push (std::unique_ptr<rai::message>{});
+	auto request (std::make_shared<rai::bulk_pull_server> (connection, std::move (req)));
+	auto block (request->get_next ());
+	ASSERT_NE (nullptr, block);
+	ASSERT_EQ (block->hash (), genesis.hash ());
+
+	block = request->get_next ();
+	ASSERT_EQ (nullptr, block);
 }
 
 TEST (bootstrap_processor, DISABLED_process_none)
diff --git a/rai/node/bootstrap.cpp b/rai/node/bootstrap.cpp
index 7ed85b28..b24de67f 100644
--- a/rai/node/bootstrap.cpp
+++ b/rai/node/bootstrap.cpp
@@ -1538,9 +1538,16 @@ void rai::bootstrap_server::run_next ()
  * to send.  To determine if "start" is interpretted as an account or
  * hash, the ledger is checked to see if the block specified exists,
  * if not then it is interpretted as an account.
+ *
+ * Additionally, if "start" is specified as a block hash the range
+ * is inclusive of that block hash, that is the range will be:
+ * [start, end); In the case that a block hash is not specified the
+ * range will be exclusive of the frontier for that account with
+ * a range of (frontier, end)
  */
 void rai::bulk_pull_server::set_current_end ()
 {
+	include_start = false;
 	assert (request != nullptr);
 	rai::transaction transaction (connection->node->store.environment, nullptr, false);
 	if (!connection->node->store.block_exists (transaction, request->end))
@@ -1560,6 +1567,7 @@ void rai::bulk_pull_server::set_current_end ()
 		}
 
 		current = request->start;
+		include_start = true;
 	}
 	else
 	{
@@ -1620,11 +1628,37 @@ void rai::bulk_pull_server::send_next ()
 std::unique_ptr<rai::block> rai::bulk_pull_server::get_next ()
 {
 	std::unique_ptr<rai::block> result;
+	bool send_current = false, set_current_to_end = false;
+
+	/*
+	 * Determine if we should reply with a block
+	 *
+	 * If our cursor is on the final block, we should signal that we
+	 * are done by returning a null result.
+	 *
+	 * Unless we are including the "start" member and this is the
+	 * start member, then include it anyway.
+	 */
 	if (current != request->end)
+	{
+		send_current = true;
+	}
+	else if (current == request->end && include_start == true)
+	{
+		send_current = true;
+
+		/*
+		 * We also need to ensure that the next time
+		 * are invoked that we return a null result
+		 */
+		set_current_to_end = true;
+	}
+
+	if (send_current)
 	{
 		rai::transaction transaction (connection->node->store.environment, nullptr, false);
 		result = connection->node->store.block_get (transaction, current);
-		if (result != nullptr)
+		if (result != nullptr && set_current_to_end == false)
 		{
 			auto previous (result->previous ());
 			if (!previous.is_zero ())
@@ -1641,6 +1675,13 @@ std::unique_ptr<rai::block> rai::bulk_pull_server::get_next ()
 			current = request->end;
 		}
 	}
+
+	/*
+	 * Once we have processed "get_next()" once our cursor is no longer on
+	 * the "start" member, so this flag is not relevant is always false.
+	 */
+	include_start = false;
+
 	return result;
 }
 
diff --git a/rai/node/bootstrap.hpp b/rai/node/bootstrap.hpp
index 619ce53c..882a58c7 100644
--- a/rai/node/bootstrap.hpp
+++ b/rai/node/bootstrap.hpp
@@ -245,6 +245,7 @@ public:
 	std::unique_ptr<rai::bulk_pull> request;
 	std::shared_ptr<std::vector<uint8_t>> send_buffer;
 	rai::block_hash current;
+	bool include_start;
 };
 class bulk_pull_blocks;
 class bulk_pull_blocks_server : public std::enable_shared_from_this<rai::bulk_pull_blocks_server>