address review comments

Signed-off-by: Yoav Tock <tock@il.ibm.com>

common/channelconfig/orderer.go

@@ -146,7 +146,7 @@ func NewOrdererConfig(ordererGroup *cb.ConfigGroup, mspConfig *MSPConfigHandler,
 	}
 
 	if channelCapabilities.ConsensusTypeBFT() {
-		if err := oc.validateSomeOrgHasEndpoints(); err != nil {
+		if err := oc.validateAllOrgsHaveEndpoints(); err != nil {
 			return nil, err
 		}
 	}
@@ -239,13 +239,17 @@ func (oc *OrdererConfig) validateBatchTimeout() error {
 	return nil
 }
 
-func (oc *OrdererConfig) validateSomeOrgHasEndpoints() error {
-	var someOrgHasEndpoints bool
+func (oc *OrdererConfig) validateAllOrgsHaveEndpoints() error {
+	var orgsMissingEndpoints []string
+
 	for _, org := range oc.Organizations() {
-		someOrgHasEndpoints = someOrgHasEndpoints || len(org.Endpoints()) > 0
+		if len(org.Endpoints()) == 0 {
+			orgsMissingEndpoints = append(orgsMissingEndpoints, org.Name())
+		}
 	}
-	if !someOrgHasEndpoints {
-		return errors.Errorf("all orderer organizations endpoints are empty")
+
+	if len(orgsMissingEndpoints) > 0 {
+		return errors.Errorf("some orderer organizations endpoints are empty: %s", orgsMissingEndpoints)
 	}
 
 	return nil

common/channelconfig/realconfig_test.go

@@ -34,7 +34,6 @@ func TestWithRealConfigtx(t *testing.T) {
 func TestOrgSpecificOrdererEndpoints(t *testing.T) {
 	t.Run("could not create channel orderer config with empty organization endpoints", func(t *testing.T) {
 		conf := genesisconfig.Load(genesisconfig.SampleDevModeSoloProfile, configtest.GetDevConfigDir())
-		// NOTE: conf.Capability is V3_0, one organization, conf.Orderer.Addresses (global endpoints) is empty, organization endpoints is not empty
 
 		cg, err := encoder.NewChannelGroup(conf)
 		require.NoError(t, err)
@@ -44,7 +43,7 @@ func TestOrgSpecificOrdererEndpoints(t *testing.T) {
 		cryptoProvider, err := sw.NewDefaultSecurityLevelWithKeystore(sw.NewDummyKeyStore())
 		require.NoError(t, err)
 		_, err = channelconfig.NewChannelConfig(cg, cryptoProvider)
-		require.EqualError(t, err, "could not create channel Orderer sub-group config: all orderer organizations endpoints are empty")
+		require.EqualError(t, err, "could not create channel Orderer sub-group config: some orderer organizations endpoints are empty: [SampleOrg]")
 	})
 
 	t.Run("could not create channelgroup with empty organization endpoints", func(t *testing.T) {

common/configtx/test/helper.go

@@ -66,6 +66,14 @@ func MakeGenesisBlockFromMSPs(channelID string, appMSPConf, ordererMSPConf *mspp
 		ModPolicy: channelconfig.AdminsPolicyKey,
 	}
 
+	ordererOrgProtos := &cb.OrdererAddresses{
+		Addresses: profile.Orderer.Organizations[0].OrdererEndpoints,
+	}
+	ordererOrg.Values[channelconfig.EndpointsKey] = &cb.ConfigValue{
+		Value:     protoutil.MarshalOrPanic(ordererOrgProtos),
+		ModPolicy: channelconfig.AdminsPolicyKey,
+	}
+
 	applicationOrg := protoutil.NewConfigGroup()
 	applicationOrg.ModPolicy = channelconfig.AdminsPolicyKey
 	applicationOrg.Values[channelconfig.MSPKey] = &cb.ConfigValue{

internal/configtxgen/encoder/encoder.go

@@ -141,6 +141,9 @@ func NewChannelGroup(conf *genesisconfig.Profile) (*cb.ConfigGroup, error) {
 
 	addValue(channelGroup, channelconfig.HashingAlgorithmValue(), channelconfig.AdminsPolicyKey)
 	addValue(channelGroup, channelconfig.BlockDataHashingStructureValue(), channelconfig.AdminsPolicyKey)
+	if conf.Orderer != nil && len(conf.Orderer.Addresses) > 0 {
+		addValue(channelGroup, channelconfig.OrdererAddressesValue(conf.Orderer.Addresses), ordererAdminsPolicyName)
+	}
 
 	if conf.Consortium != "" {
 		addValue(channelGroup, channelconfig.ConsortiumValue(conf.Consortium), channelconfig.AdminsPolicyKey)

internal/peer/common/common.go

@@ -280,11 +280,6 @@ func GetOrdererEndpointOfChain(chainID string, signer Signer, endorserClient pb.
 		return nil, errors.WithMessage(err, "error loading channel config")
 	}
 
-	ordererAddresses := bundle.ChannelConfig().OrdererAddresses()
-	if len(ordererAddresses) > 0 {
-		logger.Warningf("Deprecated global OrdererAddresses exist: %v; ignoring", ordererAddresses)
-	}
-
 	ordererConfig, ok := bundle.OrdererConfig()
 	if !ok {
 		return nil, errors.New("missing OrdererConfig in channel config")
@@ -296,7 +291,16 @@ func GetOrdererEndpointOfChain(chainID string, signer Signer, endorserClient pb.
 		orgAddresses = append(orgAddresses, org.Endpoints()...)
 	}
 
-	return orgAddresses, nil
+	ordererAddresses := bundle.ChannelConfig().OrdererAddresses()
+	if len(orgAddresses) > 0 {
+		if len(ordererAddresses) > 0 {
+			logger.Warningf("Deprecated global OrdererAddresses exist: %s; ignoring them", ordererAddresses)
+		}
+		return orgAddresses, nil
+	}
+
+	logger.Warningf("Org specific endpoints are missing, returning (deprecated) global OrdererAddresses: %s; ", ordererAddresses)
+	return ordererAddresses, nil
 }
 
 // CheckLogLevel checks that a given log level string is valid

internal/peer/common/common_test.go

@@ -266,14 +266,41 @@ func TestGetOrdererEndpointFromConfigTx(t *testing.T) {
 	cryptoProvider, err := sw.NewDefaultSecurityLevelWithKeystore(sw.NewDummyKeyStore())
 	require.NoError(t, err)
 
-	t.Run("green-path", func(t *testing.T) {
+	t.Run("green-path V3", func(t *testing.T) {
 		tlsCA, err := tlsgen.NewCA()
 		require.NoError(t, err)
 		certDir := t.TempDir()
 		profile := genesisconfig.Load(genesisconfig.SampleAppChannelEtcdRaftProfile, configtest.GetDevConfigDir())
-		profile.Capabilities = map[string]bool{"V3_0": true}
+		generateCertificates(profile, tlsCA, certDir)
+		t.Logf("%+v", profile.Orderer.Organizations[0])
+
+		channelGroup, err := encoder.NewChannelGroup(profile)
+		require.NoError(t, err)
+		channelConfig := &cb.Config{ChannelGroup: channelGroup}
+
+		mockEndorserClient := common.GetMockEndorserClient(
+			&pb.ProposalResponse{
+				Response:    &pb.Response{Status: 200, Payload: protoutil.MarshalOrPanic(channelConfig)},
+				Endorsement: &pb.Endorsement{},
+			},
+			nil,
+		)
+
+		ordererEndpoints, err := common.GetOrdererEndpointOfChain("test-channel", signer, mockEndorserClient, cryptoProvider)
+		require.NoError(t, err)
+		require.Equal(t, []string{"127.0.0.1:7050", "127.0.0.1:7051", "127.0.0.1:7052"}, ordererEndpoints)
+	})
+
+	t.Run("green-path V2 ignores global addresses", func(t *testing.T) {
+		tlsCA, err := tlsgen.NewCA()
+		require.NoError(t, err)
+		certDir := t.TempDir()
+		profile := genesisconfig.Load(genesisconfig.SampleAppChannelEtcdRaftProfile, configtest.GetDevConfigDir())
+		profile.Capabilities = map[string]bool{"V2_0": true}
+		profile.Orderer.Addresses = []string{"globalAddr:666"} // should be ignored
 		generateCertificates(profile, tlsCA, certDir)
 
+		t.Logf("%+v", profile.Orderer.Addresses)
 		t.Logf("%+v", profile.Orderer.Organizations[0])
 
 		channelGroup, err := encoder.NewChannelGroup(profile)
@@ -293,6 +320,35 @@ func TestGetOrdererEndpointFromConfigTx(t *testing.T) {
 		require.Equal(t, []string{"127.0.0.1:7050", "127.0.0.1:7051", "127.0.0.1:7052"}, ordererEndpoints)
 	})
 
+	t.Run("green-path V2 takes global addresses", func(t *testing.T) {
+		tlsCA, err := tlsgen.NewCA()
+		require.NoError(t, err)
+		certDir := t.TempDir()
+		profile := genesisconfig.Load(genesisconfig.SampleAppChannelEtcdRaftProfile, configtest.GetDevConfigDir())
+		profile.Capabilities = map[string]bool{"V2_0": true}
+		profile.Orderer.Addresses = []string{"globalAddr:666"}  // should be taken
+		profile.Orderer.Organizations[0].OrdererEndpoints = nil // because per-org are missing
+		generateCertificates(profile, tlsCA, certDir)
+
+		t.Logf("%+v", profile.Orderer.Organizations[0])
+
+		channelGroup, err := encoder.NewChannelGroup(profile)
+		require.NoError(t, err)
+		channelConfig := &cb.Config{ChannelGroup: channelGroup}
+
+		mockEndorserClient := common.GetMockEndorserClient(
+			&pb.ProposalResponse{
+				Response:    &pb.Response{Status: 200, Payload: protoutil.MarshalOrPanic(channelConfig)},
+				Endorsement: &pb.Endorsement{},
+			},
+			nil,
+		)
+
+		ordererEndpoints, err := common.GetOrdererEndpointOfChain("test-channel", signer, mockEndorserClient, cryptoProvider)
+		require.NoError(t, err)
+		require.Equal(t, []string{"globalAddr:666"}, ordererEndpoints)
+	})
+
 	t.Run("error-invoking-CSCC", func(t *testing.T) {
 		mockEndorserClient := common.GetMockEndorserClient(
 			nil,

sampleconfig/configtx.yaml

@@ -313,9 +313,9 @@ Orderer: &OrdererDefaults
     # ConsenterMapping contains the definition of consenter identity, endpoints, and crypto material.
     # The ConsenterMapping is used in the BFT consensus protocol, and should include enough servers to ensure
     # fault-tolerance; In BFT this number is at least 3*F+1, where F is the number of potential failures.
-    # In BFT it is highly recommended that the addresses for delivery & broadcast (Ordrer/Addresses or
-    # the OrdererEndpoints item in the org definition) map 1:1 to the Orderer/ConsenterMapping (for cluster consensus).
-    # That is, every consenter should be represented by a delivery endpoint.
+    # In BFT it is highly recommended that the addresses for delivery & broadcast (the OrdererEndpoints item in the
+    # org definition) map 1:1 to the Orderer/ConsenterMapping (for cluster consensus). That is, every consenter should
+    # be represented by a delivery endpoint. Note that in BFT (V3) global Orderer/Addresses are no longer supported.
     ConsenterMapping:
     - ID: 1
       Host: bft0.example.com