Listen on multiple addresses for net_plugin p2p. #1411

jgiszczak · 2023-07-15T23:51:00Z

This PR allows net_plugin to listen on multiple address:port pairs.

--p2p-listen-endpoint may now be specified multiple times. Duplicates are ignored.
--p2p-server-address may now be specified multiple times. It may be specified no more times than the number of p2p listen endpoints. Each p2p listen endpoint will be assigned a p2p-server-address in order until the list is exhausted. Listen endpoints with no corresponding p2p-server-address will use their listen address and port as usual. For outgoing connections, the first p2p-server-address will be used as the peer address in handshakes. If not specified, the first listen endpoint will be used.

This feature enables differing firewall rules per port, allowing the ability to distinguish between internal cluster peers, other producer peers, and general public peers, as well as any other categories which may be useful.

Documentation Updates

This PR adds the following fields to the results of the connections API in net_api_plugin:

remote_ip
remote_port
is_socket_open
is_blocks_only
is_transactions_only

The first two are strings. The rest are boolean values.

spoonincode

This change prevents disabling p2p listen endpoint, is there some way we can maintain that ability?

plugins/net_plugin/net_plugin.cpp

heifner · 2023-07-19T16:24:03Z

plugins/net_plugin/net_plugin.cpp

-            p2p_address = options.at( "p2p-listen-endpoint" ).as<string>();
-            EOS_ASSERT( p2p_address.length() <= max_p2p_address_length, chain::plugin_config_exception,
-                        "p2p-listen-endpoint too long, must be less than ${m}", ("m", max_p2p_address_length) );
+         if( options.count( "p2p-listen-endpoint" ) && !options.at("p2p-listen-endpoint").as<vector<string>>().empty() &&  options.at("p2p-listen-endpoint").as<vector<string>>()[0].length()) {


I thinks && !options.at("p2p-listen-endpoint").as<vector<string>>()[0].empty() is easier to understand.

I think we can remove vector checks at all. code below should perfectly handle empty vector. Or you can add a check later if vector is empty - looks more succinct, easier to understand and more optimal since you do not construct vector multiple times

We'd like to retain the checks in case the default value is ever removed. I figured it was ok to reconstruct the vector because this is startup code executed only once and it would be very unusual for the vector size to exceed even 10 elements.

my main point was having concise and easy to read code at first. I agree that performance could be neglected here if it adds readability or simplicity.
So if you remove it or at least split that huge if statement - that will improve readability.
technically I do not see any difference in functionality if you'll leave it like that:

if( options.count( "p2p-listen-endpoint" ) ) { }

in that case p2p_addresses will be empty anyways and sort /erase won't crash on empty vector. If I missed something you could just split the if like that:

if( options.count( "p2p-listen-endpoint" ) ) { auto p2ps = options.at( "p2p-listen-endpoint" ).as<vector<string>>(); if (!p2ps.empty() && !p2ps.front().empty()) { p2p_addresses = p2ps; /*...*/ } }

I'm not hard on this, just trying to keep code reasonably simple. net_plugin code is complicated enough already.

Updated for readability.

plugins/net_plugin/net_plugin.cpp

heifner · 2023-07-19T16:31:57Z

plugins/net_plugin/net_plugin.cpp


-      if( !p2p_address.empty() ) {
-         auto [host, port] = fc::split_host_port(listen_address);
+      std::transform(p2p_addresses.begin(), p2p_addresses.end(), p2p_server_addresses.begin(), 


I know you do a resize above, but an assert that these are the same size would be good.

it looks better to move line 3991 just above that one.

Added assert.

@dimas1185 I'm sorry, the line numbers have moved around too much. I'm not sure what you mean.

@jgiszczak you can skip it. assert looks good.
I meant this one:

`p2p_server_addresses.resize(p2p_addresses.size()); // extend with empty entries as needed`

plugins/net_plugin/net_plugin.cpp

Outbound connections are always attributed to the first listen address.

dimas1185

please add unit and/or integration test where you check behavior for multiple endpoints specified

Erase undefined elements from p2p_addresses vector after std::unique(). Add assert before std::transform of two vectors. Const correctness and whitespace cleanup.

plugins/net_plugin/net_plugin.cpp

heifner · 2023-07-20T16:32:53Z

plugins/net_plugin/include/eosio/net_plugin/auto_bp_peering.hpp

@@ -182,7 +182,7 @@ class bp_connection_manager {

               fc_dlog(self()->get_logger(), "pending_downstream_neighbors: ${pending_downstream_neighbors}",
                       ("pending_downstream_neighbors", to_string(pending_downstream_neighbors)));
-               for (auto neighbor : pending_downstream_neighbors) { self()->connections.connect(config.bp_peer_addresses[neighbor]); }
+               for (auto neighbor : pending_downstream_neighbors) { self()->connections.connect(config.bp_peer_addresses[neighbor], *self()->p2p_addresses.begin() ); }


As discussed please update help and PR description indicating that the first configured server_address is reported in the handshake message to peers. Also guard against p2p_addresses being empty here.

my concern resolved

dimas1185 · 2023-07-21T19:21:25Z

tests/p2p_multiple_listen_test.py

@@ -0,0 +1,75 @@
+#!/usr/bin/env python3


thanks for adding the test.
It is a bit confusing, can you add more comments about its actions?

also, now net_plugin's options p2p-listen-endpoint and p2p-server-address could be a vector. So I think we should have a test where you specify multiple entries and ensure those are used appropriately.

Comments added and text expanded to multiple ports and to test the --p2p-server-address parameter better as well.

Exercise multiple listen endpoints, with overrides, in p2p test.

Add network topology diagrams to p2p_multiple_listen_test.

BenjaminGormanPMP · 2023-08-17T13:35:48Z

[Group: "P2P Enhancements"]

jgiszczak added 2 commits July 15, 2023 18:43

Listen on multiple addresses for net_plugin p2p.

3a7c072

Move post of timers outside listener post loop.

d5fdd64

jgiszczak requested review from dimas1185 and heifner July 17, 2023 16:11

jgiszczak marked this pull request as ready for review July 17, 2023 16:12

spoonincode previously requested changes Jul 17, 2023

View reviewed changes

Restore running nodeos with zero p2p listen ports.

1c81166

heifner requested changes Jul 19, 2023

View reviewed changes

Set p2p_address for outgoing connections as well.

db93c63

Outbound connections are always attributed to the first listen address.

dimas1185 suggested changes Jul 19, 2023

View reviewed changes

jgiszczak added 2 commits July 19, 2023 14:02

Rename connection's p2p_address to listen_address.

4d71d2a

Erase undefined elements from p2p_addresses vector after std::unique(). Add assert before std::transform of two vectors. Const correctness and whitespace cleanup.

Add p2p multiple listen port test and new fields to connections API

3c10138

heifner added the OCI Work exclusive to OCI team label Jul 20, 2023

heifner reviewed Jul 20, 2023

View reviewed changes

plugins/net_plugin/net_plugin.cpp Outdated Show resolved Hide resolved

heifner reviewed Jul 20, 2023

View reviewed changes

jgiszczak added 3 commits July 20, 2023 18:12

Expand description of p2p-server-address option and normalize language.

a4f9399

Merge branch 'main' into p2p-multiple-listen-addresses

1bbb36a

Merge branch 'main' into p2p-multiple-listen-addresses

374f8a3

dimas1185 reviewed Jul 21, 2023

View reviewed changes

jgiszczak added 3 commits July 21, 2023 17:26

Revise net_plugin p2p-listen-endpoint parsing for readability.

2f55f15

Exercise multiple listen endpoints, with overrides, in p2p test.

Add test to verify nodeos can run with p2p disabled.

4a4875e

Extend auto bp peering test to include p2p-server-address args.

31f3889

Add network topology diagrams to p2p_multiple_listen_test.

dimas1185 approved these changes Jul 25, 2023

View reviewed changes

Merge branch 'main' into p2p-multiple-listen-addresses

501056f

heifner approved these changes Jul 25, 2023

View reviewed changes

jgiszczak merged commit 172eb8d into main Jul 25, 2023

jgiszczak deleted the p2p-multiple-listen-addresses branch July 25, 2023 16:12

jgiszczak mentioned this pull request Aug 28, 2023

Support throttling block syncing to peers. #1540

Closed

heifner linked an issue Sep 19, 2023 that may be closed by this pull request

Allow use of multiple ports for P2P #1324

Closed

bhazzard mentioned this pull request Sep 19, 2023

Allow use of multiple ports for P2P #1324

Closed

jgiszczak mentioned this pull request Oct 10, 2023

[5.0] Support throttling block syncing to peers #1741

Merged

heifner mentioned this pull request Oct 10, 2023

[5.0 -> main] Support throttling block syncing to peers #1742

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Listen on multiple addresses for net_plugin p2p. #1411

Listen on multiple addresses for net_plugin p2p. #1411

jgiszczak commented Jul 15, 2023 •

edited

Loading

spoonincode left a comment

heifner Jul 19, 2023

dimas1185 Jul 19, 2023 •

edited

Loading

jgiszczak Jul 21, 2023

dimas1185 Jul 21, 2023 •

edited

Loading

jgiszczak Jul 21, 2023

heifner Jul 19, 2023

dimas1185 Jul 19, 2023

jgiszczak Jul 19, 2023

jgiszczak Jul 21, 2023

dimas1185 Jul 21, 2023

dimas1185 left a comment

heifner Jul 20, 2023

dimas1185 Jul 21, 2023

jgiszczak Jul 21, 2023

BenjaminGormanPMP commented Aug 17, 2023

Listen on multiple addresses for net_plugin p2p. #1411

Listen on multiple addresses for net_plugin p2p. #1411

Conversation

jgiszczak commented Jul 15, 2023 • edited Loading

Documentation Updates

spoonincode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas1185 Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas1185 Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas1185 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenjaminGormanPMP commented Aug 17, 2023

jgiszczak commented Jul 15, 2023 •

edited

Loading

dimas1185 Jul 19, 2023 •

edited

Loading

dimas1185 Jul 21, 2023 •

edited

Loading