Fix mergeset limit violation bug (#1855)

* Added a test which reproduces a bug where virtual's mergeset exceeds the mergeset limit -- this test currently fails

* Added a simple condition to fix the mergeset limit bug

* Format issue

domain/consensus/processes/blockvalidator/block_header_in_context_test.go

@@ -165,14 +165,15 @@ func TestCheckMergeSizeLimit(t *testing.T) {
 	testutils.ForAllNets(t, true, func(t *testing.T, consensusConfig *consensus.Config) {
 		consensusConfig.MergeSetSizeLimit = 2 * uint64(consensusConfig.K)
 		factory := consensus.NewFactory()
-		tc, teardown, err := factory.NewTestConsensus(consensusConfig, "TestCheckParentsIncest")
+		tc, teardown, err := factory.NewTestConsensus(consensusConfig, "TestCheckMergeSizeLimit")
 		if err != nil {
 			t.Fatalf("Error setting up consensus: %+v", err)
 		}
 		defer teardown(false)
 
 		chain1TipHash := consensusConfig.GenesisHash
-		for i := uint64(0); i < consensusConfig.MergeSetSizeLimit+2; i++ {
+		// We add a chain larger by one than chain2 below, to make this one the selected chain
+		for i := uint64(0); i < consensusConfig.MergeSetSizeLimit+1; i++ {
 			chain1TipHash, _, err = tc.AddBlock([]*externalapi.DomainHash{chain1TipHash}, nil, nil)
 			if err != nil {
 				t.Fatalf("AddBlock: %+v", err)
@@ -180,7 +181,9 @@ func TestCheckMergeSizeLimit(t *testing.T) {
 		}
 
 		chain2TipHash := consensusConfig.GenesisHash
-		for i := uint64(0); i < consensusConfig.MergeSetSizeLimit+1; i++ {
+		// We add a merge set of size exactly MergeSetSizeLimit (to violate the limit),
+		// since selected parent is also counted
+		for i := uint64(0); i < consensusConfig.MergeSetSizeLimit; i++ {
 			chain2TipHash, _, err = tc.AddBlock([]*externalapi.DomainHash{chain2TipHash}, nil, nil)
 			if err != nil {
 				t.Fatalf("AddBlock: %+v", err)
@@ -193,3 +196,56 @@ func TestCheckMergeSizeLimit(t *testing.T) {
 		}
 	})
 }
+
+func TestVirtualSelectionViolatingMergeSizeLimit(t *testing.T) {
+	testutils.ForAllNets(t, true, func(t *testing.T, consensusConfig *consensus.Config) {
+		consensusConfig.MergeSetSizeLimit = 2 * uint64(consensusConfig.K)
+		factory := consensus.NewFactory()
+		tc, teardown, err := factory.NewTestConsensus(consensusConfig, "TestVirtualSelectionViolatingMergeSizeLimit")
+		if err != nil {
+			t.Fatalf("Error setting up consensus: %+v", err)
+		}
+		defer teardown(false)
+
+		chain1TipHash := consensusConfig.GenesisHash
+		// We add a chain larger than chain2 below, to make this one the selected chain
+		for i := uint64(0); i < consensusConfig.MergeSetSizeLimit; i++ {
+			chain1TipHash, _, err = tc.AddBlock([]*externalapi.DomainHash{chain1TipHash}, nil, nil)
+			if err != nil {
+				t.Fatalf("AddBlock: %+v", err)
+			}
+		}
+
+		chain2TipHash := consensusConfig.GenesisHash
+		// We add a merge set of size exactly MergeSetSizeLimit-1 (to still not violate the limit)
+		for i := uint64(0); i < consensusConfig.MergeSetSizeLimit-1; i++ {
+			chain2TipHash, _, err = tc.AddBlock([]*externalapi.DomainHash{chain2TipHash}, nil, nil)
+			if err != nil {
+				t.Fatalf("AddBlock: %+v", err)
+			}
+		}
+
+		// We now add a single block over genesis which is expected to exceed the limit
+		_, _, err = tc.AddBlock([]*externalapi.DomainHash{consensusConfig.GenesisHash}, nil, nil)
+		if err != nil {
+			t.Fatalf("AddBlock: %+v", err)
+		}
+
+		stagingArea := model.NewStagingArea()
+		virtualSelectedParent, err := tc.GetVirtualSelectedParent()
+		if err != nil {
+			t.Fatalf("GetVirtualSelectedParent: %+v", err)
+		}
+		selectedParentAnticone, err := tc.DAGTraversalManager().AnticoneFromVirtualPOV(stagingArea, virtualSelectedParent)
+		if err != nil {
+			t.Fatalf("AnticoneFromVirtualPOV: %+v", err)
+		}
+
+		// Test if Virtual's mergeset is too large
+		// Note: the selected parent itself is also counted in the mergeset limit
+		if len(selectedParentAnticone)+1 > (int)(consensusConfig.MergeSetSizeLimit) {
+			t.Fatalf("Virtual's mergset size (%d) exeeds merge set limit (%d)",
+				len(selectedParentAnticone)+1, consensusConfig.MergeSetSizeLimit)
+		}
+	})
+}

domain/consensus/processes/consensusstatemanager/pick_virtual_parents.go

@@ -59,7 +59,8 @@ func (csm *consensusStateManager) pickVirtualParents(stagingArea *model.StagingA
 	selectedVirtualParents := []*externalapi.DomainHash{virtualSelectedParent}
 	mergeSetSize := uint64(1) // starts counting from 1 because selectedParent is already in the mergeSet
 
-	for len(candidates) > 0 && uint64(len(selectedVirtualParents)) < uint64(csm.maxBlockParents) {
+	// First condition implies that no point in searching since limit was already reached
+	for mergeSetSize < csm.mergeSetSizeLimit && len(candidates) > 0 && uint64(len(selectedVirtualParents)) < uint64(csm.maxBlockParents) {
 		candidate := candidates[0]
 		candidates = candidates[1:]