Merge pull request #4779 from pwojcikdev/fix-epoch-conflict-confirm

Fix `epoch_conflict_confirm` test

nano/core_test/block_store.cpp

@@ -1630,11 +1630,8 @@ TEST (block_store, final_vote)
 		ASSERT_EQ (store->final_vote.count (transaction), 0);
 		store->final_vote.put (transaction, qualified_root, nano::block_hash (2));
 		ASSERT_EQ (store->final_vote.count (transaction), 1);
-		// Clearing with incorrect root shouldn't remove
-		store->final_vote.clear (transaction, qualified_root.previous ());
-		ASSERT_EQ (store->final_vote.count (transaction), 1);
 		// Clearing with correct root should remove
-		store->final_vote.clear (transaction, qualified_root.root ());
+		store->final_vote.del (transaction, qualified_root);
 		ASSERT_EQ (store->final_vote.count (transaction), 0);
 	}
 }

nano/core_test/ledger.cpp

@@ -5482,8 +5482,8 @@ TEST (ledger, migrate_lmdb_to_rocksdb)
 	ASSERT_FALSE (rocksdb_store.confirmation_height.get (rocksdb_transaction, nano::dev::genesis_key.pub, confirmation_height_info));
 	ASSERT_EQ (confirmation_height_info.height, 2);
 	ASSERT_EQ (confirmation_height_info.frontier, send->hash ());
-	ASSERT_EQ (rocksdb_store.final_vote.get (rocksdb_transaction, nano::root (send->previous ())).size (), 1);
-	ASSERT_EQ (rocksdb_store.final_vote.get (rocksdb_transaction, nano::root (send->previous ()))[0], nano::block_hash (2));
+	ASSERT_TRUE (rocksdb_store.final_vote.get (rocksdb_transaction, send->qualified_root ()).has_value ());
+	ASSERT_EQ (rocksdb_store.final_vote.get (rocksdb_transaction, send->qualified_root ()).value (), nano::block_hash (2));
 
 	// Retry migration while rocksdb folder is still present
 	auto error_on_retry = ledger.migrate_lmdb_to_rocksdb (path);

nano/core_test/node.cpp

@@ -2157,6 +2157,13 @@ TEST (node, epoch_conflict_confirm)
 	nano::keypair key;
 	nano::keypair epoch_signer (nano::dev::genesis_key);
 	nano::state_block_builder builder;
+
+	// Node 1 is the voting node
+	// Send sends to an account we control: send -> open -> change
+	// Send2 sends to an account with public key of the open block
+	// Epoch open qualified root: (open, 0) on account with the same public key as the hash of the open block
+	// Epoch open and change have the same root!
+
 	auto send = builder.make_block ()
 				.account (nano::dev::genesis_key.pub)
 				.previous (nano::dev::genesis->hash ())
@@ -2203,34 +2210,30 @@ TEST (node, epoch_conflict_confirm)
 					  .work (*system.work.generate (open->hash ()))
 					  .build ();
 
-	// Process initial blocks on node1
-	ASSERT_TRUE (nano::test::process (node1, { send, send2, open }));
-
-	// Confirm open block in node1 to allow generating votes
-	nano::test::confirm (node1.ledger, open);
-
-	// Process initial blocks on node0
-	ASSERT_TRUE (nano::test::process (node0, { send, send2, open }));
+	// Process initial blocks
+	ASSERT_TRUE (nano::test::process (node0, nano::test::clone ({ send, send2, open })));
+	ASSERT_TRUE (nano::test::process (node1, nano::test::clone ({ send, send2, open })));
 
-	// Process conflicting blocks on node 0 as blocks coming from live network
-	ASSERT_TRUE (nano::test::process_live (node0, { change, epoch_open }));
+	// Process conflicting blocks on nodes as blocks coming from live network
+	ASSERT_TRUE (nano::test::process_live (node0, nano::test::clone ({ change, epoch_open })));
+	ASSERT_TRUE (nano::test::process_live (node1, nano::test::clone ({ change, epoch_open })));
 
 	// Ensure blocks were propagated to both nodes
 	ASSERT_TIMELY (5s, nano::test::exists (node0, { change, epoch_open }));
 	ASSERT_TIMELY (5s, nano::test::exists (node1, { change, epoch_open }));
 
 	// Confirm initial blocks in node1 to allow generating votes later
-	ASSERT_TRUE (nano::test::start_elections (system, node1, { change, epoch_open, send2 }, true));
+	nano::test::confirm (node1, { change, epoch_open, send2 });
 	ASSERT_TIMELY (5s, nano::test::confirmed (node1, { change, epoch_open, send2 }));
 
-	// Start elections for node0 for conflicting change and epoch_open blocks (those two blocks have the same root)
+	// Start elections on node0 for conflicting change and epoch_open blocks (these two blocks have the same root)
 	ASSERT_TRUE (nano::test::activate (node0, { change, epoch_open }));
 	ASSERT_TIMELY (5s, nano::test::active (node0, { change, epoch_open }));
 
-	// Make node1 a representative
+	// Make node1 a representative so it can vote for both blocks
 	system.wallet (1)->insert_adhoc (nano::dev::genesis_key.prv);
 
-	// Ensure the elections for conflicting blocks have completed
+	// Ensure the elections for conflicting blocks have started
 	ASSERT_TIMELY (5s, nano::test::active (node0, { change, epoch_open }));
 
 	// Ensure both conflicting blocks were successfully processed and confirmed

nano/lib/blocks.cpp

@@ -210,7 +210,7 @@ nano::block_hash nano::block::full_hash () const
 
 nano::block_sideband const & nano::block::sideband () const
 {
-	debug_assert (sideband_m.is_initialized ());
+	release_assert (sideband_m.is_initialized ());
 	return *sideband_m;
 }
 
@@ -569,6 +569,11 @@ nano::send_block::send_block (bool & error_a, boost::property_tree::ptree const
 	}
 }
 
+std::shared_ptr<nano::block> nano::send_block::clone () const
+{
+	return std::make_shared<nano::send_block> (*this);
+}
+
 bool nano::send_block::operator== (nano::block const & other_a) const
 {
 	return blocks_equal (*this, other_a);
@@ -758,6 +763,11 @@ nano::open_block::open_block (bool & error_a, boost::property_tree::ptree const
 	}
 }
 
+std::shared_ptr<nano::block> nano::open_block::clone () const
+{
+	return std::make_shared<nano::open_block> (*this);
+}
+
 void nano::open_block::generate_hash (blake2b_state & hash_a) const
 {
 	hashables.hash (hash_a);
@@ -1029,6 +1039,11 @@ nano::change_block::change_block (bool & error_a, boost::property_tree::ptree co
 	}
 }
 
+std::shared_ptr<nano::block> nano::change_block::clone () const
+{
+	return std::make_shared<nano::change_block> (*this);
+}
+
 void nano::change_block::generate_hash (blake2b_state & hash_a) const
 {
 	hashables.hash (hash_a);
@@ -1326,6 +1341,11 @@ nano::state_block::state_block (bool & error_a, boost::property_tree::ptree cons
 	}
 }
 
+std::shared_ptr<nano::block> nano::state_block::clone () const
+{
+	return std::make_shared<nano::state_block> (*this);
+}
+
 void nano::state_block::generate_hash (blake2b_state & hash_a) const
 {
 	nano::uint256_union preamble (static_cast<uint64_t> (nano::block_type::state));
@@ -1792,6 +1812,11 @@ nano::receive_block::receive_block (bool & error_a, boost::property_tree::ptree
 	}
 }
 
+std::shared_ptr<nano::block> nano::receive_block::clone () const
+{
+	return std::make_shared<nano::receive_block> (*this);
+}
+
 void nano::receive_block::generate_hash (blake2b_state & hash_a) const
 {
 	hashables.hash (hash_a);

nano/lib/blocks.hpp

@@ -23,6 +23,7 @@ namespace nano
 class block
 {
 public:
+	virtual ~block () = default;
 	// Return a digest of the hashables in this block.
 	nano::block_hash const & hash () const;
 	// Return a digest of hashables and non-hashables in this block.
@@ -46,10 +47,10 @@ class block
 	virtual nano::block_type type () const = 0;
 	virtual nano::signature const & block_signature () const = 0;
 	virtual void signature_set (nano::signature const &) = 0;
-	virtual ~block () = default;
 	virtual bool valid_predecessor (nano::block const &) const = 0;
 	static size_t size (nano::block_type);
 	virtual nano::work_version work_version () const;
+	virtual std::shared_ptr<nano::block> clone () const = 0;
 	// If there are any changes to the hashables, call this to update the cached hash
 	void refresh ();
 	bool is_send () const noexcept;
@@ -138,6 +139,7 @@ class send_block : public nano::block
 	bool operator== (nano::block const &) const override;
 	bool operator== (nano::send_block const &) const;
 	bool valid_predecessor (nano::block const &) const override;
+	std::shared_ptr<nano::block> clone () const override;
 	send_hashables hashables;
 	nano::signature signature;
 	uint64_t work;
@@ -192,6 +194,7 @@ class receive_block : public nano::block
 	bool operator== (nano::block const &) const override;
 	bool operator== (nano::receive_block const &) const;
 	bool valid_predecessor (nano::block const &) const override;
+	std::shared_ptr<nano::block> clone () const override;
 	receive_hashables hashables;
 	nano::signature signature;
 	uint64_t work;
@@ -247,6 +250,7 @@ class open_block : public nano::block
 	bool operator== (nano::block const &) const override;
 	bool operator== (nano::open_block const &) const;
 	bool valid_predecessor (nano::block const &) const override;
+	std::shared_ptr<nano::block> clone () const override;
 	nano::open_hashables hashables;
 	nano::signature signature;
 	uint64_t work;
@@ -302,6 +306,7 @@ class change_block : public nano::block
 	bool operator== (nano::block const &) const override;
 	bool operator== (nano::change_block const &) const;
 	bool valid_predecessor (nano::block const &) const override;
+	std::shared_ptr<nano::block> clone () const override;
 	nano::change_hashables hashables;
 	nano::signature signature;
 	uint64_t work;
@@ -368,6 +373,7 @@ class state_block : public nano::block
 	bool operator== (nano::block const &) const override;
 	bool operator== (nano::state_block const &) const;
 	bool valid_predecessor (nano::block const &) const override;
+	std::shared_ptr<nano::block> clone () const override;
 	nano::state_hashables hashables;
 	nano::signature signature;
 	uint64_t work;

nano/node/cli.cpp

@@ -646,10 +646,10 @@ std::error_code nano::handle_node_options (boost::program_options::variables_map
 			{
 				auto root_str = root_it->second.as<std::string> ();
 				auto transaction (node.node->store.tx_begin_write ());
-				nano::root root;
+				nano::qualified_root root;
 				if (!root.decode_hex (root_str))
 				{
-					node.node->store.final_vote.clear (transaction, root);
+					node.node->store.final_vote.del (transaction, root);
 					std::cout << "Successfully cleared final votes" << std::endl;
 				}
 				else

nano/node/request_aggregator.cpp

@@ -12,6 +12,7 @@
 #include <nano/node/wallet.hpp>
 #include <nano/secure/ledger.hpp>
 #include <nano/secure/ledger_set_any.hpp>
+#include <nano/secure/ledger_set_confirmed.hpp>
 #include <nano/store/component.hpp>
 
 nano::request_aggregator::request_aggregator (request_aggregator_config const & config_a, nano::node & node_a, nano::stats & stats_a, nano::vote_generator & generator_a, nano::vote_generator & final_generator_a, nano::local_vote_history & history_a, nano::ledger & ledger_a, nano::wallets & wallets_a, nano::vote_router & vote_router_a) :
@@ -208,68 +209,45 @@ void nano::request_aggregator::erase_duplicates (std::vector<std::pair<nano::blo
 	requests_a.end ());
 }
 
+// This filters candidates for vote generation, the final decision and necessary checks are also performed by the vote generator
 auto nano::request_aggregator::aggregate (nano::secure::transaction const & transaction, request_type const & requests_a, std::shared_ptr<nano::transport::channel> const & channel_a) const -> aggregate_result
 {
 	std::vector<std::shared_ptr<nano::block>> to_generate;
 	std::vector<std::shared_ptr<nano::block>> to_generate_final;
 	for (auto const & [hash, root] : requests_a)
 	{
-		bool generate_final_vote (false);
-		std::shared_ptr<nano::block> block;
+		// Ledger by hash
+		std::shared_ptr<nano::block> block = ledger.any.block_get (transaction, hash);
 
-		// 2. Final votes
-		auto final_vote_hashes (ledger.store.final_vote.get (transaction, root));
-		if (!final_vote_hashes.empty ())
+		// Ledger by root
+		if (!block && !root.is_zero ())
 		{
-			generate_final_vote = true;
-			block = ledger.any.block_get (transaction, final_vote_hashes[0]);
-			// Allow same root vote
-			if (block != nullptr && final_vote_hashes.size () > 1)
+			// Search for block root
+			if (auto successor = ledger.any.block_successor (transaction, root.as_block_hash ()))
 			{
-				// WTF? This shouldn't be done like this
-				to_generate_final.push_back (block);
-				block = ledger.any.block_get (transaction, final_vote_hashes[1]);
-				debug_assert (final_vote_hashes.size () == 2);
+				block = ledger.any.block_get (transaction, successor.value ());
+				release_assert (block);
 			}
 		}
 
-		// 4. Ledger by hash
-		if (block == nullptr)
-		{
-			block = ledger.any.block_get (transaction, hash);
-			// Confirmation status. Generate final votes for confirmed
-			if (block != nullptr)
+		auto should_generate_final_vote = [&] (auto const & block) {
+			release_assert (block);
+
+			// Check if final vote is set for this block
+			if (auto final_hash = ledger.store.final_vote.get (transaction, block->qualified_root ()))
 			{
-				nano::confirmation_height_info confirmation_height_info;
-				ledger.store.confirmation_height.get (transaction, block->account (), confirmation_height_info);
-				generate_final_vote = (confirmation_height_info.height >= block->sideband ().height);
+				return final_hash == block->hash ();
 			}
-		}
-
-		// 5. Ledger by root
-		if (block == nullptr && !root.is_zero ())
-		{
-			// Search for block root
-			auto successor = ledger.any.block_successor (transaction, root.as_block_hash ());
-			if (successor)
+			// If the final vote is not set, generate vote if the block is confirmed
+			else
 			{
-				auto successor_block = ledger.any.block_get (transaction, successor.value ());
-				release_assert (successor_block != nullptr);
-				block = std::move (successor_block);
-
-				// Confirmation status. Generate final votes for confirmed successor
-				if (block != nullptr)
-				{
-					nano::confirmation_height_info confirmation_height_info;
-					ledger.store.confirmation_height.get (transaction, block->account (), confirmation_height_info);
-					generate_final_vote = (confirmation_height_info.height >= block->sideband ().height);
-				}
+				return ledger.confirmed.block_exists (transaction, block->hash ());
 			}
-		}
+		};
 
 		if (block)
 		{
-			if (generate_final_vote)
+			if (should_generate_final_vote (block))
 			{
 				to_generate_final.push_back (block);
 				stats.inc (nano::stat::type::requests, nano::stat::detail::requests_final);

nano/store/final_vote.hpp

@@ -22,10 +22,9 @@ class final_vote
 
 public:
 	virtual bool put (store::write_transaction const & transaction_a, nano::qualified_root const & root_a, nano::block_hash const & hash_a) = 0;
-	virtual std::vector<nano::block_hash> get (store::transaction const & transaction_a, nano::root const & root_a) = 0;
-	virtual void del (store::write_transaction const & transaction_a, nano::root const & root_a) = 0;
+	virtual std::optional<nano::block_hash> get (store::transaction const & transaction_a, nano::qualified_root const & qualified_root_a) = 0;
+	virtual void del (store::write_transaction const & transaction_a, nano::qualified_root const & root_a) = 0;
 	virtual size_t count (store::transaction const & transaction_a) const = 0;
-	virtual void clear (store::write_transaction const &, nano::root const &) = 0;
 	virtual void clear (store::write_transaction const &) = 0;
 	virtual iterator begin (store::transaction const & transaction_a, nano::qualified_root const & root_a) const = 0;
 	virtual iterator begin (store::transaction const & transaction_a) const = 0;

nano/store/lmdb/final_vote.cpp

@@ -23,42 +23,29 @@ bool nano::store::lmdb::final_vote::put (store::write_transaction const & transa
 	return result;
 }
 
-std::vector<nano::block_hash> nano::store::lmdb::final_vote::get (store::transaction const & transaction, nano::root const & root_a)
+std::optional<nano::block_hash> nano::store::lmdb::final_vote::get (store::transaction const & transaction, nano::qualified_root const & qualified_root_a)
 {
-	std::vector<nano::block_hash> result;
-	nano::qualified_root key_start{ root_a.raw, 0 };
-	for (auto i = begin (transaction, key_start), n = end (transaction); i != n && nano::qualified_root{ i->first }.root () == root_a; ++i)
+	nano::store::lmdb::db_val result;
+	auto status = store.get (transaction, tables::final_votes, qualified_root_a, result);
+	std::optional<nano::block_hash> final_vote_hash;
+	if (store.success (status))
 	{
-		result.push_back (i->second);
+		final_vote_hash = static_cast<nano::block_hash> (result);
 	}
-	return result;
+	return final_vote_hash;
 }
 
-void nano::store::lmdb::final_vote::del (store::write_transaction const & transaction, nano::root const & root)
+void nano::store::lmdb::final_vote::del (store::write_transaction const & transaction, nano::qualified_root const & root)
 {
-	std::vector<nano::qualified_root> final_vote_qualified_roots;
-	for (auto i = begin (transaction, nano::qualified_root{ root.raw, 0 }), n = end (transaction); i != n && nano::qualified_root{ i->first }.root () == root; ++i)
-	{
-		final_vote_qualified_roots.push_back (i->first);
-	}
-
-	for (auto & final_vote_qualified_root : final_vote_qualified_roots)
-	{
-		auto status = store.del (transaction, tables::final_votes, final_vote_qualified_root);
-		store.release_assert_success (status);
-	}
+	auto status = store.del (transaction, tables::final_votes, root);
+	store.release_assert_success (status);
 }
 
 size_t nano::store::lmdb::final_vote::count (store::transaction const & transaction_a) const
 {
 	return store.count (transaction_a, tables::final_votes);
 }
 
-void nano::store::lmdb::final_vote::clear (store::write_transaction const & transaction_a, nano::root const & root_a)
-{
-	del (transaction_a, root_a);
-}
-
 void nano::store::lmdb::final_vote::clear (store::write_transaction const & transaction_a)
 {
 	store.drop (transaction_a, nano::tables::final_votes);