[consensus] remove duplicated signature verification

[LeoHChen]

Check the following issue on the detailed explanation of why this commit makes sense.
#3225

The onPrepare and onCommit will do signature verification of the message payload.
So, there is no need to do duplicated sanity check when handling Prepare/Commit messages.

We reserve the signature verification on other messages, as they are not duplicated.
This PR can reduce the CPU load of leader during consensus.

Signed-off-by: Leo Chen leo@harmony.one

[gupadhyaya]

big assumption, but intention looks correct. however, I think a malicious validator can fool the leader in checkDoubleSign validation. here is how: a malicious validator will modify the commit message to make it look like double sign and send it on behalf of some other validator. since we didn't check that commit message was actually from the claimed sender (note that this part comes later, checkDoubleSign is before that), now the leader will falsely mark a genuine validator as double signing validator, hence banning him.

may be moving the checkDoubleSign after the verifyHash could fix this issue. but needs a careful look.

[LeoHChen]

big assumption, but intention looks correct. however, I think a malicious validator can fool the leader in checkDoubleSign validation. here is how: a malicious validator will modify the commit message to make it look like double sign and send it on behalf of some other validator. since we didn't check that commit message was actually from the claimed sender (note that this part comes later, checkDoubleSign is before that), now the leader will falsely mark a genuine validator as double signing validator, hence banning him.

may be moving the checkDoubleSign after the verifyHash could fix this issue. but needs a careful look.

Thanks for the comments and good insight, I didn't look closely on the double-sign checking though. Let me dive deeper.

[LeoHChen]

@gupadhyaya , @rlan35 , please review again.

[rlan35]

instead of not doing any sanity check on p2p message with signatures, we should try explore p2p level message checking to make sure it's not tampered. P2p

[gupadhyaya]

@LeoHChen VerifyHash is not sufficient to guarantee that the tampered receive message will have no harm. It is still possible to make the leader think that a genuine validator has double signed by keeping the blockHash and payload intact, but tampering the blockNum and viewID (checkDoubleSign lines 25&26). I suggest carefully evaluating all fields of the received message to see the impact of tampering and making the fixes.

[rlan35]

I just did a round of checking on the p2p message fields and how they are used. Seems for leader, this change will prevent potential problems. But for validators, there are cases that can cause issue:

Here the BlockNum is trusted and used to check sync status. Here we should use the BlockNum associated with the actual block (blockObj.NumberU64())

And for other parts, mostly the issue is with ViewID, since we have view change, so the sender commit to a viewID by signing on it. Before, we rely on the signature on the p2p message to commit to the viewID. So if we remove it, attacker can mess up the viewID of messages and cause confusion.

[rlan35]

And for validator OnAnnounce. The message fields are directly used for serious checks that may trigger view change. e.g in onAnnounceSanityChecks.

Actually for OnAnnounce, since there is no block data to check (block data is in Prepared message). The p2p message signature is the only verification to verify it's the real leader. We should keep the sig checking for onAnnounce then.

[rlan35]

And for validator OnAnnounce. The message fields are directly used for serious checks that may trigger view change. e.g in onAnnounceSanityChecks.

Actually for OnAnnounce, since there is no block data to check (block data is in Prepared message). The p2p message signature is the only verification to verify it's the real leader. We should keep the sig checking for onAnnounce then.

Nevermind these comments. The sig checks are only removed for leader logic. Then it looks good.

[rlan35]

@LeoHChen VerifyHash is not sufficient to guarantee that the tampered receive message will have no harm. It is still possible to make the leader think that a genuine validator has double signed by keeping the blockHash and payload intact, but tampering the blockNum and viewID (checkDoubleSign lines 25&26). I suggest carefully evaluating all fields of the received message to see the impact of tampering and making the fixes.

blockNum and ViewID are checked in isRightBlockNumAndViewID, and BlockHash are verified with the sig, so they are all good.