[Feature] feat: allow-external-peers #3163

joske · 2024-03-06T10:00:31Z

Feature https://github.com/AleoHQ/snarkOS/issues/3155

ljedrz · 2024-03-06T10:04:05Z

Ok, so if nogossip is set, the node will not be requesting more peers; shouldn't this also mean that the node doesn't "shuffle" its peers, so that it doesn't lose them?

joske · 2024-03-06T10:09:45Z

@vicsn ?

vicsn · 2024-03-06T10:35:15Z

shouldn't this also mean that the node doesn't "shuffle" its peers, so that it doesn't lose them?

Yes, I did not have this logic in mind. @joske can you analyse and update the PR? Depending on the scope of required changes, we may also need to rename the flag to make it sound appropriate.

Can you also test the behavior manually? One suggested approach: start a network of e.g. 10 validators each having the new flag and the same 10 hardcoded client peers, and after half an hour you can check if all validators still have all client peers connected. Feel free to suggest another approach. Soon™️ this will be an automated integration test in CI, but that will take a few weeks still. :)

vicsn

For clarity, can you share with how many validators and clients you tested?

I just tested using the devnet.sh script, without the --allow-outside-peers flag, and the clients still connect, is that expected behaviour? Or is there extra code we need so we can easily test in devnet.sh if this feature works?

Can you also confirm this is not an issue? https://github.com/AleoHQ/snarkOS/pull/3163#issuecomment-1980505760

cli/src/commands/start.rs

vicsn

Logic mostly LGTM.

Test duplicate_disconnect_attempts is failing, I assume because we set the new flag to false when the test expects it to be true.

Can you add the new flag to the validators in the devnet.sh and .devnet/start.sh scripts?

howardwu · 2024-03-09T00:34:26Z

I would advocate for allow-external-peers or no-external-peers (preferred) as the default flag

vicsn · 2024-03-09T08:47:14Z

I would advocate for allow-external-peers or no-external-peers (preferred) as the default flag

This PR ensures default behaviour WITHOUT flag is to NOT connect with peers. Therefore calling the flag --allow-external-peers sounds good to me, --no-external-peers is confusing.

howardwu · 2024-03-10T19:49:17Z

--allow-external-peers works for me. This means the default behavior is to not allow external peers.

@joske can you update this PR to reflect this change?

joske · 2024-03-10T22:16:52Z

Yes, was waiting for confirmation on the final name :)

howardwu · 2024-03-10T22:17:48Z

Yes, was waiting for confirmation on the final name :)

Please see above: --allow-external-peers is confirmed

.devnet/start.sh

devnet.sh

…ossip

vicsn · 2024-03-14T17:49:48Z

node/src/client/mod.rs

@@ -117,6 +117,7 @@ impl<N: Network, C: ConsensusStorage<N>> Client<N, C> {
            trusted_peers,
            Self::MAXIMUM_NUMBER_OF_PEERS as u16,
            matches!(storage_mode, StorageMode::Development(_)),
+            false,


I should have spotted this sooner, but this is a bug. I think this should be true, and we should set it true for all the other invocations of Router::new as well, except for the validator where its set by a parameter. Because clients/provers are expected to always do peer discovery.

raychu86 · 2024-03-14T18:07:02Z

node/src/prover/mod.rs

@@ -110,6 +110,7 @@ impl<N: Network, C: ConsensusStorage<N>> Prover<N, C> {
            trusted_peers,
            Self::MAXIMUM_NUMBER_OF_PEERS as u16,
            matches!(storage_mode, StorageMode::Development(_)),
+            false,


https://github.com/AleoHQ/snarkOS/pull/3163/files#r1525287483

@vicsn Should this be true as well according to your comment above?

howardwu · 2024-03-15T01:06:25Z

@joske can you update all instances of false to true? This includes the tests.

node/router/tests/common/mod.rs

howardwu · 2024-03-15T16:53:55Z

node/router/src/handshake.rs

@@ -264,6 +264,12 @@ impl<N: Network> Router<N> {
        if self.is_connected(&peer_ip) {
            bail!("Dropping connection request from '{peer_ip}' (already connected)")
        }
+        // Only allow trusted peers to connect if we are a validator
+        // (unless allow_external_peers is set)
+        let is_validator = self.node_type().is_validator();


nit: is_validator is no longer necessary as part of the check (because allow_external_peers takes precedence)

howardwu · 2024-03-15T16:54:00Z

node/router/src/heartbeat.rs

@@ -107,6 +107,12 @@ pub trait Heartbeat<N: Network>: Outbound<N> {
            return;
        }

+        let is_validator = self.router().node_type().is_validator();
+        // Skip if the node is not requesting peers.
+        if is_validator && !self.router().allow_external_peers() {


nit: is_validator is no longer necessary as part of the check (because allow_external_peers takes precedence)

howardwu · 2024-03-15T16:54:06Z

node/router/src/heartbeat.rs

-            for peer_ip in self.router().connected_peers().into_iter().choose_multiple(rng, 3) {
-                self.send(peer_ip, Message::PeerRequest(PeerRequest));
+            let is_validator = self.router().node_type().is_validator();
+            if !is_validator || self.router().allow_external_peers() {


nit: is_validator is no longer necessary as part of the check (because allow_external_peers takes precedence)

howardwu · 2024-03-15T16:54:11Z

node/router/src/inbound.rs

@@ -125,6 +125,9 @@ pub trait Inbound<N: Network>: Reading + Outbound<N> {
                if !self.router().cache.contains_outbound_peer_request(peer_ip) {
                    bail!("Peer '{peer_ip}' is not following the protocol (unexpected peer response)")
                }
+                if self.router().node_type().is_validator() && !self.router().allow_external_peers() {


nit: is_validator is no longer necessary as part of the check (because allow_external_peers takes precedence)

howardwu · 2024-03-15T16:56:27Z

cli/src/commands/start.rs

+    #[clap(long = "allow-external-peers")]
+    /// If the flag is set, the validator will allow untrusted peers to connect


Suggested change

#[clap(long = "allow-external-peers")]

/// If the flag is set, the validator will allow untrusted peers to connect

/// If the flag is set, the validator will allow untrusted peers to connect

#[clap(long = "allow-external-peers")]

joske force-pushed the feat/nopeergossip branch from 2784f6a to b7278ee Compare March 7, 2024 08:11

joske changed the title ~~feat: nopeergossip~~ feat: peergossip Mar 7, 2024

joske force-pushed the feat/nopeergossip branch 2 times, most recently from 514bb7e to f8274d2 Compare March 7, 2024 09:32

joske marked this pull request as ready for review March 7, 2024 12:53

howardwu closed this Mar 7, 2024

joske reopened this Mar 7, 2024

joske force-pushed the feat/nopeergossip branch from f8274d2 to 41dbb80 Compare March 7, 2024 19:26

vicsn changed the title ~~feat: peergossip~~ feat: allow-outside-peers Mar 7, 2024

vicsn self-requested a review March 7, 2024 19:29

joske force-pushed the feat/nopeergossip branch from 41dbb80 to 4747fd4 Compare March 7, 2024 19:52

vicsn reviewed Mar 8, 2024

View reviewed changes

cli/src/commands/start.rs Outdated Show resolved Hide resolved

joske force-pushed the feat/nopeergossip branch 2 times, most recently from db5a291 to 2485f0a Compare March 8, 2024 09:11

feat: allow-outside-peers

3221e52

joske force-pushed the feat/nopeergossip branch from 2485f0a to 3221e52 Compare March 8, 2024 09:12

vicsn suggested changes Mar 8, 2024

View reviewed changes

gweiworld mentioned this pull request Mar 8, 2024

Client Sync (Test Final): 10 clients per validator (directly connected as fringe clients), 900 clients connecting to the fringe clients, in a decentralized topology. monadicus/snarkops#17

Closed

1 task

joske added 2 commits March 8, 2024 15:57

feat: adjust devnet scripts

c8a02c8

fix: test

ce1ca02

fix: rename parameter

6003adb

joske changed the title ~~feat: allow-outside-peers~~ feat: allow-external-peers Mar 11, 2024

howardwu reviewed Mar 12, 2024

View reviewed changes

.devnet/start.sh Outdated Show resolved Hide resolved

howardwu reviewed Mar 12, 2024

View reviewed changes

devnet.sh Outdated Show resolved Hide resolved

howardwu reviewed Mar 12, 2024

View reviewed changes

devnet.sh Outdated Show resolved Hide resolved

joske added 2 commits March 13, 2024 08:36

fix: rename parameter in script

a682e2a

Merge remote-tracking branch 'aleo/mainnet-staging' into feat/nopeerg…

8b2db44

…ossip

vicsn reviewed Mar 14, 2024

View reviewed changes

raychu86 reviewed Mar 14, 2024

View reviewed changes

joske commented Mar 15, 2024

View reviewed changes

node/router/tests/common/mod.rs Show resolved Hide resolved

fix: allow external peers for non-validators

6e14920

vicsn approved these changes Mar 15, 2024

View reviewed changes

howardwu reviewed Mar 15, 2024

View reviewed changes

vicsn mentioned this pull request Mar 15, 2024

feat: allow-external-peers #2 #3172

Closed

vicsn added 3 commits March 15, 2024 21:33

Use allow_external_peers without checking is_validator

8ecd4e1

Nit: Reverse order of arguments

a74d626

Correct comments

47ac52f

raychu86 approved these changes Mar 18, 2024

View reviewed changes

raychu86 merged commit 1e3eee6 into AleoNet:mainnet-staging Mar 18, 2024
23 of 24 checks passed

vicsn mentioned this pull request Mar 25, 2024

[Fix] Make --node optional to ensure it has an influence even when using --dev; renames --storage_path to --storage #3189

Merged

niklaslong mentioned this pull request Mar 26, 2024

[Fix] Fix flaky connection cleanup case #3192

Merged

howardwu mentioned this pull request Mar 26, 2024

[Tracking] Pre-release of the mainnet branch #3160

Draft

howardwu changed the title ~~feat: allow-external-peers~~ [Feature] feat: allow-external-peers Mar 26, 2024

This was referenced Mar 29, 2024

[Bug] Peers are not sufficiently connecting to each other #3196

Closed

[Test] Add unit test for validator Router connections #3198

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] feat: allow-external-peers #3163

[Feature] feat: allow-external-peers #3163

joske commented Mar 6, 2024

ljedrz commented Mar 6, 2024

joske commented Mar 6, 2024

vicsn commented Mar 6, 2024

vicsn left a comment •

edited

Loading

vicsn left a comment

howardwu commented Mar 9, 2024

vicsn commented Mar 9, 2024

howardwu commented Mar 10, 2024

joske commented Mar 10, 2024

howardwu commented Mar 10, 2024

vicsn Mar 14, 2024

raychu86 Mar 14, 2024 •

edited

Loading

vicsn Mar 14, 2024

howardwu commented Mar 15, 2024

howardwu Mar 15, 2024

howardwu Mar 15, 2024

howardwu Mar 15, 2024

howardwu Mar 15, 2024

howardwu Mar 15, 2024

		#[clap(long = "allow-external-peers")]
		/// If the flag is set, the validator will allow untrusted peers to connect

[Feature] feat: allow-external-peers #3163

[Feature] feat: allow-external-peers #3163

Conversation

joske commented Mar 6, 2024

ljedrz commented Mar 6, 2024

joske commented Mar 6, 2024

vicsn commented Mar 6, 2024

vicsn left a comment • edited Loading

Choose a reason for hiding this comment

vicsn left a comment

Choose a reason for hiding this comment

howardwu commented Mar 9, 2024

vicsn commented Mar 9, 2024

howardwu commented Mar 10, 2024

joske commented Mar 10, 2024

howardwu commented Mar 10, 2024

vicsn Mar 14, 2024

Choose a reason for hiding this comment

raychu86 Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

vicsn Mar 14, 2024

Choose a reason for hiding this comment

howardwu commented Mar 15, 2024

howardwu Mar 15, 2024

Choose a reason for hiding this comment

howardwu Mar 15, 2024

Choose a reason for hiding this comment

howardwu Mar 15, 2024

Choose a reason for hiding this comment

howardwu Mar 15, 2024

Choose a reason for hiding this comment

howardwu Mar 15, 2024

Choose a reason for hiding this comment

vicsn left a comment •

edited

Loading

raychu86 Mar 14, 2024 •

edited

Loading