Istanbul message handler checks whether the sender is a member of the committee #545
Conversation
aidan-kwon
requested review from
ehnuje,
jeongkyun-oh,
KimKyungup,
kjhman21 and
joowon-byun
as code owners
| @@ -71,6 +71,9 @@ func (c *core) handleCommit(msg *message, src istanbul.Validator) error { | |||
| } | |||
|
|
|||
| //logger.Error("receive handle commit","num", commit.View.Sequence) | |||
| if !c.valSet.CheckInSubList(msg.Hash, commit.View, src.Address()) { | |||
| return errNotFromCommittee | |||
| } | |||
|
|
|||
| if err := c.checkMessage(msgCommit, commit.View); err != nil { | |||
There was a problem hiding this comment.
Is it possible to call CheckInSubList inside checkMessage? Don't we need to check if the sender is in the committee for preprepare or processBacklog?
There was a problem hiding this comment.
We don't need to check preprepare message since it only has one valid sender(proposer) at a time and is checked with its own routine.
Because CheckInSubList is not a common routine for all Istanbul message, I think it is better to place CheckInSubList in the current position.
| @@ -202,12 +202,7 @@ func (sb *backend) Gossip(valSet istanbul.ValidatorSet, payload []byte) error { | |||
|
|
|||
| // checkInSubList checks if the node is in a sublist | |||
| func (sb *backend) checkInSubList(prevHash common.Hash, valSet istanbul.ValidatorSet) bool { | |||
There was a problem hiding this comment.
More clear?
| func (sb *backend) checkInSubList(prevHash common.Hash, valSet istanbul.ValidatorSet) bool { | |
| func (sb *backend) checkInCommittee(prevHash common.Hash, valSet istanbul.ValidatorSet) bool { |
There was a problem hiding this comment.
"SubList" is commonly used in overall code instead of "Committee".
I will consider changing all of them later.
There was a problem hiding this comment.
what about remove func (sb *backend) checkInSubList and use valSet.CheckInSubList directly on line 243?
There was a problem hiding this comment.
@laggu Good suggestion. I will consider it later.
|
@andybclee @laggu Please take a look. :) |
Proposed changes
AS-IS
Istanbul message handler checks whether the sender is a validator.
However, it doesn't check whether the sender is a member of the committee.
*Committee is a subset of validators and composed by the block number and the round number.
TO-BE
Istanbul message handler checks whether the sender is a member of the committee.
Types of changes
Please put an x in the boxes related to your change.
Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
$ make test)Related issues
Further comments
If a validator is not a member of the committee, he/she doesn't propagate prepare/commit/roundChange messages, but he/she sends them to eventMux of its Istanbul core. It may result in the mismatch of message count among validators and prevent achieving a consensus.
This issue can be mitigated with this PR by validating the sender in the receiver side. Moreover, we can validate the sender in the sender side also before it sends messages. It will be handled in the following PR.