feat(dot/peerset): Implement peer scoring

[arijitAD]

Changes

• Implement Peerset
• Implement peer state manager
• Implement Network behaviour
• Integrate with gossamer
• Add test case

Tests

go test github.com/ChainSafe/gossamer/dot/peerset -v 

Issues

• closes research and implement scoring for network sync module #1399

Primary Reviewer

• @noot

[timwu20]

Suggested change

	gossipSuccessReason = "SuccessFull gossip"
	gossipSuccessReason = "Successful gossip"

[arijitAD]

Done

[timwu20]

nit:

Suggested change

	func (h *Handler) GetMessages() chan interface{} {
	func (h *Handler) Messages() chan interface{} {

[arijitAD]

Done

[timwu20]

nit:

Suggested change

	func (h *Handler) GetReputation(peerID peer.ID) (int32, error) {
	func (h *Handler) Reputation(peerID peer.ID) (int32, error) {

[arijitAD]

Done

[timwu20]

nit:

Suggested change

	func (h *Handler) GetSortedPeers() chan interface{} {
	func (h *Handler) SortedPeers() chan interface{} {

[arijitAD]

Done

[timwu20]

any specific reason why you don't expose these attributes.

[arijitAD]

Made the variable public.

[timwu20]

can we make this configurable?

[arijitAD]

Done

[timwu20]

making a custom int32 type with .Add(v maxInt32) and .Sub(v maxInt32) receiver functions would be nice.

[arijitAD]

Done

[arijitAD]

basically looks good, about to approve, please just check the core.HandleTransactionMessage comment and probably un-parallelize network tests as it looks like some are failing now

Done

Updated the error checking in core.HandleTransactionMessage

[noot]

nice work :D

[qdm12]

The t.Parallel() is still no there, but I guess it's because we can't run this in parallel?

[qdm12]

nit we should wait for goroutines to finish on the calling layer. This might require a lot of code changes, so this is more of a awareness suggestion, not needed to do here. Basically each function running as a goroutine should have a ctx and done channel as arguments which the caller handles. For example canceling the context doesn't mean the function in the goroutine is done, it might take some extra time for it to finish.

Suggested change

	func (s *Service) startProcessingMsg() {
	msgCh := s.host.cm.peerSetHandler.Messages()
	for {
	select {
	case <-s.ctx.Done():
	return
	func (s *Service) startProcessingMsg(done chan<- struct{}) {
	defer close(done)
	msgCh := s.host.cm.peerSetHandler.Messages()
	for {
	select {
	case <-s.ctx.Done():
	return

[arijitAD]

Yes, For other packages this is a good suggestion that we should flush/process the entire channel before closing the go routine. But in the case of peer scoring, it's okay to not process all the data since the peer scoring will be impacted on restart.

We cannot parallelize the tests since we will have a port conflict issue.

[qdm12]

That's another aspect, yes. I was more talking about waiting for threads (goroutines) to exit, a bit like a Join operation if you wish. I even wrote a library goshutdown a few months back to handle shutdown of goroutines gracefully, although that's more focused for the top level / main function of the program.

[arijitAD]

This looks interesting. I will explore it.

[qdm12]

If we wrap the bottom error only (aka fmt.Errorf("context: %w", err)), we should export these (and all defined errors pretty much).

[qdm12]

nits ...

• rename set to configs. set is rather generic and in my view a set is unordered unlike a slice. Also rename infoSet to infoList for example. You may want to rename Set* to List* in other parts.
• assign to slice using index directly
• remove Go zero value field initialisation

Suggested change

	func NewPeerState(set []*config) (*PeersState, error) {
	if len(set) == 0 {
	return nil, errConfigSetIsEmpty
	}
	infoSet := make([]Info, 0, len(set))
	for _, cfg := range set {
	info := Info{
	numIn: 0,
	numOut: 0,
	maxIn: cfg.inPeers,
	maxOut: cfg.outPeers,
	noSlotNodes: make(map[peer.ID]interface{}),
	}
	infoSet = append(infoSet, info)
	}
	func NewPeerState(configs []*config) (*PeersState, error) {
	if len(configs) == 0 {
	return nil, errConfigSetIsEmpty
	}
	infoList := make([]Info, len(set))
	for i, cfg := range set {
	infoList[i] = Info{
	maxIn: cfg.inPeers,
	maxOut: cfg.outPeers,
	noSlotNodes: make(map[peer.ID]interface{}),
	}
	}

[arijitAD]

Done

[qdm12]

nit please make this a global error

[arijitAD]

Done

[qdm12]

nit maybe add a default case returning an error in case we add a new state in the future? Or change that last case to default:

[arijitAD]

Changes it to default.

[qdm12]

Can you run all these tests in parallel maybe?

[arijitAD]

Done

[qdm12]

nit Can these be constants?

[arijitAD]

Done

[timwu20]

@arijitAD is this still an open question?

[timwu20]

do we need to keep these function signatures since this implements an interface?

[arijitAD]

I have added this check for error checking

if errors.Is(err, runtime.ErrInvalidTransaction)

[arijitAD]

I think we need to keep it otherwise interface will not be implemented.

[timwu20]

yea it's a best practice to avoid having contexts as a struct attribute. I think this will involve a larger refactor though.

[timwu20]

nit:

Suggested change

	type PeerAdder interface {
	type PeerAdd interface {

[arijitAD]

Done

[timwu20]

nit:

Suggested change

	type PeerRemover interface {
	type PeerRemove interface {

[arijitAD]

Done

[timwu20]

nit:

Suggested change

	type PeerGetter interface {
	type Peer interface {

[arijitAD]

Done

Update the code

[github-actions]

🎉 This PR is included in version 0.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀