Remove Epoch From the EpochConfig - #575
Conversation
| } | ||
| if finalization.Finalization.Round >= highestRound { | ||
| highestRound = finalization.Finalization.Round | ||
| // We do not need to check if this finalization is associated for a sealing block, because it is impossible for a finalization |
There was a problem hiding this comment.
// We do not need to check if this finalization is associated for a sealing block, because it is impossible for a finalization
// for another epoch to exist in our wal, yet the original epoch remains un-sealed.
That's true, but why are we writing this?
There was a problem hiding this comment.
i added because at first i thought we needed to also set the epoch here, so i wrote it here since it wasn't initially obvious. Maybe a mythos scan will note this as a bug if not documented in the future
There was a problem hiding this comment.
I understand that it might make sense to make this comment in the context of this PR.
However after the PR is merged - I think it'll just be confusing.
There is actually a way for a finalization of a higher epoch be present in our WAL - if we have a sealing block in seq 100 but we don't have a finalization for seq 99, we will write the finalization of the sealing block in the WAL.
I suggest we just remove this text.
There was a problem hiding this comment.
sounds good, updated
| require.NoError(t, conf.Storage.Index(context.Background(), epoch1Block, Finalization{})) | ||
| require.Equal(t, uint16(1), epoch1Block.Blacklist().NodeCount, | ||
|
|
||
| // The epoch number is the sequence of the last indexed sealing block, so both nodes |
There was a problem hiding this comment.
because we can no longer artificially set the epoch in the config, so we need to go through with actual messages/storage
|
getting a determinist failure on the instance test, will look closer tomorrow. I think its because im not setting the epoch correctly when we build off a non-simplex block |
| nodes := []NodeID{{1}, {2}, {3}, {4}} | ||
| block := testutil.NewTestBlock(ProtocolMetadata{Seq: 0}, emptyBlacklist) | ||
| finalization := Finalization{ | ||
| Finalization: ToBeSignedFinalization{ |
There was a problem hiding this comment.
i had to update the test finalizations to contain QCs
Signed-off-by: Sam Liokumovich <65994425+samliok@users.noreply.github.com>
2 participants
this simplifies the instance, and is no longer necessary with the recent addition of
block.SealingBlockInfoAnother main benefit is we don't need to artificially set the Epoch value after initializing it. Since the epoch's config already has all the information it needs to know about its own epoch, it should be responsible for setting this value.
We also weren't properly setting the epoch if the only blocks in storage were non-simplex blocks, so this PR adds a fix in
setMetadataFromStorageand an instance test