Peer honesty implementation #2008
Conversation
iulianpascalau
commented
Jun 24, 2020
iulianpascalau
commented
- added peer honesty implementation able to black list a public key if it insists to send bad messages. This is done currently only on consensus topic, other topics will follow.
- consensus now do not count messages in antiflood instance as it does not require to do so due to the improved filtering mechanism and peer honesty tracking process
…f it insists to send bad messages. This is done currently only on consensus topic, other topics will follow. - consensus now do not count messages in antiflood instance as it does not require to do so due to the improved filtering mechanism and peer honesty tracking process
| #for the current settings, a pk will reach to value 0 after maximum 2h (if it stopped misbehaving or stopped sending good messages) | ||
| DecayCoefficient = 0.9779 | ||
| DecayUpdateIntervalInSeconds = 10 | ||
| MaxScore = 100 |
There was a problem hiding this comment.
Nice that we have both positive and negative scores plus something neutral.
| IncreaseCalled func(pk string, topic string, value float64) | ||
| DecreaseCalled func(pk string, topic string, value float64) | ||
| ChangeScoreCalled func(pk string, topic string, units int) | ||
| DecreaseCalled func(pk string, topic string, value float64) |
There was a problem hiding this comment.
Perhaps DecreaseCalled can be removed now?
| // ChangeScore - | ||
| func (phhs *PeerHonestyHandlerStub) ChangeScore(pk string, topic string, units int) { | ||
| if phhs.ChangeScoreCalled != nil { | ||
| phhs.ChangeScoreCalled(pk, topic, units) |
There was a problem hiding this comment.
Not sure why the return statement below.
There was a problem hiding this comment.
done, remoed
node/mock/peerHonestyHandlerStub.go
Outdated
| phhs.IncreaseCalled(pk, topic, value) | ||
| return | ||
| } | ||
| ChangeScoreCalled func(pk string, topic string, units int) |
There was a problem hiding this comment.
It's possible to move the stubs to testscommon if they are identical - although sometimes we do not want that.
process/errors.go
Outdated
| @@ -808,3 +808,24 @@ var ErrInvalidUserNameLength = errors.New("invalid user name length") | |||
|
|
|||
| // ErrTxValueOutOfBounds signals that transaction value is out of bounds | |||
| var ErrTxValueOutOfBounds = errors.New("tx value is out of bounds") | |||
|
|
|||
| // ErrNilBlackListedPkCache signals that a nil black listed public key cache | |||
There was a problem hiding this comment.
signals that - comment is missing the predicate.
| return | ||
| } | ||
|
|
||
| if pph.blackListedPkCache.Has(ps.pk) { |
There was a problem hiding this comment.
So we do not extend the stay in the black list period for already-blacklisted peers even if shouldBlacklist == true, right? Well, it makes sense since a blacklisted peer can't misbehave anymore anyway - at least not with effect (I am just thinking out loud).
There was a problem hiding this comment.
exactly, since this is checked on each 10 seconds or so, it will prolong the blacklisted status beyond easy understanding of this mechanism
| // createMockPeerHonestyConfig creates a peer honesty config with reasonable values | ||
| func createMockPeerHonestyConfig() config.PeerHonestyConfig { | ||
| return config.PeerHonestyConfig{ | ||
| DecayCoefficient: 0.9995, |
There was a problem hiding this comment.
I know I am exaggerating here, but can you use the same value as the one from config.toml perhaps?
| assert.Nil(t, err) | ||
| } | ||
|
|
||
| func TestP2pPeerHonesty_Close(t *testing.T) { |
There was a problem hiding this comment.
This test is not very short (in duration) - just a remark.
There was a problem hiding this comment.
I know, shortened
| @@ -0,0 +1,418 @@ | |||
| package peerHonesty | |||
There was a problem hiding this comment.
👍 Tests cover everything with the exception of impossible cases (bad casts etc.).
| pph.Put(pks[idx], topic, initial[idx]) | ||
| } | ||
|
|
||
| pph.applyDecay() |
There was a problem hiding this comment.
Perhaps add an extra test that calls applyDecay a lot of times, just to assert that all scores go to zero?
…ndNetwork/elrond-go into peer-honesty-implementation
iulianpascalau
dismissed stale reviews from andreibancioiu and sasurobert
via
9a24c20
