Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix mismatched channel owners #4750

Merged
merged 3 commits into from
Oct 16, 2024
Merged

Fix mismatched channel owners #4750

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
753c7e6
Move `node.inbound ()`
pwojcikdev Oct 8, 2024
77e1237
Assert channel owner
pwojcikdev Oct 8, 2024
7730d9a
Fix tests
pwojcikdev Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions nano/core_test/bootstrap_server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ TEST (bootstrap_server, serve_account_blocks)
request.payload = request_payload;
request.update_header ();

node.network.inbound (request, nano::test::fake_channel (node));
node.inbound (request, nano::test::fake_channel (node));

ASSERT_TIMELY_EQ (5s, responses.size (), 1);

Expand Down Expand Up @@ -137,7 +137,7 @@ TEST (bootstrap_server, serve_hash)
request.payload = request_payload;
request.update_header ();

node.network.inbound (request, nano::test::fake_channel (node));
node.inbound (request, nano::test::fake_channel (node));

ASSERT_TIMELY_EQ (5s, responses.size (), 1);

Expand Down Expand Up @@ -182,7 +182,7 @@ TEST (bootstrap_server, serve_hash_one)
request.payload = request_payload;
request.update_header ();

node.network.inbound (request, nano::test::fake_channel (node));
node.inbound (request, nano::test::fake_channel (node));

ASSERT_TIMELY_EQ (5s, responses.size (), 1);

Expand Down Expand Up @@ -221,7 +221,7 @@ TEST (bootstrap_server, serve_end_of_chain)
request.payload = request_payload;
request.update_header ();

node.network.inbound (request, nano::test::fake_channel (node));
node.inbound (request, nano::test::fake_channel (node));

ASSERT_TIMELY_EQ (5s, responses.size (), 1);

Expand Down Expand Up @@ -260,7 +260,7 @@ TEST (bootstrap_server, serve_missing)
request.payload = request_payload;
request.update_header ();

node.network.inbound (request, nano::test::fake_channel (node));
node.inbound (request, nano::test::fake_channel (node));

ASSERT_TIMELY_EQ (5s, responses.size (), 1);

Expand Down Expand Up @@ -303,7 +303,7 @@ TEST (bootstrap_server, serve_multiple)
request.payload = request_payload;
request.update_header ();

node.network.inbound (request, nano::test::fake_channel (node));
node.inbound (request, nano::test::fake_channel (node));
}
}

Expand Down Expand Up @@ -359,7 +359,7 @@ TEST (bootstrap_server, serve_account_info)
request.payload = request_payload;
request.update_header ();

node.network.inbound (request, nano::test::fake_channel (node));
node.inbound (request, nano::test::fake_channel (node));

ASSERT_TIMELY_EQ (5s, responses.size (), 1);

Expand Down Expand Up @@ -405,7 +405,7 @@ TEST (bootstrap_server, serve_account_info_missing)
request.payload = request_payload;
request.update_header ();

node.network.inbound (request, nano::test::fake_channel (node));
node.inbound (request, nano::test::fake_channel (node));

ASSERT_TIMELY_EQ (5s, responses.size (), 1);

Expand Down Expand Up @@ -450,7 +450,7 @@ TEST (bootstrap_server, serve_frontiers)
request.payload = request_payload;
request.update_header ();

node.network.inbound (request, nano::test::fake_channel (node));
node.inbound (request, nano::test::fake_channel (node));

ASSERT_TIMELY_EQ (5s, responses.size (), 1);

Expand Down Expand Up @@ -503,7 +503,7 @@ TEST (bootstrap_server, serve_frontiers_invalid_count)
request.payload = request_payload;
request.update_header ();

node.network.inbound (request, nano::test::fake_channel (node));
node.inbound (request, nano::test::fake_channel (node));
}

ASSERT_TIMELY_EQ (5s, node.stats.count (nano::stat::type::bootstrap_server, nano::stat::detail::invalid), 1);
Expand All @@ -521,7 +521,7 @@ TEST (bootstrap_server, serve_frontiers_invalid_count)
request.payload = request_payload;
request.update_header ();

node.network.inbound (request, nano::test::fake_channel (node));
node.inbound (request, nano::test::fake_channel (node));
}

ASSERT_TIMELY_EQ (5s, node.stats.count (nano::stat::type::bootstrap_server, nano::stat::detail::invalid), 2);
Expand All @@ -539,7 +539,7 @@ TEST (bootstrap_server, serve_frontiers_invalid_count)
request.payload = request_payload;
request.update_header ();

node.network.inbound (request, nano::test::fake_channel (node));
node.inbound (request, nano::test::fake_channel (node));
}

ASSERT_TIMELY_EQ (5s, node.stats.count (nano::stat::type::bootstrap_server, nano::stat::detail::invalid), 3);
Expand Down
8 changes: 4 additions & 4 deletions nano/core_test/network.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ TEST (receivable_processor, confirm_insufficient_pos)
nano::confirm_ack con1{ nano::dev::network_params.network, vote };
auto channel1 = std::make_shared<nano::transport::inproc::channel> (node1, node1);
ASSERT_EQ (1, election->votes ().size ());
node1.network.inbound (con1, channel1);
node1.inbound (con1, channel1);
ASSERT_TIMELY_EQ (5s, 2, election->votes ().size ())
ASSERT_FALSE (election->confirmed ());
}
Expand All @@ -402,7 +402,7 @@ TEST (receivable_processor, confirm_sufficient_pos)
nano::confirm_ack con1{ nano::dev::network_params.network, vote };
auto channel1 = std::make_shared<nano::transport::inproc::channel> (node1, node1);
ASSERT_EQ (1, election->votes ().size ());
node1.network.inbound (con1, channel1);
node1.inbound (con1, channel1);
ASSERT_TIMELY_EQ (5s, 2, election->votes ().size ())
ASSERT_TRUE (election->confirmed ());
}
Expand Down Expand Up @@ -743,10 +743,10 @@ TEST (network, duplicate_revert_publish)
auto channel = nano::test::establish_tcp (system, *other_node, node.network.endpoint ());
ASSERT_NE (nullptr, channel);
ASSERT_EQ (0, publish.digest);
node.network.inbound (publish, channel);
node.inbound (publish, channel);
ASSERT_TRUE (node.network.filter.apply (bytes.data (), bytes.size ()));
publish.digest = digest;
node.network.inbound (publish, channel);
node.inbound (publish, channel);
ASSERT_FALSE (node.network.filter.apply (bytes.data (), bytes.size ()));
}

Expand Down
46 changes: 23 additions & 23 deletions nano/core_test/node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,15 +689,15 @@ TEST (node, fork_flip)
.build ();
nano::publish publish2{ nano::dev::network_params.network, send2 };
auto ignored_channel = nano::test::fake_channel (node1);
node1.network.inbound (publish1, ignored_channel);
node2.network.inbound (publish2, ignored_channel);
node1.inbound (publish1, ignored_channel);
node2.inbound (publish2, ignored_channel);
ASSERT_TIMELY_EQ (5s, 1, node1.active.size ());
ASSERT_TIMELY_EQ (5s, 1, node2.active.size ());
system.wallet (0)->insert_adhoc (nano::dev::genesis_key.prv);
// Fill nodes with forked blocks
node1.network.inbound (publish2, ignored_channel);
node1.inbound (publish2, ignored_channel);
ASSERT_TIMELY (5s, node1.active.active (*send2));
node2.network.inbound (publish1, ignored_channel);
node2.inbound (publish1, ignored_channel);
ASSERT_TIMELY (5s, node2.active.active (*send1));
auto election1 (node2.active.election (nano::qualified_root (nano::dev::genesis->hash (), nano::dev::genesis->hash ())));
ASSERT_NE (nullptr, election1);
Expand Down Expand Up @@ -829,7 +829,7 @@ TEST (node, fork_open)
.build ();
nano::publish publish1{ nano::dev::network_params.network, send1 };
auto channel1 = std::make_shared<nano::transport::fake::channel> (node);
node.network.inbound (publish1, channel1);
node.inbound (publish1, channel1);
ASSERT_TIMELY (5s, (election = node.active.election (publish1.block->qualified_root ())) != nullptr);
election->force_confirm ();
ASSERT_TIMELY (5s, node.active.empty () && node.block_confirmed (publish1.block->hash ()));
Expand All @@ -848,7 +848,7 @@ TEST (node, fork_open)
.work (*system.work.generate (key1.pub))
.build ();
nano::publish publish2{ nano::dev::network_params.network, open1 };
node.network.inbound (publish2, channel1);
node.inbound (publish2, channel1);
ASSERT_TIMELY_EQ (5s, 1, node.active.size ());

// create 2nd open block, which is a fork of open1 block
Expand All @@ -860,7 +860,7 @@ TEST (node, fork_open)
.work (*system.work.generate (key1.pub))
.build ();
nano::publish publish3{ nano::dev::network_params.network, open2 };
node.network.inbound (publish3, channel1);
node.inbound (publish3, channel1);
ASSERT_TIMELY (5s, (election = node.active.election (publish3.block->qualified_root ())) != nullptr);

// we expect to find 2 blocks in the election and we expect the first block to be the winner just because it was first
Expand Down Expand Up @@ -1856,14 +1856,14 @@ TEST (node, DISABLED_local_votes_cache)
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<nano::transport::fake::channel> (node);
node.network.inbound (message1, channel);
node.inbound (message1, channel);
ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes), 1);
node.network.inbound (message2, channel);
node.inbound (message2, channel);
ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes), 2);
for (auto i (0); i < 100; ++i)
{
node.network.inbound (message1, channel);
node.network.inbound (message2, channel);
node.inbound (message1, channel);
node.inbound (message2, channel);
}
// Make sure a new vote was not generated
ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes), 2);
Expand All @@ -1873,15 +1873,15 @@ TEST (node, DISABLED_local_votes_cache)
ASSERT_EQ (nano::block_status::progress, node.ledger.process (transaction, send3));
}
nano::confirm_req message3{ nano::dev::network_params.network, send3->hash (), send3->root () };
node.network.inbound (message3, channel);
node.inbound (message3, channel);
ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes), 3);
ASSERT_TIMELY (3s, !node.history.votes (send1->root (), send1->hash ()).empty ());
ASSERT_TIMELY (3s, !node.history.votes (send2->root (), send2->hash ()).empty ());
ASSERT_TIMELY (3s, !node.history.votes (send3->root (), send3->hash ()).empty ());
// All requests should be served from the cache
for (auto i (0); i < 100; ++i)
{
node.network.inbound (message3, channel);
node.inbound (message3, channel);
}
ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::requests, nano::stat::detail::requests_generated_votes), 3);
}
Expand Down Expand Up @@ -1935,26 +1935,26 @@ TEST (node, DISABLED_local_votes_cache_batch)
nano::confirm_req message{ nano::dev::network_params.network, batch };
auto channel = std::make_shared<nano::transport::fake::channel> (node);
// Generates and sends one vote for both hashes which is then cached
node.network.inbound (message, channel);
node.inbound (message, channel);
ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out), 1);
ASSERT_EQ (1, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out));
ASSERT_FALSE (node.history.votes (send2->root (), send2->hash ()).empty ());
ASSERT_FALSE (node.history.votes (receive1->root (), receive1->hash ()).empty ());
// Only one confirm_ack should be sent if all hashes are part of the same vote
node.network.inbound (message, channel);
node.inbound (message, channel);
ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out), 2);
ASSERT_EQ (2, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out));
// Test when votes are different
node.history.erase (send2->root ());
node.history.erase (receive1->root ());
node.network.inbound (nano::confirm_req{ nano::dev::network_params.network, send2->hash (), send2->root () }, channel);
node.inbound (nano::confirm_req{ nano::dev::network_params.network, send2->hash (), send2->root () }, channel);
ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out), 3);
ASSERT_EQ (3, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out));
node.network.inbound (nano::confirm_req{ nano::dev::network_params.network, receive1->hash (), receive1->root () }, channel);
node.inbound (nano::confirm_req{ nano::dev::network_params.network, receive1->hash (), receive1->root () }, channel);
ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out), 4);
ASSERT_EQ (4, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out));
// There are two different votes, so both should be sent in response
node.network.inbound (message, channel);
node.inbound (message, channel);
ASSERT_TIMELY_EQ (3s, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out), 6);
ASSERT_EQ (6, node.stats.count (nano::stat::type::message, nano::stat::detail::confirm_ack, nano::stat::dir::out));
}
Expand All @@ -1975,7 +1975,7 @@ TEST (node, DISABLED_local_votes_cache_generate_new_vote)
// Send a confirm req for genesis block to node
nano::confirm_req message1{ nano::dev::network_params.network, nano::dev::genesis->hash (), nano::dev::genesis->root () };
auto channel = std::make_shared<nano::transport::fake::channel> (node);
node.network.inbound (message1, channel);
node.inbound (message1, channel);

// check that the node generated a vote for the genesis block and that it is stored in the local vote cache and it is the only vote
ASSERT_TIMELY (5s, !node.history.votes (nano::dev::genesis->root (), nano::dev::genesis->hash ()).empty ());
Expand All @@ -1998,7 +1998,7 @@ TEST (node, DISABLED_local_votes_cache_generate_new_vote)
// One of the hashes is cached
std::vector<std::pair<nano::block_hash, nano::root>> roots_hashes{ std::make_pair (nano::dev::genesis->hash (), nano::dev::genesis->root ()), std::make_pair (send1->hash (), send1->root ()) };
nano::confirm_req message2{ nano::dev::network_params.network, roots_hashes };
node.network.inbound (message2, channel);
node.inbound (message2, channel);
ASSERT_TIMELY (3s, !node.history.votes (send1->root (), send1->hash ()).empty ());
auto votes2 (node.history.votes (send1->root (), send1->hash ()));
ASSERT_EQ (1, votes2.size ());
Expand Down Expand Up @@ -2441,13 +2441,13 @@ TEST (node, fork_election_invalid_block_signature)
.build ();

auto channel1 = std::make_shared<nano::transport::fake::channel> (node1);
node1.network.inbound (nano::publish{ nano::dev::network_params.network, send1 }, channel1);
node1.inbound (nano::publish{ nano::dev::network_params.network, send1 }, channel1);
ASSERT_TIMELY (5s, node1.active.active (send1->qualified_root ()));
auto election (node1.active.election (send1->qualified_root ()));
ASSERT_NE (nullptr, election);
ASSERT_EQ (1, election->blocks ().size ());
node1.network.inbound (nano::publish{ nano::dev::network_params.network, send3 }, channel1);
node1.network.inbound (nano::publish{ nano::dev::network_params.network, send2 }, channel1);
node1.inbound (nano::publish{ nano::dev::network_params.network, send3 }, channel1);
node1.inbound (nano::publish{ nano::dev::network_params.network, send2 }, channel1);
ASSERT_TIMELY (3s, election->blocks ().size () > 1);
ASSERT_EQ (election->blocks ()[send2->hash ()]->block_signature (), send2->block_signature ());
}
Expand Down
4 changes: 2 additions & 2 deletions nano/core_test/telemetry.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ TEST (telemetry, invalid_signature)
telemetry.block_count = 9999; // Change data so signature is no longer valid

auto message = nano::telemetry_ack{ nano::dev::network_params.network, telemetry };
node.network.inbound (message, nano::test::fake_channel (node));
node.inbound (message, nano::test::fake_channel (node));

ASSERT_TIMELY (5s, node.stats.count (nano::stat::type::telemetry, nano::stat::detail::invalid_signature) > 0);
ASSERT_ALWAYS (1s, node.stats.count (nano::stat::type::telemetry, nano::stat::detail::process) == 0)
Expand All @@ -267,7 +267,7 @@ TEST (telemetry, mismatched_node_id)
auto telemetry = node.local_telemetry ();

auto message = nano::telemetry_ack{ nano::dev::network_params.network, telemetry };
node.network.inbound (message, nano::test::fake_channel (node, /* node id */ { 123 }));
node.inbound (message, nano::test::fake_channel (node, /* node id */ { 123 }));

ASSERT_TIMELY (5s, node.stats.count (nano::stat::type::telemetry, nano::stat::detail::node_id_mismatch) > 0);
ASSERT_ALWAYS (1s, node.stats.count (nano::stat::type::telemetry, nano::stat::detail::process) == 0)
Expand Down
4 changes: 2 additions & 2 deletions nano/core_test/websocket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ TEST (websocket, started_election)
.build ();
nano::publish publish1{ nano::dev::network_params.network, send1 };
auto channel1 = std::make_shared<nano::transport::fake::channel> (*node1);
node1->network.inbound (publish1, channel1);
node1->inbound (publish1, channel1);
ASSERT_TIMELY (1s, node1->active.election (send1->qualified_root ()));
ASSERT_TIMELY_EQ (5s, future.wait_for (0s), std::future_status::ready);

Expand Down Expand Up @@ -213,7 +213,7 @@ TEST (websocket, stopped_election)
.build ();
nano::publish publish1{ nano::dev::network_params.network, send1 };
auto channel1 = std::make_shared<nano::transport::fake::channel> (*node1);
node1->network.inbound (publish1, channel1);
node1->inbound (publish1, channel1);
ASSERT_TIMELY (5s, node1->active.election (send1->qualified_root ()));
node1->active.erase (*send1);

Expand Down
8 changes: 0 additions & 8 deletions nano/node/network.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,14 +314,6 @@ void nano::network::flood_block_many (std::deque<std::shared_ptr<nano::block>> b
}
}

void nano::network::inbound (const nano::message & message, const std::shared_ptr<nano::transport::channel> & channel)
{
debug_assert (message.header.network == node.network_params.network.current_network);
debug_assert (message.header.version_using >= node.network_params.network.protocol_version_min);

node.message_processor.process (message, channel);
}

// Send keepalives to all the peers we've been notified of
void nano::network::merge_peers (std::array<nano::endpoint, 8> const & peers_a)
{
Expand Down
1 change: 0 additions & 1 deletion nano/node/network.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,6 @@ class network final
void erase (nano::transport::channel const &);
/** Disconnects and adds peer to exclusion list */
void exclude (std::shared_ptr<nano::transport::channel> const & channel);
void inbound (nano::message const &, std::shared_ptr<nano::transport::channel> const &);

nano::container_info container_info () const;

Expand Down
10 changes: 10 additions & 0 deletions nano/node/node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,16 @@ void nano::node::keepalive (std::string const & address_a, uint16_t port_a)
});
}

void nano::node::inbound (const nano::message & message, const std::shared_ptr<nano::transport::channel> & channel)
{
debug_assert (channel->owner () == shared_from_this ()); // This node should be the channel owner
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could have owner return a naked pointer and compare against 'this' since we're not intending to manage lifetime. I'm fine with keeping this though since it's just test code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if it was compiled in release then this should be done, but I try to avoid naked pointers unless absolutely necessary to avoid shooting myself in the foot accidentally.


debug_assert (message.header.network == network_params.network.current_network);
debug_assert (message.header.version_using >= network_params.network.protocol_version_min);

message_processor.process (message, channel);
}

void nano::node::process_active (std::shared_ptr<nano::block> const & incoming)
{
block_processor.add (incoming);
Expand Down
1 change: 1 addition & 0 deletions nano/node/node.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ class node final : public std::enable_shared_from_this<node>
bool copy_with_compaction (std::filesystem::path const &);
void keepalive (std::string const &, uint16_t);
int store_version ();
void inbound (nano::message const &, std::shared_ptr<nano::transport::channel> const &);
void process_confirmed (nano::election_status const &, uint64_t = 0);
void process_active (std::shared_ptr<nano::block> const &);
std::optional<nano::block_status> process_local (std::shared_ptr<nano::block> const &);
Expand Down
5 changes: 5 additions & 0 deletions nano/node/transport/channel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ nano::endpoint nano::transport::channel::get_peering_endpoint () const
}
}

std::shared_ptr<nano::node> nano::transport::channel::owner () const
{
return node.shared ();
}

void nano::transport::channel::operator() (nano::object_stream & obs) const
{
obs.write ("endpoint", get_endpoint ());
Expand Down
2 changes: 2 additions & 0 deletions nano/node/transport/channel.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,8 @@ class channel
nano::endpoint get_peering_endpoint () const;
void set_peering_endpoint (nano::endpoint endpoint);

std::shared_ptr<nano::node> owner () const;

mutable nano::mutex channel_mutex;

private:
Expand Down
Loading