lib/grandpa: verify block justification when handling FinalizationMessage #1084
Conversation
lib/grandpa/errors.go
Outdated
| @@ -72,5 +72,8 @@ var ErrNotFinalizationMessage = errors.New("cannot get finalized hash from VoteM | |||
| // ErrNoJustification is returned when no justification can be found for a block, ie. it has not been finalized | |||
| var ErrNoJustification = errors.New("no justification found for block") | |||
|
|
|||
| // ErrMinVotesNotMet is returned when the number of votes is less than the required minimum | |||
There was a problem hiding this comment.
could you add in a Justification?
lib/grandpa/message_handler.go
Outdated
| } | ||
|
|
||
| // confirm total # signatures >= 2/3 of number of voters | ||
| if !(sigCount >= (2/3)*len(h.grandpa.state.voters)) { |
There was a problem hiding this comment.
you could use h.grandpa.state.threshold() instead of (2/3)*len(h.grandpa.state.voters)
There was a problem hiding this comment.
good idea, this also fixes the issue you pointed out with the test.
lib/grandpa/message_handler_test.go
Outdated
| @@ -177,6 +177,57 @@ func TestMessageHandler_FinalizationMessage_NoCatchUpRequest(t *testing.T) { | |||
| cm, err := fm.ToConsensusMessage() | |||
| require.NoError(t, err) | |||
|
|
|||
| h := NewMessageHandler(gs, st.Block) | |||
| out, err := h.HandleMessage(cm) | |||
| require.EqualError(t, err, "signature is not valid") | |||
There was a problem hiding this comment.
change to require.Equal(t, err, ErrInvalidSignature)?
|
|
||
| func TestMessageHandler_FinalizationMessage_NoCatchUpRequest_ValidSig(t *testing.T) { | ||
| st := newTestState(t) | ||
| voters := newTestVoters(t) |
There was a problem hiding this comment.
hm, something in this test seems off, newTestVoters should return 9 votes, so a finalization message with only 1 signature shouldn't meet the threshold, maybe I am missing something?
There was a problem hiding this comment.
No, I fixed verify so this test fails, going to add tests...
lib/grandpa/message_handler.go
Outdated
| // verify signature | ||
| msg, err := scale.Encode(&FullVote{ | ||
| Stage: precommit, | ||
| Vote: NewVote(j.Vote.hash, j.Vote.number), | ||
| Round: fm.Round, | ||
| SetID: h.grandpa.state.setID, | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| pk, err := ed25519.NewPublicKey(j.AuthorityID[:]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| ok, err := pk.Verify(msg, j.Signature[:]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if !ok { | ||
| return ErrInvalidSignature | ||
| } | ||
|
|
||
| // verify authority in justification set | ||
| authFound := false | ||
| for _, auth := range h.grandpa.Authorities() { | ||
| if reflect.DeepEqual(auth.Key.AsBytes(), j.AuthorityID) { | ||
| authFound = true | ||
| break | ||
| } | ||
| } | ||
| if !authFound { | ||
| return ErrVoterNotFound | ||
| } |
There was a problem hiding this comment.
it might also be nice to break this into a separate function that accepts *Justification, stage, vote, round, and setID, since it'll be used for catch up response handling as well
Changes
added function
verifyJustificationwhich check given FinalizationMessage and confirms:Tests
Checklist
Issues