Include nodes from next epoch in applicable messages #51
Conversation
add test cases for different epoch phases
| suite.me.On("NodeID").Return(localID) | ||
| suite.state.On("Final").Return(suite.final) | ||
| suite.final.On("Identities", mock.Anything).Return( | ||
| func(f flow.IdentityFilter) flow.IdentityList { return suite.currentEpochIdentities[1:].Filter(f) }, |
| ) | ||
| suite.final.On("Epochs").Return(suite.epochQuery) | ||
| suite.epochQuery.On("Next").Return(suite.nextEpoch) | ||
| suite.nextEpoch.On("InitialIdentities").Return( |
There was a problem hiding this comment.
I guess here we can just do .Return(suite.nextEpochIdentities, nil)
There was a problem hiding this comment.
I try to use the functional returns in the mocks when it's returning members of Suite, because it will stay up to date with the current value of suite.nextEpochIdentities if it's changed. If you do .Return(suite.nextEpochIdentities, nil), it will always return the initial value of suite.nextEpochIdentities since suite.nextEpochIdentities is evaluated when Return is invoked in SetupTest.
There was a problem hiding this comment.
that makes a lot of sense! thanks
| @@ -302,3 +302,25 @@ func (il IdentityList) StakingKeys() []crypto.PublicKey { | |||
| } | |||
| return keys | |||
| } | |||
|
|
|||
| // Union returns a new identity list containing every identity that occurs in | |||
| // either `il`, or `other`, or both. There are no duplicates in the output, | |||
|
|
||
| uniques := make(map[flow.Identifier]struct{}) | ||
|
|
||
| // should contain one less than the sum of the two input list lengths since there is a dupe |
| // filter to only include non-consensus nodes | ||
| nextEpochRecipients := nextEpochIdentities.Filter(filter.Not(filter.HasRole(flow.RoleConsensus))) | ||
|
|
||
| return currentEpochRecipients.Union(nextEpochRecipients), nil |
There was a problem hiding this comment.
Is it worth to cache the recipientList, otherwise it always have to union when sending a block.
There was a problem hiding this comment.
I'd rather keep it simple. Even for thousands of nodes, this will be very inexpensive. Plus, when we have slashing, the cache could be potentially be invalidated every block.
There was a problem hiding this comment.
Right. we are proposing 1 block per second, so should be OK
Reference: https://github.com/dapperlabs/flow-go/issues/2594
This PR ensures that nodes joining the network in the next epoch are included in relevant network communications. In particular, the
consensus/providerengine is updated to broadcast block proposals to the next epoch's nodes once they become available. Thesynchronizationengine already accepts and responds to any messages that are valid wrt to the networking layer (and this is covered in tests by the use ofIdentifierFixturefororiginID) so no change is needed here.