Wire the epoch transition listener into the instance - #507
Conversation
565be6b to
587b3a1
Compare
587b3a1 to
0714c4e
Compare
e7226d0 to
e5d71d3
Compare
e5d71d3 to
10be7d2
Compare
d99a5c6 to
c3a7763
Compare
c3a7763 to
9e8e635
Compare
9e8e635 to
6053dd9
Compare
6053dd9 to
c37fe01
Compare
c37fe01 to
6fc3cd3
Compare
| comm := newCommunication(i.Config.Sender, i.Config.Broadcaster, validators) | ||
|
|
||
| instanceStorage := NewCallbackStorage(i.cs, msm, func(block *ParsedBlock) error { | ||
| if err := i.transitionListener.onIndex(block); err != nil { |
There was a problem hiding this comment.
if block.Type() == metadata.BlockTypeTransitioning {
if err := i.transitionListener.onIndex(block); err != nil {
return err
}
}
We only need to send either approvals or aux info if we're transitioning to a new epoch. We currently handle this in onIndex but I think it's cleaner to do it here since we anyway do it for BlockTypeSealing, this will let us collapse handleTransitionBlock into onIndex.
There was a problem hiding this comment.
yea good cleanup 👍
| return nil | ||
| } | ||
|
|
||
| // TODO: use common.Digest |
There was a problem hiding this comment.
Is this TODO too hard to do in this PR?
There was a problem hiding this comment.
no, for another because we don't use digest in many places
| epochOrNV timeAdvancer | ||
| epochChanges chan epochChange | ||
| stopCh chan struct{} | ||
| lock sync.Mutex |
There was a problem hiding this comment.
We added code to instance.go but no tests were added. Are the tests in a future PR in the chain after the test refactoring?
There was a problem hiding this comment.
yep, the tests are in the next PR
|
|
||
| // TestNonValidatorContributesApproval asserts that once the auxiliary info history is | ||
| // sufficient, a non-validator that belongs to the next validator set broadcasts its | ||
| // epoch transition approval. It does not record the approval locally (nil handleApproval), |
There was a problem hiding this comment.
// epoch transition approval. It does not record the approval locally (nil handleApproval),
// since it has no block builder to include it.
I don't understand, how is the block builder relevant here?
There was a problem hiding this comment.
yea im not sure either. removed this comment.
| approval := msg.EpochTransitionApproval | ||
| require.Equal(t, testNodeID, approval.NodeID) | ||
| require.Equal(t, nextPChainRef, approval.PChainHeight) | ||
| require.Equal(t, []byte("signature"), approval.Signature) |
There was a problem hiding this comment.
where is []byte("signature") coming from?
There was a problem hiding this comment.
from the stub signer, i moved this to a shared variable so its more clear
| require.Empty(t, env.approvals) | ||
| } | ||
|
|
||
| func TestTransitionBroadcastsApproval(t *testing.T) { |
There was a problem hiding this comment.
Isn't this test contained in other tests below?
| require.Empty(t, env.approvals) | ||
| } | ||
|
|
||
| func TestTransitionNotEnoughAuxiliary(t *testing.T) { |
There was a problem hiding this comment.
I don't understand what this test is doing, can you add some comments?
2 participants
Adds
epochTransitionListenerand hooks it into the instance:NewInstanceconstructs the listener and wires itsonIndexinto both the validator and non-validator storage paths (replacing the no-op hook).HandleMessageroutes incomingAuxiliaryInfoandEpochTransitionApprovalmessages into the MSM.