Added checks to avoid decoding messages as handshakes

[kishansagathiya]

Changes

These changes would show the appropriate error messages instead of genesis mismatch and also avoid banning the other peer.

Tests

go test -tags integration github.com/ChainSafe/gossamer

Issues

#2636

Primary Reviewer

@timwu20

[codecov]

Codecov Report

Merging #2681 (b76420f) into development (4f311a5) will increase coverage by 0.05%.
The diff coverage is 78.57%.

@@               Coverage Diff               @@
##           development    #2681      +/-   ##
===============================================
+ Coverage        62.93%   62.98%   +0.05%     
===============================================
  Files              212      212              
  Lines            27020    27038      +18     
===============================================
+ Hits             17004    17029      +25     
+ Misses            8472     8461      -11     
- Partials          1544     1548       +4     

[kishansagathiya]

Making roles enum does not fix the problem, i still have to check if it is 1, 2 or 4.

[qdm12]

Making roles enum does not fix the problem, i still have to check if it is 1, 2 or 4.

For sure, you can have them as typed byte set to those values (1,2,4), enums won't work (especially since 3 is missing lol)

[qdm12]

nit do you think we could have the cfg.Core.Roles directly as common.Roles type?

[kishansagathiya]

with latest commit, roles are getting used everywhere instead of byte. I have propagated these changes til toml config. I would not like to change toml config, since that's what deals with marshalling and marshalling config. I don't want to disturb any behaviour there.

[qdm12]

Having a byte alias probably won't change config parsing. Don't we have unit tests to verify this?

[qdm12]

Should we then remove the , ok in hs, ok := msg.(Handshake) so it panics in case it ever happens?

[kishansagathiya]

Arhhmm, I am not really sure about that. I would prefer not to remove this.

I feel like NotificationMessage and Handshake happens to have the same interface at the moment, but they don't have to. Also, not a big fan of panicking.

[qdm12]

Then let's remove that comment.

Panicing is useful for programming errors, so it doesn't go unnoticed in some error log line.
I also was team anti-panic, but it really has its use for programming errors (not from user/IO of course)

[EclesioMeloJunior]

LGTM!

[qdm12]

Making roles enum does not fix the problem, i still have to check if it is 1, 2 or 4.

Just so you know, you could use _ to skip 3 and have enums using iota.

const (
    enum1 uint8 = iota // that's 0
    _
    enum2 // that's 2
)

But I prefer to just hardcode the constant as it is now, so really just a shower thought

[github-actions]

🎉 This PR is included in version 0.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀