Address code review comments

Signed-off-by: Yacov Manevich <[email protected]>

epoch.go

@@ -321,23 +321,21 @@ func (e *Epoch) resumeFromWal(records [][]byte) error {
 		e.Comm.Broadcast(&lastMessage)
 		return e.doNotarized(notarization.Vote.Round)
 	case record.EmptyVoteRecordType:
-		tbsEmptyVote, err := ParseEmptyVoteRecord(lastRecord)
+		ev, err := ParseEmptyVoteRecord(lastRecord)
 		if err != nil {
 			return err
 		}
-		signature, err := tbsEmptyVote.Sign(e.Signer)
-		if err != nil {
-			return err
+		round, exists := e.emptyVotes[ev.Round]
+		if ! exists {
+			return fmt.Errorf("round %d not found for empty vote", ev.Round)
+		}
+		emptyVote, exists := round.votes[string(e.ID)]
+		if ! exists {
+			return fmt.Errorf("could not find my own vote for round %d", ev.Round)
 		}
-		lastMessage := Message{EmptyVoteMessage: &EmptyVote{
-			Vote: tbsEmptyVote,
-			Signature: Signature{
-				Signer: e.ID,
-				Value: signature,
-			},
-		}}
+		lastMessage := Message{EmptyVoteMessage: emptyVote}
 		e.Comm.Broadcast(&lastMessage)
-		return e.startRound()
+		return nil
 	case record.EmptyNotarizationRecordType:
 		emptyNotarization, err := EmptyNotarizationFromRecord(lastRecord, e.QCDeserializer)
 		if err != nil {

epoch_failover_test.go

@@ -118,16 +118,7 @@ func TestEpochLeaderFailoverWithEmptyNotarization(t *testing.T) {
 	nextBlockSeqToCommit := uint64(3)
 	nextRoundToCommit := uint64(4)
 
-	bbAfterCrash := &testBlockBuilder{
-		out:                make(chan *testBlock, 1),
-		in:                 make(chan *testBlock, 1),
-		blockShouldBeBuilt: make(chan struct{}, 1),
-	}
-
-	bbAfterCrash.out <- block3.(*testBlock)
-	bbAfterCrash.in <- block3.(*testBlock)
-
-	runCrashAndRestartExecution(t, e, nodes, bb, wal, storage, func(t *testing.T, e *Epoch, bb *testBlockBuilder, storage *InMemStorage, wal *testWAL) {
+	runCrashAndRestartExecution(t, e, bb, wal, storage, func(t *testing.T, e *Epoch, bb *testBlockBuilder, storage *InMemStorage, wal *testWAL) {
 		// Ensure our node proposes block with sequence 3 for round 4
 		notarizeAndFinalizeRound(t, nodes, nextRoundToCommit, nextBlockSeqToCommit, e, bb, quorum, storage, false)
 		require.Equal(t, uint64(4), storage.Height())
@@ -200,7 +191,7 @@ func TestEpochLeaderFailoverReceivesEmptyVotesEarly(t *testing.T) {
 
 	waitForBlockProposerTimeout(t, e, start)
 
-	runCrashAndRestartExecution(t, e, nodes, bb, wal, storage, func(t *testing.T, e *Epoch, bb *testBlockBuilder, storage *InMemStorage, wal *testWAL) {
+	runCrashAndRestartExecution(t, e, bb, wal, storage, func(t *testing.T, e *Epoch, bb *testBlockBuilder, storage *InMemStorage, wal *testWAL) {
 		wal.lock.Lock()
 		walContent, err := wal.ReadAll()
 		require.NoError(t, err)
@@ -284,7 +275,7 @@ func TestEpochLeaderFailover(t *testing.T) {
 
 	waitForBlockProposerTimeout(t, e, start)
 
-	runCrashAndRestartExecution(t, e, nodes, bb, wal, storage, func(t *testing.T, e *Epoch, bb *testBlockBuilder, storage *InMemStorage, wal *testWAL) {
+	runCrashAndRestartExecution(t, e, bb, wal, storage, func(t *testing.T, e *Epoch, bb *testBlockBuilder, storage *InMemStorage, wal *testWAL) {
 		lastBlock, _, ok := storage.Retrieve(storage.Height() - 1)
 		require.True(t, ok)
 
@@ -492,7 +483,7 @@ func TestEpochLeaderFailoverAfterProposal(t *testing.T) {
 	bb.blockShouldBeBuilt <- struct{}{}
 	waitForBlockProposerTimeout(t, e, start)
 
-	runCrashAndRestartExecution(t, e, nodes, bb, wal, storage, func(t *testing.T, e *Epoch, bb *testBlockBuilder, storage *InMemStorage, wal *testWAL) {
+	runCrashAndRestartExecution(t, e, bb, wal, storage, func(t *testing.T, e *Epoch, bb *testBlockBuilder, storage *InMemStorage, wal *testWAL) {
 
 		lastBlock, _, ok := storage.Retrieve(storage.Height() - 1)
 		require.True(t, ok)
@@ -582,7 +573,7 @@ func TestEpochLeaderFailoverTwice(t *testing.T) {
 
 	waitForBlockProposerTimeout(t, e, start)
 
-	runCrashAndRestartExecution(t, e, nodes, bb, wal, storage, func(t *testing.T, e *Epoch, bb *testBlockBuilder, storage *InMemStorage, wal *testWAL) {
+	runCrashAndRestartExecution(t, e, bb, wal, storage, func(t *testing.T, e *Epoch, bb *testBlockBuilder, storage *InMemStorage, wal *testWAL) {
 		lastBlock, _, ok := storage.Retrieve(storage.Height() - 1)
 		require.True(t, ok)
 
@@ -612,7 +603,7 @@ func TestEpochLeaderFailoverTwice(t *testing.T) {
 
 		waitForBlockProposerTimeout(t, e, start)
 
-		runCrashAndRestartExecution(t, e, nodes, bb, wal, storage, func(t *testing.T, e *Epoch, bb *testBlockBuilder, storage *InMemStorage, wal *testWAL) {
+		runCrashAndRestartExecution(t, e, bb, wal, storage, func(t *testing.T, e *Epoch, bb *testBlockBuilder, storage *InMemStorage, wal *testWAL) {
 			md := ProtocolMetadata{
 				Round: 3,
 				Seq:   1,
@@ -769,12 +760,14 @@ func TestEpochLeaderFailoverNotNeeded(t *testing.T) {
 	require.False(t, timedOut.Load())
 }
 
-func runCrashAndRestartExecution(t *testing.T, e *Epoch, nodes []NodeID, bb *testBlockBuilder, wal *testWAL, storage *InMemStorage, f func(t *testing.T, e *Epoch, bb *testBlockBuilder, storage *InMemStorage, wal *testWAL)) {
+func runCrashAndRestartExecution(t *testing.T, e *Epoch, bb *testBlockBuilder, wal *testWAL, storage *InMemStorage, f epochExecution) {
 	// Split the test into two scenarios:
 	// 1) The node proceeds as usual.
 	// 2) The node crashes and restarts.
 	cloneWAL := wal.Clone()
 	cloneStorage := storage.Clone()
+
+	nodes := e.Comm.ListNodes()
 
 	// Clone the block builder
 	bbAfterCrash := &testBlockBuilder{
@@ -840,3 +833,5 @@ func (rc *recordingComm) Broadcast(msg *Message) {
 	rc.BroadcastMessages <- msg
 	rc.Communication.Broadcast(msg)
 }
+
+type epochExecution func(t *testing.T, e *Epoch, bb *testBlockBuilder, storage *InMemStorage, wal *testWAL)

epoch_multinode_test.go

@@ -149,6 +149,9 @@ func newTestWAL(t *testing.T) *testWAL {
 }
 
 func (tw *testWAL) Clone() *testWAL {
+	tw.lock.Lock()
+	defer tw.lock.Unlock()
+
 	rawWAL, err := tw.ReadAll()
 	require.NoError(tw.t, err)
 

epoch_test.go

@@ -923,11 +923,16 @@ func newInMemStorage() *InMemStorage {
 
 func (mem *InMemStorage) Clone() *InMemStorage {
 	clone := newInMemStorage()
-	for seq := uint64(0); seq < mem.Height(); seq++ {
+	mem.lock.Lock()
+	height := mem.Height()
+	mem.lock.Unlock()
+	for seq := uint64(0); seq < height; seq++ {
+		mem.lock.Lock()
 		block, fCert, ok := mem.Retrieve(seq)
 		if !ok {
 			panic(fmt.Sprintf("failed retrieving block %d", seq))
 		}
+		mem.lock.Unlock()
 		clone.Index(block, fCert)
 	}
 	return clone