chore (pkg/scale): Integrate scale into grandpa library #1694
Conversation
|
Waiting to open this until scale network PR is merged |
jimjbrettj
changed the base branch from
development
to
jimmy/integrateScaleNetwork
jimjbrettj
requested review from
arijitAD,
EclesioMeloJunior,
edwardmack,
noot and
timwu20
as code owners
| vm := &VoteMessage{} | ||
| err = vm.Decode(r) | ||
| m = vm | ||
| err = scale.Unmarshal(msg.Data, &gossipMessage) |
There was a problem hiding this comment.
we need to check this error, right?
There was a problem hiding this comment.
I have it below the switch, but I can move it up if that's better
There was a problem hiding this comment.
would be nice to check the error before the switch, will be easier to relate the error to the Unmarshal
There was a problem hiding this comment.
Yes please handle the error immediately
| @@ -842,7 +843,7 @@ func (s *Service) finalise() error { | |||
| return err | |||
| } | |||
|
|
|||
| pcj, err := newJustification(s.state.round, bfc.Hash, bfc.Number, pcs).Encode() | |||
| pcj, err := scale.Marshal(newJustification(s.state.round, bfc.Hash, bfc.Number, pcs)) | |||
There was a problem hiding this comment.
nit: create a local var for the justification on another line
| // Type returns voteType | ||
| func (v *VoteMessage) Type() byte { | ||
| return voteType | ||
| } | ||
|
|
||
| // Index Returns VDT index | ||
| func (v *VoteMessage) Index() uint { return 0 } |
There was a problem hiding this comment.
| func (v *VoteMessage) Index() uint { return 0 } | |
| func (vm *VoteMessage) Index() uint { return 0 } |
Update all the other receiver functions too.
| @@ -147,9 +100,12 @@ type NeighbourMessage struct { | |||
| Number uint32 | |||
| } | |||
|
|
|||
| // Index Returns VDT index | |||
| func (m *NeighbourMessage) Index() uint { return 2 } | |||
There was a problem hiding this comment.
| func (m *NeighbourMessage) Index() uint { return 2 } | |
| func (nm *NeighbourMessage) Index() uint { return 2 } |
| return nil | ||
| } | ||
| // Index Returns VDT index | ||
| func (f *CommitMessage) Index() uint { return 1 } |
There was a problem hiding this comment.
| func (f *CommitMessage) Index() uint { return 1 } | |
| func (cm *CommitMessage) Index() uint { return 1 } |
| vm := &VoteMessage{} | ||
| err = vm.Decode(r) | ||
| m = vm | ||
| err = scale.Unmarshal(msg.Data, &gossipMessage) |
There was a problem hiding this comment.
Yes please handle the error immediately
| @@ -33,6 +31,7 @@ var ( | |||
| grandpaID protocol.ID = "/paritytech/grandpa/1" | |||
| messageID = network.ConsensusMsgType | |||
| neighbourMessageInterval = time.Minute * 5 | |||
| gossipMessage = scale.MustNewVaryingDataType(&VoteMessage{}, &CommitMessage{}, &NeighbourMessage{}, &catchUpRequest{}, &catchUpResponse{}) | |||
There was a problem hiding this comment.
don't use the same VaryingDataType in decodeMessage. This isn't thread safe if there are parallel calls to decodeMessage. You will be overwriting the same address repeatedly, which could lead to undesired side effects.
Changes
Tests
Issues