Skip to content
This repository has been archived by the owner on Apr 13, 2022. It is now read-only.

Fix "last seen date" for connected peers api route. #141

Closed
terjokhin opened this issue Dec 26, 2017 · 8 comments
Closed

Fix "last seen date" for connected peers api route. #141

terjokhin opened this issue Dec 26, 2017 · 8 comments
Labels

Comments

@terjokhin
Copy link
Contributor

Currently api route /peers/connected returns list of objects with lastSeen field. This field is being filled with now() timestamp instead of correct last seen date. However class Handshake has its own time field. Maybe we should consider use this field instead of now()?

@terjokhin
Copy link
Contributor Author

@ceilican What do you think about it? I can easily implement this changes, but firstly I need to swagger-api branch to be merged.

@terjokhin terjokhin self-assigned this Dec 26, 2017
@ceilican
Copy link
Contributor

It sounds reasonable to me. @kushti implemented the handshaking only recently, and I guess the population of lastSeen with now() precedes Handshake. What do you think, @kushti?

@Tolsi
Copy link
Contributor

Tolsi commented Dec 26, 2017

I think if you see this peer right now, then you should return now as last time :)

@ceilican
Copy link
Contributor

In principle, I think lastSeen should be neither now() nor the handshake time, but rather something between the two: the last time we have interacted with the peer (e.g. the last time we have received a modifier (or anything else) from the peer).

Both now() and the handshake time are just approximations of the true lastSeen time.

Do we currently have a way to track the true last time we interacted with the peer? If not, we could consider implementing it.

If we need to choose between the now() and handshake time approximations, I think I prefer the handshake time, because it is a conservative estimation: we are sure that we have seen the peer at the handshake moment, but we are not sure that we have seen the peer now. On the other hand, now may be a more precise estimation.

@terjokhin
Copy link
Contributor Author

Now() is just a time when we request 'connected peers' api, it hasn't any relation to real last seen date. On the other hand it's truly non-deterministic behaviour when you try to test routes with now() cause it's nearly impossible to predict what we will finally get from api. One more concern here is about that Http layer has influence on a result. I think there should not be any code inside route definition other than checking input data, asking inner services to respond, forming service's answer into response. And last, at some moment in the past handshake time was the true last seen date =) I think we need to consider implement this 'lastSeen' or remove it from api completely, cause it's really confusing. Or just move this now() generation into service instead of route.

@kushti
Copy link
Contributor

kushti commented Jan 31, 2018

Updating some field on every message could be expensive(we need to send a message to PeerManager on such an event). As an alternative, we can update PeerManager on certain kind of message to be got over wire. In Scorex we dont' have Ping/Pong messages (unlike, say, Ethereum network protocol). However, we have other pairs of messages, GetPeers/Peers and Sync/Sync to be exchanged regularly (we are not sending such requests to all the peers at the same time though), so let's use them to update lastSeen field (i.e. lets update the field on receiving one of these messages).

@terjokhin
Copy link
Contributor Author

@kushti @ceilican I think in this case this field shouldn't be called lastSeen. Cause in my opinion last seen date means the time when we got last messages from peer. I think we should rename this field to be handshakeTime or something like this and keep this first handshake timestamp as a value for this field. Cause for now using 'now()` is meaningless. I can do it on client instead. Or we can just throw it away.

@terjokhin terjokhin removed their assignment Aug 31, 2018
kushti added a commit that referenced this issue Oct 22, 2020
Fix for #141: updates for last seen & dropping inactive connections
@kushti
Copy link
Contributor

kushti commented Oct 25, 2020

New issue: #378

@kushti kushti closed this as completed Oct 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants