Add tracking for peer heartbeats

[MitchTurner]

#1348

In order to track peer heartbeats, this PR adds two new things:

• A concept of a rolling/moving average for the time between heartbeats for each peer
  • This is updated every time a heartbeat is sent from that peer
• A regular timeout in the P2P Task that will check peers for good heartbeat behavior

Originally I was looking at just reporting on heartbeat, but if a peer never reported then they would be exempt from checks

There are 3 2 cases in which we will want to punish peers for their heartbeat behavior:

• They have never sent us a heartbeat (If we treat the setup as the first heartbeat, then we can merge this with the next 👇)
• They haven't sent us a heartbeat in a long time
• They are sending heartbeats very sporadically

We can also use this data to determine which peers are performing the best

[MitchTurner]

Still need to:

• Parameterize the rates at which heartbeats are checked, and possibly the window size for the heartbeat rolling average?
• Actually implement the criteria for punishing the peers in peer_heartbeat_reputation_checks

I don't really know what any of these values should be and probably need to discuss with the team.

[MitchTurner]

This is a perfect example of why we need this refactor:
#1354

It's difficult to test the surfaces of the Task to know that punishments are being divvied out correctly.

Ideally, we could set the timeout really low, and the threshold for punishment really low, and then manually start the task and see that our fake p2p manager is receiving the report. We are bound to concrete shared so we don't have access to what is being sent.

[bvrooman]

I think the overall idea makes sense. You mentioned two criteria for which we punish peer scores:

1. They haven't sent us a heartbeat in a long time
2. They are sending heartbeats very sporadically

I imagine that for 1., you calculate the time since the last heartbeat and check if it surpasses a threshold.

For 2., is this where the moving average comes in to play? Do we punish the peer if the time since last heartbeat falls significantly below/above the moving average? We may want to introduce a target average as to discourage scenarios where the average is unsustainably low or high.

It would be nice to consolidate, if we haven't already, on the architecture for introducing these kinds of "rules" for peer score, i.e. a way to plug inputs in to an extensible rule engine.

[MitchTurner]

I imagine that for 1., you calculate the time since the last heartbeat and check if it surpasses a threshold.

Yes. This already exists in the peer_heartbeat_reputation_checks method, which gets called on a regular timeout by the select!. And maybe that's enough.

For 2., is this where the moving average comes in to play?

That's the idea right now. The idea originally was to check on every received heartbeat how that peer was doing. They would be scored based on how often they send heartbeats and the moving average was to introduce some leniency.

This sounds great to me, except for the case were we find a new peer and then never get any heartbeats from them again. So I realized we need some independent peer watching, which is where the peer_heartbeat_reputation_checks came in.

This all seems like it could be simplified some. Maybe we actually get rid of #1 checks? Since if they are taking a long time then the average will catch it.

It would be nice to consolidate, if we haven't already, on the architecture for introducing these kinds of "rules" for peer score, i.e. a way to plug inputs in to an extensible rule engine.

I wish, but peer scoring is kinda scattered around the code a bit. There isn't one place where we can catch all the behavior. For example, we can't capture any of the sync misbehavior checks in the P2P code since we don't have access to it. For now we just expose the report_peer method that other services can have a shared handle to.

[Voxelot]

This all seems like it could be simplified some. Maybe we actually get rid of #1 checks? Since if they are taking a long time then the average will catch it.

Simple is better

[MitchTurner]

Oh. I remember why I included the "time since last heartbeat" check. It's because the peer could have had a great average, but then dropped. This is the way to catch if the peer is down.

[Voxelot]

shouldn't it be clear enough from the method signature that it's an iter?

[MitchTurner]

I don't feel strongly about it. I just don't think that someone would expect an iterator from that name.

[Voxelot]

lgtm - tested the moving avg code and it seems to work 👍🏻

[Voxelot]

minor nit: shared is conventional for most of our tasks in fuel core

[MitchTurner]

It is still represented in the Task trait assoc type. I'm not a fan of having a field called shared because it doesn't communicate at all what it's for and why it's grouped. shared implies to me that it's for misc data and anyone extending it might just throw it in there. Whereas the trait assoc type communicates that it would probably be shared with other Tasks.

I was hoping to move away from that name :)

[Voxelot]

In this case I think it's fine, since this task is just mimicking the form, but not rigidly adhering to the standardized task framework from fuel-core-services. However, changing it in the broader scope will require a more comprehensive plan to improve.

[MitchTurner]

lgtm - tested the moving avg code and it seems to work 👍🏻

I was considering adding some tests for this but my natural inclination is to not test internal apis. Maybe this is a good exception because we don't have a clear boundary. Might add that before merging.

[MitchTurner]

lgtm - tested the moving avg code and it seems to work 👍🏻

I was considering adding some tests for this but my natural inclination is to not test internal apis. Maybe this is a good exception because we don't have a clear boundary. Might add that before merging.

I think I caught some bugs, so I good thing I checked.