From dc7d2db4aeafe7d11a551aa311b3ebc6db435bc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Thu, 9 Nov 2023 19:52:42 +0100 Subject: [PATCH] Remove support for confirm req with block payload --- nano/core_test/message.cpp | 31 ------ nano/core_test/message_deserializer.cpp | 17 --- nano/core_test/node.cpp | 8 +- nano/node/messages.cpp | 109 ++++++------------- nano/node/messages.hpp | 11 +- nano/node/network.cpp | 10 +- nano/node/transport/message_deserializer.cpp | 9 +- 7 files changed, 45 insertions(+), 150 deletions(-) diff --git a/nano/core_test/message.cpp b/nano/core_test/message.cpp index 082a600978..b5b7195ad8 100644 --- a/nano/core_test/message.cpp +++ b/nano/core_test/message.cpp @@ -128,35 +128,6 @@ TEST (message, confirm_ack_hash_serialization) ASSERT_EQ (hashes, con2.vote->hashes); // Check overflow with max hashes ASSERT_EQ (header.count_get (), hashes.size ()); - ASSERT_EQ (header.block_type (), nano::block_type::not_a_block); -} - -TEST (message, confirm_req_serialization) -{ - nano::keypair key1; - nano::keypair key2; - nano::block_builder builder; - auto block = builder - .send () - .previous (0) - .destination (key2.pub) - .balance (200) - .sign (nano::keypair ().prv, 2) - .work (3) - .build_shared (); - nano::confirm_req req{ nano::dev::network_params.network, block }; - std::vector bytes; - { - nano::vectorstream stream (bytes); - req.serialize (stream); - } - auto error (false); - nano::bufferstream stream2 (bytes.data (), bytes.size ()); - nano::message_header header (error, stream2); - nano::confirm_req req2 (error, stream2, header); - ASSERT_FALSE (error); - ASSERT_EQ (req, req2); - ASSERT_EQ (*req.block, *req2.block); } TEST (message, confirm_req_hash_serialization) @@ -185,7 +156,6 @@ TEST (message, confirm_req_hash_serialization) ASSERT_FALSE (error); ASSERT_EQ (req, req2); ASSERT_EQ (req.roots_hashes, req2.roots_hashes); - ASSERT_EQ (header.block_type (), nano::block_type::not_a_block); ASSERT_EQ (header.count_get (), req.roots_hashes.size ()); } @@ -239,7 +209,6 @@ TEST (message, confirm_req_hash_batch_serialization) ASSERT_EQ (req.roots_hashes, req2.roots_hashes); ASSERT_EQ (req.roots_hashes, roots_hashes); ASSERT_EQ (req2.roots_hashes, roots_hashes); - ASSERT_EQ (header.block_type (), nano::block_type::not_a_block); ASSERT_EQ (header.count_get (), req.roots_hashes.size ()); } diff --git a/nano/core_test/message_deserializer.cpp b/nano/core_test/message_deserializer.cpp index d1e717e466..e2e2851e3a 100644 --- a/nano/core_test/message_deserializer.cpp +++ b/nano/core_test/message_deserializer.cpp @@ -74,23 +74,6 @@ TEST (message_deserializer, exact_confirm_ack) message_deserializer_success_checker (message); } -TEST (message_deserializer, exact_confirm_req) -{ - nano::test::system system{ 1 }; - nano::block_builder builder; - auto block = builder - .send () - .previous (1) - .destination (1) - .balance (2) - .sign (nano::keypair ().prv, 4) - .work (*system.work.generate (nano::root (1))) - .build_shared (); - nano::confirm_req message{ nano::dev::network_params.network, block }; - - message_deserializer_success_checker (message); -} - TEST (message_deserializer, exact_confirm_req_hash) { nano::test::system system{ 1 }; diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 65ba12a705..2627406a02 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -2277,8 +2277,8 @@ TEST (node, local_votes_cache) election->force_confirm (); ASSERT_TIMELY (3s, node.ledger.cache.cemented_count == 3); system.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv); - nano::confirm_req message1{ nano::dev::network_params.network, send1 }; - nano::confirm_req message2{ nano::dev::network_params.network, send2 }; + nano::confirm_req message1{ nano::dev::network_params.network, send1->hash (), send1->root () }; + nano::confirm_req message2{ nano::dev::network_params.network, send2->hash (), send2->root () }; auto channel = std::make_shared (node); node.network.inbound (message1, channel); ASSERT_TIMELY (3s, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes) == 1); @@ -2300,7 +2300,7 @@ TEST (node, local_votes_cache) auto transaction (node.store.tx_begin_write ()); ASSERT_EQ (nano::process_result::progress, node.ledger.process (transaction, *send3).code); } - nano::confirm_req message3{ nano::dev::network_params.network, send3 }; + nano::confirm_req message3{ nano::dev::network_params.network, send3->hash (), send3->root () }; for (auto i (0); i < 100; ++i) { node.network.inbound (message3, channel); @@ -2400,7 +2400,7 @@ TEST (node, local_votes_cache_generate_new_vote) system.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv); // Send a confirm req for genesis block to node - nano::confirm_req message1{ nano::dev::network_params.network, nano::dev::genesis }; + nano::confirm_req message1{ nano::dev::network_params.network, nano::dev::genesis->hash (), nano::dev::genesis->root () }; auto channel = std::make_shared (node); node.network.inbound (message1, channel); diff --git a/nano/node/messages.cpp b/nano/node/messages.cpp index 0f4699c009..dd8ebbcda2 100644 --- a/nano/node/messages.cpp +++ b/nano/node/messages.cpp @@ -286,7 +286,7 @@ std::size_t nano::message_header::payload_length_bytes () const } case nano::message_type::confirm_req: { - return nano::confirm_req::size (block_type (), count_get ()); + return nano::confirm_req::size (*this); } case nano::message_type::node_id_handshake: { @@ -515,32 +515,22 @@ nano::confirm_req::confirm_req (bool & error_a, nano::stream & stream_a, nano::m } } -nano::confirm_req::confirm_req (nano::network_constants const & constants, std::shared_ptr const & block_a) : - message (constants, nano::message_type::confirm_req), - block (block_a) -{ - header.block_type_set (block->type ()); -} - nano::confirm_req::confirm_req (nano::network_constants const & constants, std::vector> const & roots_hashes_a) : message (constants, nano::message_type::confirm_req), roots_hashes (roots_hashes_a) { - // not_a_block (1) block type for hashes + roots request - header.block_type_set (nano::block_type::not_a_block); + debug_assert (!roots_hashes.empty ()); debug_assert (roots_hashes.size () < 16); + + // Set `not_a_block` (1) block type for hashes + roots request + // This is needed to keep compatibility with previous protocol versions (<= V25.1) + header.block_type_set (nano::block_type::not_a_block); header.count_set (static_cast (roots_hashes.size ())); } nano::confirm_req::confirm_req (nano::network_constants const & constants, nano::block_hash const & hash_a, nano::root const & root_a) : - message (constants, nano::message_type::confirm_req), - roots_hashes (std::vector> (1, std::make_pair (hash_a, root_a))) + confirm_req (constants, std::vector>{ { hash_a, root_a } }) { - debug_assert (!roots_hashes.empty ()); - // not_a_block (1) block type for hashes + roots request - header.block_type_set (nano::block_type::not_a_block); - debug_assert (roots_hashes.size () < 16); - header.count_set (static_cast (roots_hashes.size ())); } void nano::confirm_req::visit (nano::message_visitor & visitor_a) const @@ -550,52 +540,39 @@ void nano::confirm_req::visit (nano::message_visitor & visitor_a) const void nano::confirm_req::serialize (nano::stream & stream_a) const { + debug_assert (!roots_hashes.empty ()); + header.serialize (stream_a); - if (header.block_type () == nano::block_type::not_a_block) - { - debug_assert (!roots_hashes.empty ()); - // Write hashes & roots - for (auto & root_hash : roots_hashes) - { - write (stream_a, root_hash.first); - write (stream_a, root_hash.second); - } - } - else + + // Write hashes & roots + for (auto & root_hash : roots_hashes) { - debug_assert (block != nullptr); - block->serialize (stream_a); + nano::write (stream_a, root_hash.first); + nano::write (stream_a, root_hash.second); } } bool nano::confirm_req::deserialize (nano::stream & stream_a, nano::block_uniquer * uniquer_a) { - bool result (false); debug_assert (header.type == nano::message_type::confirm_req); + + bool result = false; try { - if (header.block_type () == nano::block_type::not_a_block) + uint8_t const count = header.count_get (); + for (auto i (0); i != count && !result; ++i) { - uint8_t count (header.count_get ()); - for (auto i (0); i != count && !result; ++i) + nano::block_hash block_hash (0); + nano::block_hash root (0); + nano::read (stream_a, block_hash); + nano::read (stream_a, root); + if (!block_hash.is_zero () || !root.is_zero ()) { - nano::block_hash block_hash (0); - nano::block_hash root (0); - read (stream_a, block_hash); - read (stream_a, root); - if (!block_hash.is_zero () || !root.is_zero ()) - { - roots_hashes.emplace_back (block_hash, root); - } + roots_hashes.emplace_back (block_hash, root); } - - result = roots_hashes.empty () || (roots_hashes.size () != count); - } - else - { - block = nano::deserialize_block (stream_a, header.block_type (), uniquer_a); - result = block == nullptr; } + + result = roots_hashes.empty () || (roots_hashes.size () != count); } catch (std::runtime_error const &) { @@ -608,11 +585,7 @@ bool nano::confirm_req::deserialize (nano::stream & stream_a, nano::block_unique bool nano::confirm_req::operator== (nano::confirm_req const & other_a) const { bool equal (false); - if (block != nullptr && other_a.block != nullptr) - { - equal = *block == *other_a.block; - } - else if (!roots_hashes.empty () && !other_a.roots_hashes.empty ()) + if (!roots_hashes.empty () && !other_a.roots_hashes.empty ()) { equal = roots_hashes == other_a.roots_hashes; } @@ -632,34 +605,19 @@ std::string nano::confirm_req::roots_string () const return result; } -std::size_t nano::confirm_req::size (nano::block_type type_a, std::size_t count) +std::size_t nano::confirm_req::size (nano::message_header const & header) { - std::size_t result (0); - if (type_a != nano::block_type::invalid && type_a != nano::block_type::not_a_block) - { - result = nano::block::size (type_a); - } - else if (type_a == nano::block_type::not_a_block) - { - result = count * (sizeof (nano::uint256_union) + sizeof (nano::block_hash)); - } - return result; + auto const count = header.count_get (); + return count * (sizeof (decltype (roots_hashes)::value_type::first) + sizeof (decltype (roots_hashes)::value_type::second)); } std::string nano::confirm_req::to_string () const { std::string s = header.to_string (); - if (header.block_type () == nano::block_type::not_a_block) + for (auto && roots_hash : roots_hashes) { - for (auto && roots_hash : roots_hashes) - { - s += "\n" + roots_hash.first.to_string () + ":" + roots_hash.second.to_string (); - } - } - else - { - s += "\n" + block->to_json (); + s += "\n" + roots_hash.first.to_string () + ":" + roots_hash.second.to_string (); } return s; @@ -683,14 +641,13 @@ nano::confirm_ack::confirm_ack (nano::network_constants const & constants, std:: message (constants, nano::message_type::confirm_ack), vote (vote_a) { - header.block_type_set (nano::block_type::not_a_block); debug_assert (vote_a->hashes.size () < 16); + header.count_set (static_cast (vote_a->hashes.size ())); } void nano::confirm_ack::serialize (nano::stream & stream_a) const { - debug_assert (header.block_type () == nano::block_type::not_a_block || header.block_type () == nano::block_type::send || header.block_type () == nano::block_type::receive || header.block_type () == nano::block_type::open || header.block_type () == nano::block_type::change || header.block_type () == nano::block_type::state); header.serialize (stream_a); vote->serialize (stream_a); } diff --git a/nano/node/messages.hpp b/nano/node/messages.hpp index bf4ab2b2ab..5fa1e08b44 100644 --- a/nano/node/messages.hpp +++ b/nano/node/messages.hpp @@ -145,19 +145,20 @@ class publish final : public message class confirm_req final : public message { public: - confirm_req (bool &, nano::stream &, nano::message_header const &, nano::block_uniquer * = nullptr); - confirm_req (nano::network_constants const & constants, std::shared_ptr const &); + confirm_req (bool & error, nano::stream &, nano::message_header const &, nano::block_uniquer * = nullptr); confirm_req (nano::network_constants const & constants, std::vector> const &); confirm_req (nano::network_constants const & constants, nano::block_hash const &, nano::root const &); void serialize (nano::stream &) const override; bool deserialize (nano::stream &, nano::block_uniquer * = nullptr); void visit (nano::message_visitor &) const override; bool operator== (nano::confirm_req const &) const; - std::shared_ptr block; - std::vector> roots_hashes; std::string roots_string () const; - static std::size_t size (nano::block_type, std::size_t = 0); std::string to_string () const; + + static std::size_t size (nano::message_header const &); + +public: // Payload + std::vector> roots_hashes; }; class confirm_ack final : public message diff --git a/nano/node/network.cpp b/nano/node/network.cpp index a612697e70..a9c1974399 100644 --- a/nano/node/network.cpp +++ b/nano/node/network.cpp @@ -417,20 +417,12 @@ class network_message_visitor : public nano::message_visitor { node.logger.try_log (boost::str (boost::format ("Confirm_req message from %1% for hashes:roots %2%") % channel->to_string () % message_a.roots_string ())); } - else - { - node.logger.try_log (boost::str (boost::format ("Confirm_req message from %1% for %2%") % channel->to_string () % message_a.block->hash ().to_string ())); - } } // Don't load nodes with disabled voting if (node.config.enable_voting && node.wallets.reps ().voting > 0) { - if (message_a.block != nullptr) - { - node.aggregator.add (channel, { { message_a.block->hash (), message_a.block->root () } }); - } - else if (!message_a.roots_hashes.empty ()) + if (!message_a.roots_hashes.empty ()) { node.aggregator.add (channel, message_a.roots_hashes); } diff --git a/nano/node/transport/message_deserializer.cpp b/nano/node/transport/message_deserializer.cpp index 661a70c511..1a90637280 100644 --- a/nano/node/transport/message_deserializer.cpp +++ b/nano/node/transport/message_deserializer.cpp @@ -236,14 +236,7 @@ std::unique_ptr nano::transport::message_deserializer::deseri auto incoming = std::make_unique (error, stream, header, &block_uniquer_m); if (!error && nano::at_end (stream)) { - if (incoming->block == nullptr || !network_constants_m.work.validate_entry (*incoming->block)) - { - return incoming; - } - else - { - status = parse_status::insufficient_work; - } + return incoming; } else {