From 7383bdb6caa4d435443ece3a4afe168aea1838b2 Mon Sep 17 00:00:00 2001 From: samliok Date: Tue, 4 Aug 2026 16:13:45 -0400 Subject: [PATCH] Compare against the round we held before verifying a replicated block createNotarizedBlockVerificationTask decided whether to start a new round by comparing e.round against the replicated block's round, which stays true for as long as we are ahead of that block. Capture the round before processing instead, matching what createFinalizedBlockVerificationTask already does. --- simplex/epoch.go | 5 +- simplex/replication_test.go | 107 ++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 1 deletion(-) diff --git a/simplex/epoch.go b/simplex/epoch.go index 2fcccfa8..c69a191f 100644 --- a/simplex/epoch.go +++ b/simplex/epoch.go @@ -2284,6 +2284,9 @@ func (e *Epoch) createNotarizedBlockVerificationTask(block common.Block, notariz e.lock.Lock() defer e.lock.Unlock() + // Capture the round before processing, so we can tell afterwards whether we advanced. + epochRound := e.round + // we started verifying the block when we didn't have a notarization, however its // possible we received a notarization or empty notarization for this block in the meantime. round, ok := e.rounds[md.Round] @@ -2317,7 +2320,7 @@ func (e *Epoch) createNotarizedBlockVerificationTask(block common.Block, notariz } // If we have advanced a round, and the new round is beyond our replication state, start the new round. - if e.round > md.Round && e.round > e.replicationState.GetHighestRound() { + if e.round > epochRound && e.round > e.replicationState.GetHighestRound() { if err := e.startRound(); err != nil { e.haltedError = err return md.Digest diff --git a/simplex/replication_test.go b/simplex/replication_test.go index a9050341..d9011148 100644 --- a/simplex/replication_test.go +++ b/simplex/replication_test.go @@ -1940,3 +1940,110 @@ func TestLeaderStartsRoundAfterReplicatedQuorumRound(t *testing.T) { }) } } + +// countingBlockBuilder records the round of every block building request. +type countingBlockBuilder struct { + *testutil.TestBlockBuilder + + lock sync.Mutex + rounds []uint64 +} + +func (c *countingBlockBuilder) BuildBlock(ctx context.Context, metadata common.ProtocolMetadata, blacklist common.Blacklist) (common.VerifiedBlock, bool) { + c.lock.Lock() + c.rounds = append(c.rounds, metadata.Round) + c.lock.Unlock() + + return c.TestBlockBuilder.BuildBlock(ctx, metadata, blacklist) +} + +func (c *countingBlockBuilder) buildCountForRound(round uint64) int { + c.lock.Lock() + defer c.lock.Unlock() + + var count int + for _, r := range c.rounds { + if r == round { + count++ + } + } + return count +} + +// TestReplicatedNotarizationDoesNotRestartCurrentRound asserts that verifying a replicated +// notarized block for a past round does not restart the round we are already in. +// createNotarizedBlockVerificationTask compares e.round against the replicated block's round +// rather than against the round we held before processing it, which is permanently true while +// catching up, so the leader builds a second block for a round it has already proposed in. +func TestReplicatedNotarizationDoesNotRestartCurrentRound(t *testing.T) { + nodes := []common.NodeID{{1}, {2}, {3}, {4}} + + // nodes[1] leads round 1 but not round 0, so round 0 can be skipped and replicated later. + require.NotEqual(t, nodes[1], simplex.LeaderForRound(nodes, 0)) + require.Equal(t, nodes[1], simplex.LeaderForRound(nodes, 1)) + + comm := &recordingComm{ + Communication: testutil.NewNoopComm(nodes), + BroadcastMessages: make(chan *common.Message, 100), + SentMessages: make(chan *common.Message, 100), + } + + bb := &countingBlockBuilder{TestBlockBuilder: testutil.NewTestBlockBuilder().WithBuiltBuffer(4)} + conf, _, _ := testutil.DefaultTestNodeEpochConfig(t, nodes[1], comm, bb) + conf.ReplicationEnabled = true + + e, err := simplex.NewEpoch(conf) + require.NoError(t, err) + t.Cleanup(e.Stop) + require.NoError(t, e.Start()) + + // The block the round 0 leader proposed, which our node never received directly. + blockB := testutil.NewTestBlock(e.Metadata(), emptyBlacklist) + require.Equal(t, uint64(0), blockB.BlockHeader().Round) + + validators := e.Comm.Validators() + quorum := common.Quorum(len(validators)) + notarization, err := testutil.NewNotarization(e.Logger, e.SignatureAggregatorCreator(validators), blockB, validators.NodeIDs()[:quorum]) + require.NoError(t, err) + + // Round 0 is empty notarized, so we advance to round 1 without ever storing a block or a + // notarization for round 0. + require.NoError(t, e.HandleMessage(&common.Message{ + EmptyNotarization: testutil.NewEmptyNotarization(nodes[:quorum], 0), + }, nodes[0])) + + require.Eventually(t, func() bool { + return e.Metadata().Round == 1 + }, 5*time.Second, 10*time.Millisecond, "node did not advance to round 1") + + // As leader of round 1 the node proposes. Wait for the proposal so that the first build has + // finished and e.rounds[1] is populated. + require.Eventually(t, func() bool { + for { + select { + case msg := <-comm.BroadcastMessages: + if msg.VerifiedBlockMessage != nil { + return msg.VerifiedBlockMessage.VerifiedBlock.BlockHeader().Round == 1 + } + default: + return false + } + } + }, 5*time.Second, 10*time.Millisecond, "leader never proposed a block for round 1") + + require.Equal(t, 1, bb.buildCountForRound(1), "round 1 should have been built exactly once") + + // A peer replicates the notarized block for round 0, the round we skipped. + require.NoError(t, e.HandleMessage(&common.Message{ + ReplicationResponse: &common.ReplicationResponse{ + Data: []common.QuorumRound{{Block: blockB, Notarization: ¬arization}}, + }, + }, nodes[0])) + + require.Never(t, func() bool { + return bb.buildCountForRound(1) > 1 + }, 3*time.Second, 10*time.Millisecond, + "verifying a replicated notarized block for round 0 restarted round 1, so the leader built a second block for a round it had already proposed in") + + require.Equal(t, uint64(1), e.Metadata().Round, "round should not have advanced") +}