[Test] Add unit test for validator Router connections #3198
Conversation
node/router/src/handshake.rs
Outdated
| @@ -251,7 +252,7 @@ impl<N: Network> Router<N> { | |||
| } | |||
|
|
|||
| /// Ensure the peer is allowed to connect. | |||
| fn ensure_peer_is_allowed(&self, peer_ip: SocketAddr) -> Result<()> { | |||
| fn ensure_peer_is_allowed(&self, peer_ip: SocketAddr, node_type: NodeType) -> Result<()> { | |||
There was a problem hiding this comment.
Note that we have to trust the node type as it's given to us by the peer, I don't think we previously used it for anything so we're introducing a new assumption.
There was a problem hiding this comment.
You are right. The easiest immediate resolution is to pass in validator routers explicitly via --peers. I added a unit test for it and updated the PR description. 2974612
| // Connect node0 to node1. | ||
| node0.connect(node1.local_ip()); | ||
| // Sleep briefly. | ||
| tokio::time::sleep(Duration::from_millis(200)).await; |
There was a problem hiding this comment.
deadline could be used here to avoid the sleep.
There was a problem hiding this comment.
Will keep it in mind for future tests, for now I'll keep the logic similar to the rest of the tests in the file.
There was a problem hiding this comment.
IIRC connect returns a JoinHandle that can be .awaited (and once it's complete, it means the connection is either ready or that it has failed), so there is no need for a sleep here
There was a problem hiding this comment.
Just tried it. Just awaiting node0's connect is not enough, because node1 might not be connected yet. I suggest we leave refactoring this file for later.
vicsn
changed the title
vicsn
changed the title
| // Connect node1 to node0. | ||
| node1.connect(node0.local_ip()); | ||
| // Sleep briefly. | ||
| tokio::time::sleep(Duration::from_millis(200)).await; |
|
|
||
| { | ||
| // Connect node0 to node1. | ||
| node0.connect(node1.local_ip()); |
There was a problem hiding this comment.
aren't we racing against the Heartbeat (that might cause node1 to attempt to connect to node0) here? then again, that direction should fail, so it's probably fine
There was a problem hiding this comment.
heartbeat is not turned on, only if we call initialize_routing.
howardwu
changed the title
Motivation
Due to the recently introduced
--allow-external-peersflag, validator Routers do not by default accept connections from other validator Routers. We noticed on tests with only validators that thesnarkos_router_connected_totalmetric returned 0.To make this behavior clear, added a unit test
test_validator_connection. In order for validator's Routers to connect to one another, their address has to be explicitly passed via--peers.Note that if we want to change that behaviour, we need to either:
Resolverto track whether we are not connected to the same address twice)Related PRs
Closes #3196
Corrects yet another regression from #3163
Improves on #3192