feat(p2p): ensure time synchronization in the network #2255

onur-ozkan · 2024-10-28T12:37:49Z

Implementation Details

Once a connection is established with a peer, validation check is immediately performed to confirm the peer's clock. If the peer's time exceeds the maximum allowed cap, the connection is terminated. This approach ensures that all peers performing operations in the network are synchronized in terms of time. Current time cap is set to 30 seconds, which can be changed if it doesn't fit the requirements.

Each peer is temporarily added to the RECENTLY_DIALED_PEERS map for 5 minutes before any connection attempt. This prevents repeated connection attempts to peers that are unavailable or out-of-sync, which reduces unnecessary reconnection overheads.

                      +---------------------+
                      |  Connection Attempt |
                      |       On a Peer     |
                      +---------------------+
                                |
                                v
                   +------------------------+
                   |   Is already dialed    |
                   |       recently?        |
                   +------------------------+
                      |                    |
                      Yes                  No
                      |                    |
                      v                    v
        +---------------------+       +-----------------------+
        |   Skip Connection   |       |  Make the Connection  |
        +---------------------+       +-----------------------+
                                             |
                                             v
                            +---------------------------------+
                            |     Check Peer Time Validity    |
                            +---------------------------------+
                                    |              |
                               Time Valid     Time Invalid
                                    |              |
                                    v              v
                                    |       +-------------------------+
                                    |       | Disconnect from Peer    |
                                    |       +-------------------------+
                                    |
                                    |
                                    v
                +------------------------------------+
                |       Validation completed         |
                +------------------------------------+

Breaking Changes

A new request-response payload (GetPeerUtcTimestamp) has been added. This request is sent at each connection attempt, which makes it mandatory for all seed nodes and GUI applications in the network to be updated to this version. Meaning, this version is incompatible with any older version.
The NetworkInfoRequest has been renamed to PeerInfoRequest. This likely affects the encoded payload of GetMm2Version (not yet verified but highly likely).

Resolves #1115 and #1683

~~Blocked by #2256~~

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan · 2024-10-31T12:22:04Z

mm2src/mm2_p2p/src/application/request_response/mod.rs

-    /// TODO: This should be called `PeerInfoRequest` instead. However, renaming it
-    /// will introduce a breaking change in the network and is not worth it. Do this
-    /// renaming when there is already a breaking change in the release.
-    NetworkInfo(network_info::NetworkInfoRequest),


This PR already leads to breaking change. It's perfect time to rename this.

One way to avoid breaking changes is to have a timeout on the request for peer timestamp and allow them to connect if there is no response from the peer. We later remove this when most peers update. Another way is to get the mm2 version and do the timestamp request depending on the version. What do you think?

One way to avoid breaking changes is to have a timeout on the request for peer timestamp and allow them to connect if there is no response from the peer. We later remove this when most peers update.

This sounds like a good plan!

Another way is to get the mm2 version and do the timestamp request depending on the version. What do you think?

I think the first idea was better, but then I need to revert the peer-info renaming part. What about bundling these changes in some branch, and then releasing multiple breaking changes at once (like removing mm2 binaries and some other stuff) ?

What about bundling these changes in some branch, and then releasing multiple breaking changes at once (like removing mm2 binaries and some other stuff) ?

I am fine with this or the first approach.

Signed-off-by: onur-ozkan <work@onurozkan.dev>

… into time-synced-network

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan · 2024-11-05T05:32:43Z

mm2src/mm2_p2p/src/behaviours/atomicdex.rs

+/// Used in time validation logic for each peer which runs immediately  after the
+/// `ConnectionEstablished` event.
+const MAX_TIME_GAP_FOR_CONNECTED_PEER: u64 = 30;


I am not sure if this is a good default. Should we make it like 20 or 25?

30 seconds are fine since it's half the max allowed difference for swaps..

komodo-defi-framework/mm2src/mm2_main/src/lp_swap.rs

Line 154 in f487947

const MAX_STARTED_AT_DIFF: u64 = 60;

However, An edge case that can happen in theory is time differences accumulating across the network. Consider a scenario with peers A, B, C and D:

Peers B and C are relay/seed nodes

Peers A and D are starting a swap as taker/maker

If time differences exist as follows:

Peer A is 30 secs off peer B but they allow them to connect

Peer D is also 30 secs off peer C but they allow them to connect

Peer B and C are out of sync as well (any number of seconds less than 30)

Peer A and D can have more than 60 seconds difference and the swap fails.

This is an edge case that we should keep in mind in case we got time difference errors in a swap after this fix, but I think it will happen in very rare cases if any.

P.S. let's make it 20 secs to reduce the probability of this edge case happening.

One last note :)
We should probably make time differences dependent, for instance MAX_STARTED_AT_DIFF = MAX_TIME_GAP_FOR_CONNECTED_PEER * 3 in case a dev wants to change one of them, they should be aware of how this affects or is affected by other time differences.

The edge case can only happen if seed nodes are not synced in time, which shouldn't be the case I believe? Since we are the maintainers of the seed network.

P.S. let's make it 20 secs to reduce the probability of this edge case happening.

I can do that, but I am not sure if that's a good idea. I had ~9 seconds diff between my laptop and desktop computer, so I believe in some cases it could be even more than that.

Is there a way to inform the peer that their clock is off so that they can adjust it?

Just read internal discussions about this, the currently connected peers is one solution to this but not optimal, will think of something if there are any other solutions. A problem we have is the no login mode, GUIs should be able to show the orderbooks and other info for no login mode even if the peer clock is off, it will be strange for someone who wants to have a look at our dex to get a warning about system clock when not logged in.

IMO it's totally reasonable to tell someone to sync their time if their system isn't (which shouldn't be the case for general-purpose OS users). There are so many apps doing this.

There are so many apps doing this.

Never seen it for web apps like komodo wallet though.

I guess maybe you never run your OS unsynced in time :D We can provide some REST api to provide some general data for no-login mode. If we want to keep the network synced in time, I don't know what else we can do other than not connecting to unsynced ones.

I guess maybe you never run your OS unsynced in time :D

You are probably right :D

If we want to keep the network synced in time, I don't know what else we can do other than not connecting to unsynced ones.

I would leave the PR as is and if we got something from GUI team about this we can look for workarounds for this particular case. We need to mention it though while they are testing.

Signed-off-by: onur-ozkan <work@onurozkan.dev>

laruh · 2024-11-06T12:04:45Z

@onur-ozkan

Once a connection is established with a peer, validation check is immediately performed to confirm the peer's clock. If the peer's time exceeds the maximum allowed cap

Could you clarify please, we compare which peer time with what?

onur-ozkan · 2024-11-06T12:39:19Z

@onur-ozkan

Once a connection is established with a peer, validation check is immediately performed to confirm the peer's clock. If the peer's time exceeds the maximum allowed cap

Could you clarify please, we compare which peer time with what?

Current peer's time against to the remote peer.

mariocynicys

Thanks so much for the detailed PR desc :)
First review iter, will prolly need just one more.

mm2src/mm2_p2p/src/behaviours/atomicdex.rs

mariocynicys · 2024-11-12T10:06:51Z

mm2src/mm2_p2p/src/behaviours/atomicdex.rs

+const MAX_TIME_GAP_FOR_CONNECTED_PEER: u64 = 30;
+
+/// Used for storing peers in [`RECENTLY_DIALED_PEERS`].
+const DIAL_RETRY_DELAY: Duration = Duration::from_secs(60 * 5);


nit: I would write this 5 * 60 :)

explanation: it's just semantically clearer to put the conversion factors at the end (and ordered, so for 2 days converted to seconds u would use 2 * 24 * 60 * 60)

mm2src/mm2_p2p/src/application/request_response/network_info.rs

mm2src/mm2_p2p/src/behaviours/atomicdex.rs

mariocynicys · 2024-11-12T12:26:43Z

mm2src/mm2_p2p/src/behaviours/atomicdex.rs

+
+    match request_one_peer(peer, encoded_request, rp_sender).await {
+        PeerResponse::Ok { res } => {
+            if let Ok(timestamp) = decode_message::<u64>(&res) {


ummm, this is decoded as u64, but the response here...

mariocynicys · 2024-11-12T12:28:22Z

mm2src/mm2_main/src/lp_stats.rs

+fn process_get_peer_utc_timestamp_request() -> Result<Vec<u8>, String> {
+    let timestamp = common::get_utc_timestamp();
+    encode_message(&timestamp).map_err(|e| e.to_string())
+}


...encodes it as a i64. we should stick to only one. u64 makes the most sense application-wise as we don't really need the negative side? right?

but curious, is the encoding of i64 and u64 compatible?

we should stick to only one. u64 makes the most sense application-wise as we don't really need the negative side? right?

Why do you want to cast the value again when you can simply encode it?

but curious, is the encoding of i64 and u64 compatible?

They are

Why do you want to cast the value again when you can simply encode it?

didn't know the encoding for both is compatible, still not sure how the negative side is encoded (probably as rust's then, will checkout).
so thing is, you actually do some kind of an implicit cast here using data encoding and decoding, you encode the number as i64, send it, decode it as u64 from the receiver. i assume a negative i64 will decode fine as a u64 just like what the as keyword does but give a garbage data (will check more on that encoding lib).

what i am suggesting is, we can encode it as i64 and decode it as i64 as well, we check abs_diff without the need for any fallible conversions what so ever. (the suggestion stems from the fact that i64 encodes just like u64 over the network, i.e. if we ever change this to u64 encoding we will still be backward compatible. will have to double check this fact still :) ).

still not sure how the negative side is encoded

Theoretically, it should be fine on encoding but should panic on decoding, which is the main purpose on the decoding side (as negative ones are invalid).

Signed-off-by: onur-ozkan <work@onurozkan.dev>

shamardy

Thanks for the PR! I have a few suggestion in addition to these 2 comments #2255 (comment), #2255 (comment)

mm2src/mm2_p2p/src/behaviours/atomicdex.rs

shamardy · 2024-11-13T19:54:48Z

mm2src/mm2_p2p/src/behaviours/atomicdex.rs

+                if !pre_dial_check(&mut recently_dialed_peers, addr) {
+                    continue;
+                }


Shouldn't this check be part of the filtering above? This is to maintain the intended
connect_bootstrap_num.

If peer is not synced in time, we will keep trying to connect it in the maintain logic, no?

we will keep trying to connect it in the maintain logic, no?

Not really as I suggested to filter them out. For example, if we need 4 peers and have 3 recently dialed ones (to get them in this list this means they were disconnected):

Current: We select 4 and we might get 3 recently dialed ones in the list, then pre_dial_check filters out 3, leaving us with 1 actual attempt
With filtering: We'd select 4 non-recently-dialed peers from the start

While the maintain cycle every 30 seconds can solve this, but we might keep selecting those same 3 recently dialed peers every cycle just to remove them in pre_dial_check, a peer might find it difficult to maintain the minimum needed peers.

Right, makes sense.

shamardy · 2024-11-13T19:57:35Z

mm2src/mm2_p2p/src/behaviours/atomicdex.rs

+                if !pre_dial_check(&mut recently_dialed_peers, &addr) {
+                    continue;
+                }


Same for this, it should be a criteria when choosing random peers. We can even redial some of the recently dialed peers if we can't find enough fresh peers to connect to for this part and the above comment.

Signed-off-by: onur-ozkan <work@onurozkan.dev>

mariocynicys · 2024-11-14T08:53:35Z

mm2src/mm2_main/src/lp_stats.rs

+    let timestamp = common::get_utc_timestamp();
+    encode_message(&timestamp).map_err(|e| e.to_string())


if we don't wanna be explicit with casting, let's at least leave a comment that i64 will encode just as u64.
it's so confusing seeing one type sent from this end and another one received.

I think that explanation alone makes it more complicated than it should be. I didn't think it would be this confusing 🤔. I would rather cast it to u64 before sending it instead of explaining it like "we do this because of encoding numbers..."

me too tbh, i favor explicit casting all the way :/

mariocynicys · 2024-11-14T09:00:59Z

mm2src/mm2_p2p/src/behaviours/atomicdex.rs

+/// Determines if a dial attempt to the remote should be made.
+///
+/// Returns `false` if a dial attempt to the given address has already been made,
+/// in which case the caller must skip the dial attempt.
+fn pre_dial_check(recently_dialed_peers: &mut MutexGuard<TimedMap<StdClock, Multiaddr, ()>>, addr: &Multiaddr) -> bool {


Neither the doc comment nor the func name signify how this function changes the timedmap (only visible from the mutability of the passed map). Let's change this to something like check_and_mark_dialed or something.

Also, no need for the MutexGuard annotation here.

Neither the doc comment nor the func name signify how this function changes the timedmap (only visible from the mutability of the passed map)

"only visible from the mutability of the passed map" that's visible everywhere, both in the function signature and on the caller side!

I struggle to see how check_and_mark_dialed describes the function better than pre_dial_check but whatever, I am okay with it.

Also, no need for the MutexGuard annotation here.

It gives/documents the background context how this map is utilized from caller.

mariocynicys · 2024-11-14T09:09:38Z

mm2src/mm2_p2p/src/behaviours/atomicdex.rs

+                        .any(|addr| pre_dial_check(&mut recently_dialed_peers, addr))
+                        && !connected_relays.contains(peer)


we should move the connected_relays check first as there is no need to maker the peer as dialed then find out we are already connected.

also this should be just a check, it shouldn't mutate the recent dialed peers.

Makes sense

mariocynicys · 2024-11-14T09:12:03Z

mm2src/mm2_p2p/src/behaviours/atomicdex.rs

@@ -824,11 +926,17 @@ fn maintain_connection_to_relays(swarm: &mut AtomicDexSwarm, bootstrap_addresses
                if swarm.behaviour().core.gossipsub.is_connected_to_addr(&addr) {
                    continue;
                }
+
+                if !pre_dial_check(&mut recently_dialed_peers, &addr) {


and here we check the dialing one more time, if the peer had one address and it wasn't dialed yet, we would have marked it as dialed in the filer we did above and we won't be able to connect to it here.

we should also not update the dial expiry if we didn't dial (due to dial check failing)!

and here we check the dialing one more time

Was leftover, thanks.

we should also not update the dial expiry if we didn't dial (due to dial check failing)!

I don't understand what you mean, but if you are saying that we shouldn't add peers to recently dialed peers I think that's exactly what we should do. If we fail to connect some peer, we shouldn't try again for sometime.

I don't understand what you mean, but if you are saying that we shouldn't add peers to recently dialed peers I think that's exactly what we should do. If we fail to connect some peer, we shouldn't try again for sometime.

e.g. for clarification:
we connected to some node, say bootstrap one, marked it as dialed with some expiry, then disconnected for some reason (you name it). Now when we try to connect back to maintain the min_connected, we will find that we dialed this node recently, but instead of just going with our day we would re-mark it is dialed again (even though we didn't dial it) and reset the expiry timer. in a bad situation where we can't find enough peers, the node will just be stuck there forever and ever resetting the timer.

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan added enhancement New feature or request in progress Changes will be made from the author labels Oct 28, 2024

onur-ozkan mentioned this pull request Oct 29, 2024

refactor(P2P): types and modules #2256

Merged

onur-ozkan force-pushed the time-synced-network branch from b675759 to afe2212 Compare October 30, 2024 11:37

add time validation core logic

4343c0e

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan force-pushed the time-synced-network branch from afe2212 to 4343c0e Compare October 31, 2024 08:35

onur-ozkan added 2 commits October 31, 2024 14:17

nit fixes

3b69689

Signed-off-by: onur-ozkan <work@onurozkan.dev>

handle time gap

5acfe8d

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan force-pushed the time-synced-network branch from f9dd830 to 5acfe8d Compare October 31, 2024 11:34

improve logging

82ae6d2

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan force-pushed the time-synced-network branch from da8335c to 82ae6d2 Compare October 31, 2024 11:40

onur-ozkan added 2 commits October 31, 2024 14:51

add more trackable processing logs

5771790

Signed-off-by: onur-ozkan <work@onurozkan.dev>

improve info log and remove debugging leftover

f42e706

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan added the breaking-change might require breaking backward compatibility label Oct 31, 2024

rename NetworkInfoRequest to PeerInfoRequest

b693d8c

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan commented Oct 31, 2024

View reviewed changes

onur-ozkan added 2 commits November 4, 2024 17:09

handle recently dialed peers

92af285

Signed-off-by: onur-ozkan <work@onurozkan.dev>

add useful logs

86eb068

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan force-pushed the time-synced-network branch from 5ba1ac5 to e86a166 Compare November 4, 2024 14:29

create function for pre-dial check

c1a6eaf

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan force-pushed the time-synced-network branch from e86a166 to c1a6eaf Compare November 4, 2024 14:38

onur-ozkan added 5 commits November 4, 2024 17:41

set max cap for timestamp channel

e5c284d

Signed-off-by: onur-ozkan <work@onurozkan.dev>

remove leftover

130af67

Signed-off-by: onur-ozkan <work@onurozkan.dev>

use Multiaddr as key

422626c

Signed-off-by: onur-ozkan <work@onurozkan.dev>

Merge branch 'dev' of github.com:KomodoPlatform/komodo-defi-framework…

3859afc

… into time-synced-network

fix p2p tests

1c7229c

Signed-off-by: onur-ozkan <work@onurozkan.dev>

shamardy added breaking-change: p2p and removed breaking-change might require breaking backward compatibility labels Nov 4, 2024

onur-ozkan added the under review label Nov 5, 2024

onur-ozkan marked this pull request as ready for review November 5, 2024 05:22

onur-ozkan removed the in progress Changes will be made from the author label Nov 5, 2024

onur-ozkan commented Nov 5, 2024

View reviewed changes

update logs

0691aba

Signed-off-by: onur-ozkan <work@onurozkan.dev>

shamardy added the P2 label Nov 11, 2024

mariocynicys reviewed Nov 12, 2024

View reviewed changes

onur-ozkan added 2 commits November 13, 2024 08:49

rename leftovers

3692673

Signed-off-by: onur-ozkan <work@onurozkan.dev>

update timing values

8487ab1

Signed-off-by: onur-ozkan <work@onurozkan.dev>

shamardy reviewed Nov 13, 2024

View reviewed changes

onur-ozkan added 2 commits November 13, 2024 23:27

minor fixes

faeee43

Signed-off-by: onur-ozkan <work@onurozkan.dev>

update pre dial check calls

1f2950e

Signed-off-by: onur-ozkan <work@onurozkan.dev>

mariocynicys reviewed Nov 14, 2024

View reviewed changes

apply nit fixes

9ad57a7

Signed-off-by: onur-ozkan <work@onurozkan.dev>

shamardy removed the P2 label Nov 15, 2024

		let timestamp = common::get_utc_timestamp();
		encode_message(&timestamp).map_err(\|e\| e.to_string())

		.any(\|addr\| pre_dial_check(&mut recently_dialed_peers, addr))
		&& !connected_relays.contains(peer)

feat(p2p): ensure time synchronization in the network #2255

Are you sure you want to change the base?

feat(p2p): ensure time synchronization in the network #2255

Conversation

onur-ozkan commented Oct 28, 2024 • 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

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

onur-ozkan Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laruh commented Nov 6, 2024

onur-ozkan commented Nov 6, 2024

mariocynicys 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

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

shamardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy Nov 13, 2024 • 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

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

onur-ozkan commented Oct 28, 2024 •

edited

Loading

onur-ozkan Nov 13, 2024 •

edited

Loading

shamardy Nov 13, 2024 •

edited

Loading