Start creating connections (edges) with large nonces #7966
Conversation
mm-near
changed the title
| &self, | ||
| clock: &time::Clock, | ||
| peer1: &PeerId, | ||
| with_nonce: Option<u64>, |
There was a problem hiding this comment.
The approach you took to inject a nonce is totally valid and I agree with it, however on the other hand it has become quite intrusive, given that we need to pass around the nonce explicitly through a bunch of calls and it is used only for tests. Please consider using a lower level abstraction than PeerActor to force a custom nonce. I already planned to do that in my next PR: https://github.com/near/nearcore/pull/8076/files#diff-b691489bc9c0e1683942223dad298acaca321241108c799bf7376c641d75be1a . PTAL at chain/network/src/peer_manager/tests/nonce.rs there, to see whether it would also simplify your PR.
There was a problem hiding this comment.
the with_nonce was only to fix that one test (that I see that you also fixed in your PR)
| // Make sure that it gets updated too. | ||
| for new_edge in new_local_edges { | ||
| this.tier2.update_edge(&new_edge); | ||
| } |
There was a problem hiding this comment.
local_edges are the edges of the graph adjacent to this node.
edge stored in connection is the edge that the ends of the connection have agreed upon during handshake.
Given that now the edge is supposed to change dynamically and that it is delivered to a node via SyncRoutingTable message it doesn't make sense to duplicate it I guess. We can either:
- remove edge field from the connection entirely and just rely on local_edges (I think that would be preferred)
- revert to using ResponseUpdateNonce message, so that each PeerActor can manage the connection.edge on their own.
There was a problem hiding this comment.
yeah, we could remove the edge from the connection -- but I'd do that in a separate PR (as this one is becoming too large)
There was a problem hiding this comment.
also you unnecesarily take a lock on the whole pool in Pool::update_edge. Instead just update the edge in-place.
There was a problem hiding this comment.
and for that you might want ArcMutex/ArcSwap instead of AtomicCell.
| use crate::tcp; | ||
| use crate::testonly::make_rng; | ||
| use crate::testonly::stream; | ||
| use crate::time; | ||
| use crate::types::Edge; | ||
| use ::time::Duration; |
There was a problem hiding this comment.
use crate::time;
don't import Duration directly, and use crate::time instead of ::time.
| &self, | ||
| clock: &time::Clock, | ||
| peer1: &PeerId, | ||
| with_nonce: Option<u64>, |
There was a problem hiding this comment.
is this even used now once you merged with master?
There was a problem hiding this comment.
yes it is - in last_edge.
* preparing for sending large nonces * also show nonce on debug page. * add time since nonce was created to html * set proper nonces on re-initialzing of the connection * fixed broken test * added more tests * reviews feedback part1 * comments * logs fix * comments and cleanup unnecessary clock advance * added missing updates * compile fixes * test fixed * fixed compilation issue * review comment * fixed after merge
No description provided.