Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Peer Reputation for syncing #943

Open
4 of 15 tasks
Voxelot opened this issue Jan 26, 2023 · 8 comments
Open
4 of 15 tasks

Peer Reputation for syncing #943

Voxelot opened this issue Jan 26, 2023 · 8 comments
Assignees

Comments

@Voxelot
Copy link
Member

Voxelot commented Jan 26, 2023

The plan is to first come up with an approach how to handle Peer Reputation.
That includes, but not limited to:

  • ban & its consequences (potential holes in the approach, different attacks etc)
  • peer scoring (both positive and negative?)
  • defining implementation details

Once we've made a decision on the approach, and defined implementation details then we can start working on it.

Offense Affects Internal Node? Severity Explanation Completing PR
Bad Block Height No High Peering is useless if the values provided don't match what is advertised by peer #1331
Bad Header No High " #1331
Bad Transactions No High " #1331
Bad Block No High " #1331
Heartbeats Stopped No Medium This is concerning but doesn't guarantee the peer is unreachable. It's best to know that peer at least claims to be available #1356
Slow Heartbeats No Low This is helpful in selecting a peer that is most available, but shouldn't be a reason to ban a member unless it gets really bad. #1356
Incorrect Response Data No High We should avoid peers that give us bad data. There aren't really situations where a friendly node would be sending bad data on accident, and if they were we might want to cut the connection anyway
Too Many Requests No Medium This is a serious attack vector, but it's not obvious how we might define "too much". It also depends on the validity of the txs. Too many alwasy can cause problems, but if they are all valid, then its not as punishable
Weak Connection No Low Unclear what this means. This might overlap with "Slow Heartbeats" slightly
Request Timeout Almost No Medum This is more likely to happen with a friendly peer than getting bad data, but in some ways is more harmful as it necessarily takes longer and is just as detrimental to the performance of a node. #1346
Gossiping bad transactions No Low Malicious peer broadcasts invalid transactions to the network.

Rewardable Actions:

Current Progress:

@xgreenx
Copy link
Collaborator

xgreenx commented Jan 31, 2023

It would be the main issue to track the progress and discuss how the reputation system should work. I assign this issue to @leviathanbeak because he is the main keeper of all the P2P stuff=) But it is a critical part of the blockchain, and almost everyone from the @FuelLabs/client should review it/participate in the discussion.

The first step is to define future steps and fill in the issue description. @leviathanbeak Could you handle it, please?

Maybe we need to investigate potential reputation systems and describe/compare them here.

@freesig
Copy link
Contributor

freesig commented Feb 2, 2023

Maybe this would be worth a notion document? Also we could try out github discussions although I've never actually used that.
I have personally found using mermaid js in a sequence diagram quite nice for writing specs recently. I think the actual mermaid code is quiet readable but as a bonus you get a diagram.

@freesig
Copy link
Contributor

freesig commented Feb 2, 2023

One thing that I think we should really keep in mind during this design though is the fundamental limit that anything we do will only be tied to a key pair. If key pairs are cheap / easy to make (as in we don't add any additional requirements to join the network) then this is a very low bar for entry. It's worth keeping this in mind before we invest too much energy into these kinds of measures.

@leviathanbeak
Copy link
Contributor

I started a Page in Notion.

If you have some comments, if you disagree with something, you can leave comments, update the document etc

If you have a completely different idea on how to approach it, then maybe creating different page within Peer Reputation page would be better. So that we can keep and compare the ideas.

@xgreenx
Copy link
Collaborator

xgreenx commented Feb 22, 2023

@leviathanbeak Thank you for the notion document!=)

I have questions and suggestions regarding the description.

  • It is not clear how the internal gossipsub score is calculated. Could you describe it or put a link if it is a part of the libp2p?

  • I think the Minor and Major reports would not be enough to cover all use cases because each operation/action has a different price(consuming a different amount of resources). For example, the sharing of known peers is not an expensive operation, and we can do a lot of them simultaneously. The transaction verification is more costly because it requires us to run validation logic on it, and we can't do many of them simultaneously. Block verification is much more expensive, and so on.
    I like the substrate's approach to having a separate price for each operation and using i32 as a cumulative value(also, their document contains some good ideas that are worth thinking about too).
    Also, I think it would be nice to specify all places where we need to do scoring in the notion. In this case, we will have a clear understanding of the flow. Based on the flow, we can find what interactions are critical and decisions regarding the price.

  • Also, maybe we want a stale reputation mechanism(when the node's reputation decreases with time because of no interactions). It will give a connection rotation with useless peers.

  • Do we want to flush/load the reputation on shutdown/startup? Or do we want to start from scratch and calculate it in the runtime?

  • Why do we need to track app scoring separately? Maybe it can be part of the inner scoring of the P2PService?

@leviathanbeak
Copy link
Contributor

Hey @xgreenx

  • It is not clear how the internal gossipsub score is calculated. Could you describe it or put a link if it is a part of the libp2p?
  • I will write a better explanation in Notion probably and will link it here
  • I think the Minor and Major reports would not be enough to cover all use cases because each operation/action has a different price(consuming a different amount of resources). For example, the sharing of known peers is not an expensive operation, and we can do a lot of them simultaneously. The transaction verification is more costly because it requires us to run validation logic on it, and we can't do many of them simultaneously. Block verification is much more expensive, and so on.
  • Good point, that was the example of the approach, where we should abstract the point deduction calculation from other services by having a simple enum, but it can definitely hold more fields than just Minor and Major we can give it as many levels as we want - this is where we work together on deciding on it, this framework just provides the initial push/direction.

I like the substrate's approach to having a separate price for each operation and using i32 as a cumulative value(also, their document contains some good ideas that are worth thinking about too).

  • I will look at it, I went with floats because gossipsub uses floats, but I'm definitely open to what is best for us.

Also, I think it would be nice to specify all places where we need to do scoring in the notion. In this case, we will have a clear understanding of the flow. Based on the flow, we can find what interactions are critical and decisions regarding the price.

  • I agree, I can start with this but definitely need the input/feedback from all of you as well.
  • Also, maybe we want a stale reputation mechanism(when the node's reputation decreases with time because of no interactions). It will give a connection rotation with useless peers.
  • Gossipsub has a decay process, we can implement something for our own reputation, I think it would be better we do that in separate PR from the current one. But definitely good idea.
  • Do we want to flush/load the reputation on shutdown/startup? Or do we want to start from scratch and calculate it in the runtime?
  • This would be a team decision, I'd definitely go for persisting to something, it would be a lot of valuable info lost if we ditched everything on restart.
  • Why do we need to track app scoring separately? Maybe it can be part of the inner scoring of the P2PService?
  • Not sure I understood this completely, but let me try to answer, I implemented the Peer Scoring constructs outside of p2p module so that they can be imported to other modules - consequently the logic of deciding peer score deduction and similar was included there. But the tracking of peer scoring is definitely within P2PService inside PeerManager behaviour.

@leviathanbeak
Copy link
Contributor

Here's the link to Gossipsub calculation process.

leviathanbeak added a commit that referenced this issue Apr 15, 2023
The approach is mostly detailed in
[Notion](https://www.notion.so/fuellabs/New-Proposal-96da9de6609b4faf94c30ae6bd355bc4).

These are the first steps of our Peer Reputation approach, there are
certain _todos_ left, could be added later on or if needed I can add
them with this PR.

Todo:
- [x] peer reputation decay with time
- [ ] implement unban peers after certain period
- [x] reserved peers should not be banned/scored ? - this is kind of
"implemented" by only updating the score of non-reserved peers


Ref #943

---------

Co-authored-by: ControlCplusControlV <44706811+ControlCplusControlV@users.noreply.github.com>
Co-authored-by: green <xgreenx9999@gmail.com>
Co-authored-by: Brandon Kite <brandonkite92@gmail.com>
MitchTurner added a commit that referenced this issue Aug 31, 2023
First implementation of #943

During block import, if the peer gives incorrect values, dock their peer
score. If they successfully report a block, improve their peer score.
@MitchTurner
Copy link
Member

MitchTurner commented Sep 5, 2023

The notion doc references a bunch of cases that aren't covered yet:

enum NegativeScore {

  // Heartbeat protocol
	BadBlockHeight // Invalid block height from heartbeat
  SlowHeartbeats // Irregular heartbeats / connection errors out often etc

  // Request / Response Protocol
	IncorrectResponseData // deserialization failed
	TooManyRequests // single peer makes way too many requests in short period
	BadTransactions // Invalid/Incorrect Txs
	BadBlock // Invalid/Incorrect block sent  
	RequestTimeout // Do we have timeouts? We need to handle case of no response
  BadHeader // Invalid/Incorret header

  // PeerInfo - behaviour of nodes
  WeakConnection // node keeps disconnecting
}

and

enum PositiveScore {

	// Heartbeat protocol
  ReuglarHeartbeats // heartbeats always on time, after N amount of successful heartbeats increase reputation
	
	// Request / Response protocol
	ValidResponse // small increase of reputation on every valid response
	QuickResponse // keep cache of top 5 fastest response, if it's a new top 5 response, increase reputation
    
  // PeerInfo - behaviour of nodes
  StrongConnection // rarely disconnects
}

All the things related to the importer, we've done, but things that happen at a lower level, like deserialization, response time, connection strength, etc, need some thinking.

crypto523 pushed a commit to crypto523/fuel-core that referenced this issue Oct 7, 2024
The approach is mostly detailed in
[Notion](https://www.notion.so/fuellabs/New-Proposal-96da9de6609b4faf94c30ae6bd355bc4).

These are the first steps of our Peer Reputation approach, there are
certain _todos_ left, could be added later on or if needed I can add
them with this PR.

Todo:
- [x] peer reputation decay with time
- [ ] implement unban peers after certain period
- [x] reserved peers should not be banned/scored ? - this is kind of
"implemented" by only updating the score of non-reserved peers


Ref FuelLabs/fuel-core#943

---------

Co-authored-by: ControlCplusControlV <44706811+ControlCplusControlV@users.noreply.github.com>
Co-authored-by: green <xgreenx9999@gmail.com>
Co-authored-by: Brandon Kite <brandonkite92@gmail.com>
crypto523 pushed a commit to crypto523/fuel-core that referenced this issue Oct 7, 2024
First implementation of FuelLabs/fuel-core#943

During block import, if the peer gives incorrect values, dock their peer
score. If they successfully report a block, improve their peer score.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants