Wal Recovery #39
Wal Recovery #39
Conversation
| @@ -340,6 +433,28 @@ func (e *Epoch) maybeCollectFinalizationCertificate(round *Round) error { | |||
| return e.assembleFinalizationCertificate(round) | |||
| } | |||
|
|
|||
| func (e *Epoch) NewFinalizationCertificate(finalizations []*Finalization) (FinalizationCertificate, error) { | |||
There was a problem hiding this comment.
I understand you refactored this method outside of assembleFinalizationCertificate to use it in a test, but we shouldn't use Epoch methods to create messages that are used to test the Epoch.
If there is a bug in Epoch logic we don't want to repeat it in the test.
There was a problem hiding this comment.
separated to a new file called record_test.go. Now that creating finalization/notarization certificates don't rely on epoch, would it make sense to use newNotarization & newFinalizationCertificate in epoch? Since they are decoupled, don't see why epoch should re-implement these functions. However if our test notarizations/finalizations are going to deviate from production then we shouldn't combine.
epoch_test.go
Outdated
| require.NoError(t, err) | ||
| require.Len(t, records, 2) | ||
| // expectedNotarizationRecord, err := newNotarizationRecord(signatureAggregator, block, nodes[0:quorum], conf.Signer) | ||
| // require.NoError(t, err) |
There was a problem hiding this comment.
why is this commented out?
There was a problem hiding this comment.
because the bytes of the notarization record are not always guaranteed to be the same every time you create it. I set up a PR #44 addressing this
epoch_test.go
Outdated
| require.NoError(t, err) | ||
|
|
||
| rounds := uint64(100) | ||
| for i := uint64(0); i < rounds; i++ { |
There was a problem hiding this comment.
is it possible not to start the test from a 0 sequence? Maybe have the instance finalize into storage a couple of blocks before recovering it?
Let's be sure that the logic that loops through the records in the WAL and ingests the messages works when the records start from an arbitrary round and not zero.
There was a problem hiding this comment.
TestRecoverFromWalWithStorage includes a test for this
| @@ -370,8 +370,7 @@ func (e *Epoch) isFinalizationCertificateValid(fCert *FinalizationCertificate) ( | |||
| return false, nil | |||
| } | |||
|
|
|||
| e.Logger.Debug("Received finalization without any signatures in it") | |||
There was a problem hiding this comment.
not sure what this log was doing here, if its valid then there should be signatures on it
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
Signed-off-by: Yacov Manevich <[email protected]>
No description provided.