vechain / thor

A general purpose blockchain highly compatible with Ethereum's ecosystem
GNU Lesser General Public License v3.0
795 stars 248 forks source link

Adds allowed-peers Flag #688

Closed otherview closed 2 months ago

otherview commented 3 months ago

Description

Adds a new flag that limits the connections of a node. This features help and allows for a few features like bootstrapping to be limited to a single node (good for metrics), DMZ's and traffic-shapping in general is now easier.

Flags accept a list of enodes :

--allowed-peers enode://797fdd968592ca3b59a143f1aa2f152913499d4bb469f2bd5b62dfb1257707b4cb0686563fe144ee2088b1cc4f174bd72df51dbeb7ec1c5b6a8d8599c756f38b@107.150.112.22:55555 enode://3eae6740af6180bb015309f7a07ff7405d6f1f9f1e5a9f2fabbd36b0c00b862521e63ff23573ffdb9035f2237c26513cb9f02454f9ada993e60b99ffc187bb54@107.150.112.21:55555

Fixes # (issue)

Type of change

Please delete options that are not relevant.

How Has This Been Tested?

codecov-commenter commented 3 months ago

Codecov Report

Attention: Patch coverage is 5.26316% with 36 lines in your changes are missing coverage. Please review.

Project coverage is 61.35%. Comparing base (2441238) to head (12816f0).

Files Patch % Lines
cmd/thor/utils.go 0.00% 34 Missing :warning:
cmd/thor/main.go 0.00% 1 Missing :warning:
p2psrv/server.go 66.66% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #688 +/- ## ========================================== + Coverage 61.24% 61.35% +0.11% ========================================== Files 194 194 Lines 18207 18221 +14 ========================================== + Hits 11150 11179 +29 + Misses 5975 5960 -15 Partials 1082 1082 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

claytonneal commented 3 months ago

heya, do we need up update any README or docs.vechain.org for this?

libotony commented 3 months ago

We already have a --bootnode flag that does part of the feature but does not limit other connections. I would consider adding a new flag --no-discovery that disables the discovery feature.

As the --bootnode flag indicates bootstrapping/discovery node, might not be ideal to indicate that it's the nodes that used to be connecting via peer-to-peer network, a new flag can be added for the designated feature.

And for the --no-discovery option, I think it's better to be a hidden flag, as it won't be a general usage and shouldn't be used in general cases.

otherview commented 2 months ago

Myself and @libotony did a pair-prog on this, due to flag behaviours we ended up agreeing on allow-peers-only as a flag that only allows connectivity to certain peers.

The TLDR is that --no-discovery + --bootnodes in certain cases can still allow connections to other nodes.

paologalligit commented 2 months ago

Myself and @libotony did a pair-prog on this, due to flag behaviours we ended up agreeing on allow-peers-only as a flag that only allows connectivity to certain peers.

The TLDR is that --no-discovery + --bootnodes in certain cases can still allow connections to other nodes.

About the last line, is it because this is only an outgoing connection restricted list and so it doesn't imply that another node could start an incoming connection with the node (even if not in the allow-peers flag)?