[FAB-2468] configtx ChannelHeader to ChannelId

https://jira.hyperledger.org/browse/FAB-2468 After an audit with Elli, it's been concluded that the ChannelHeader embedded in the configtx.proto Config and ConfigUpdate messages are unnecessary, that the only piece of information needed is the ChannelId. By removing these, the messages will be simpler to construct and understand. Change-Id: I45273ee71ad981fdfa2f76e7a1717b316a5e5584 Signed-off-by: Jason Yellick <jyellick@us.ibm.com>
hyperledger · Mar 3, 2017 · fdd62b0 · fdd62b0
1 parent 29d7fc0
commit fdd62b0
Show file tree

Hide file tree

Showing 15 changed files with 154 additions and 195 deletions.
diff --git a/common/configtx/manager.go b/common/configtx/manager.go
@@ -1,5 +1,5 @@
 /*
-Copyright IBM Corp. 2016 All Rights Reserved.
+Copyright IBM Corp. 2017 All Rights Reserved.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -23,6 +23,7 @@ import (
 
 	"github.com/hyperledger/fabric/common/configtx/api"
 	cb "github.com/hyperledger/fabric/protos/common"
+	"github.com/hyperledger/fabric/protos/utils"
 
 	logging "github.com/op/go-logging"
 )
@@ -99,42 +100,40 @@ func validateChainID(chainID string) error {
 	return nil
 }
 
-func NewManagerImpl(configEnv *cb.ConfigEnvelope, initializer api.Initializer, callOnUpdate []func(api.Manager)) (api.Manager, error) {
-	if configEnv == nil {
-		return nil, fmt.Errorf("Nil config envelope")
-	}
-
-	if configEnv.Config == nil {
-		return nil, fmt.Errorf("Nil config envelope Config")
+func NewManagerImpl(envConfig *cb.Envelope, initializer api.Initializer, callOnUpdate []func(api.Manager)) (api.Manager, error) {
+	if envConfig == nil {
+		return nil, fmt.Errorf("Nil envelope")
 	}
 
-	if configEnv.Config.Channel == nil {
-		return nil, fmt.Errorf("Nil config envelope Config.Channel")
+	configEnv := &cb.ConfigEnvelope{}
+	header, err := utils.UnmarshalEnvelopeOfType(envConfig, cb.HeaderType_CONFIG, configEnv)
+	if err != nil {
+		return nil, fmt.Errorf("Bad envelope: %s", err)
 	}
 
-	if configEnv.Config.Header == nil {
-		return nil, fmt.Errorf("Nil config envelop Config Header")
+	if configEnv.Config == nil {
+		return nil, fmt.Errorf("Nil config envelope Config")
 	}
 
-	if err := validateChainID(configEnv.Config.Header.ChannelId); err != nil {
+	if err := validateChainID(header.ChannelId); err != nil {
 		return nil, fmt.Errorf("Bad channel id: %s", err)
 	}
 
-	configMap, err := mapConfig(configEnv.Config.Channel)
+	configMap, err := mapConfig(configEnv.Config.ChannelGroup)
 	if err != nil {
 		return nil, fmt.Errorf("Error converting config to map: %s", err)
 	}
 
 	cm := &configManager{
 		Resources:    initializer,
 		initializer:  initializer,
-		sequence:     computeSequence(configEnv.Config.Channel),
-		chainID:      configEnv.Config.Header.ChannelId,
+		sequence:     configEnv.Config.Sequence,
+		chainID:      header.ChannelId,
 		config:       configMap,
 		callOnUpdate: callOnUpdate,
 	}
 
-	result, err := cm.processConfig(configEnv.Config.Channel)
+	result, err := cm.processConfig(configEnv.Config.ChannelGroup)
 	if err != nil {
 		return nil, err
 	}
@@ -177,10 +176,8 @@ func (cm *configManager) ProposeConfigUpdate(configtx *cb.Envelope) (*cb.ConfigE
 
 	return &cb.ConfigEnvelope{
 		Config: &cb.Config{
-			Header: &cb.ChannelHeader{
-				ChannelId: cm.chainID,
-			},
-			Channel: channelGroup,
+			Sequence:     cm.sequence,
+			ChannelGroup: channelGroup,
 		},
 		LastUpdate: configtx,
 	}, nil
@@ -210,7 +207,7 @@ func (cm *configManager) prepareApply(configEnv *cb.ConfigEnvelope) (map[string]
 		return nil, nil, fmt.Errorf("Config cannot be nil")
 	}
 
-	if !reflect.DeepEqual(channelGroup, configEnv.Config.Channel) {
+	if !reflect.DeepEqual(channelGroup, configEnv.Config.ChannelGroup) {
 		return nil, nil, fmt.Errorf("ConfigEnvelope LastUpdate did not produce the supplied config result")
 	}
 

diff --git a/common/configtx/manager_test.go b/common/configtx/manager_test.go
@@ -61,19 +61,28 @@ func makeConfigPair(id, modificationPolicy string, lastModified uint64, data []b
 	}
 }
 
-func makeConfigEnvelope(chainID string, configPairs ...*configPair) *cb.ConfigEnvelope {
+func makeEnvelopeConfig(channelID string, configPairs ...*configPair) *cb.Envelope {
 	values := make(map[string]*cb.ConfigValue)
 	for _, pair := range configPairs {
 		values[pair.key] = pair.value
 	}
 
-	return &cb.ConfigEnvelope{
-		Config: &cb.Config{
-			Header: &cb.ChannelHeader{ChannelId: chainID},
-			Channel: &cb.ConfigGroup{
-				Values: values,
+	return &cb.Envelope{
+		Payload: utils.MarshalOrPanic(&cb.Payload{
+			Header: &cb.Header{
+				ChannelHeader: utils.MarshalOrPanic(&cb.ChannelHeader{
+					Type:      int32(cb.HeaderType_CONFIG),
+					ChannelId: channelID,
+				}),
 			},
-		},
+			Data: utils.MarshalOrPanic(&cb.ConfigEnvelope{
+				Config: &cb.Config{
+					ChannelGroup: &cb.ConfigGroup{
+						Values: values,
+					},
+				},
+			}),
+		}),
 	}
 }
 
@@ -84,7 +93,7 @@ func makeConfigUpdateEnvelope(chainID string, configPairs ...*configPair) *cb.En
 	}
 
 	config := &cb.ConfigUpdate{
-		Header: &cb.ChannelHeader{ChannelId: chainID},
+		ChannelId: chainID,
 		WriteSet: &cb.ConfigGroup{
 			Values: values,
 		},
@@ -110,7 +119,7 @@ func TestCallback(t *testing.T) {
 	}
 
 	cm, err := NewManagerImpl(
-		makeConfigEnvelope(defaultChain, makeConfigPair("foo", "foo", 0, []byte("foo"))),
+		makeEnvelopeConfig(defaultChain, makeConfigPair("foo", "foo", 0, []byte("foo"))),
 		defaultInitializer(), []func(api.Manager){callback})
 
 	if err != nil {
@@ -125,7 +134,7 @@ func TestCallback(t *testing.T) {
 // TestDifferentChainID tests that a config update for a different chain ID fails
 func TestDifferentChainID(t *testing.T) {
 	cm, err := NewManagerImpl(
-		makeConfigEnvelope(defaultChain, makeConfigPair("foo", "foo", 0, []byte("foo"))),
+		makeEnvelopeConfig(defaultChain, makeConfigPair("foo", "foo", 0, []byte("foo"))),
 		defaultInitializer(), nil)
 
 	if err != nil {
@@ -143,7 +152,7 @@ func TestDifferentChainID(t *testing.T) {
 // TestOldConfigReplay tests that resubmitting a config for a sequence number which is not newer is ignored
 func TestOldConfigReplay(t *testing.T) {
 	cm, err := NewManagerImpl(
-		makeConfigEnvelope(defaultChain, makeConfigPair("foo", "foo", 0, []byte("foo"))),
+		makeEnvelopeConfig(defaultChain, makeConfigPair("foo", "foo", 0, []byte("foo"))),
 		defaultInitializer(), nil)
 
 	if err != nil {
@@ -161,7 +170,7 @@ func TestOldConfigReplay(t *testing.T) {
 // TestValidConfigChange tests the happy path of updating a config value with no defaultModificationPolicy
 func TestValidConfigChange(t *testing.T) {
 	cm, err := NewManagerImpl(
-		makeConfigEnvelope(defaultChain, makeConfigPair("foo", "foo", 0, []byte("foo"))),
+		makeEnvelopeConfig(defaultChain, makeConfigPair("foo", "foo", 0, []byte("foo"))),
 		defaultInitializer(), nil)
 
 	if err != nil {
@@ -190,7 +199,7 @@ func TestValidConfigChange(t *testing.T) {
 // config values while advancing another
 func TestConfigChangeRegressedSequence(t *testing.T) {
 	cm, err := NewManagerImpl(
-		makeConfigEnvelope(defaultChain, makeConfigPair("foo", "foo", 1, []byte("foo"))),
+		makeEnvelopeConfig(defaultChain, makeConfigPair("foo", "foo", 1, []byte("foo"))),
 		defaultInitializer(), nil)
 
 	if err != nil {
@@ -213,7 +222,7 @@ func TestConfigChangeRegressedSequence(t *testing.T) {
 // config values while advancing another
 func TestConfigChangeOldSequence(t *testing.T) {
 	cm, err := NewManagerImpl(
-		makeConfigEnvelope(defaultChain, makeConfigPair("foo", "foo", 1, []byte("foo"))),
+		makeEnvelopeConfig(defaultChain, makeConfigPair("foo", "foo", 1, []byte("foo"))),
 		defaultInitializer(), nil)
 
 	if err != nil {
@@ -236,7 +245,7 @@ func TestConfigChangeOldSequence(t *testing.T) {
 // by omitting them in the new config
 func TestConfigImplicitDelete(t *testing.T) {
 	cm, err := NewManagerImpl(
-		makeConfigEnvelope(
+		makeEnvelopeConfig(
 			defaultChain,
 			makeConfigPair("foo", "foo", 0, []byte("foo")),
 			makeConfigPair("bar", "bar", 0, []byte("bar")),
@@ -261,7 +270,7 @@ func TestConfigImplicitDelete(t *testing.T) {
 // TestEmptyConfigUpdate tests to make sure that an empty config is rejected as an update
 func TestEmptyConfigUpdate(t *testing.T) {
 	cm, err := NewManagerImpl(
-		makeConfigEnvelope(defaultChain, makeConfigPair("foo", "foo", 0, []byte("foo"))),
+		makeEnvelopeConfig(defaultChain, makeConfigPair("foo", "foo", 0, []byte("foo"))),
 		defaultInitializer(), nil)
 
 	if err != nil {
@@ -281,7 +290,7 @@ func TestEmptyConfigUpdate(t *testing.T) {
 // increasing the config item's LastModified
 func TestSilentConfigModification(t *testing.T) {
 	cm, err := NewManagerImpl(
-		makeConfigEnvelope(
+		makeEnvelopeConfig(
 			defaultChain,
 			makeConfigPair("foo", "foo", 0, []byte("foo")),
 			makeConfigPair("bar", "bar", 0, []byte("bar")),
@@ -309,7 +318,7 @@ func TestSilentConfigModification(t *testing.T) {
 func TestConfigChangeViolatesPolicy(t *testing.T) {
 	initializer := defaultInitializer()
 	cm, err := NewManagerImpl(
-		makeConfigEnvelope(defaultChain, makeConfigPair("foo", "foo", 0, []byte("foo"))),
+		makeEnvelopeConfig(defaultChain, makeConfigPair("foo", "foo", 0, []byte("foo"))),
 		initializer, nil)
 
 	if err != nil {
@@ -331,7 +340,7 @@ func TestConfigChangeViolatesPolicy(t *testing.T) {
 func TestUnchangedConfigViolatesPolicy(t *testing.T) {
 	initializer := defaultInitializer()
 	cm, err := NewManagerImpl(
-		makeConfigEnvelope(defaultChain, makeConfigPair("foo", "foo", 0, []byte("foo"))),
+		makeEnvelopeConfig(defaultChain, makeConfigPair("foo", "foo", 0, []byte("foo"))),
 		initializer, nil)
 
 	if err != nil {
@@ -369,7 +378,7 @@ func TestUnchangedConfigViolatesPolicy(t *testing.T) {
 func TestInvalidProposal(t *testing.T) {
 	initializer := defaultInitializer()
 	cm, err := NewManagerImpl(
-		makeConfigEnvelope(defaultChain, makeConfigPair("foo", "foo", 0, []byte("foo"))),
+		makeEnvelopeConfig(defaultChain, makeConfigPair("foo", "foo", 0, []byte("foo"))),
 		initializer, nil)
 
 	if err != nil {
@@ -391,7 +400,7 @@ func TestMissingHeader(t *testing.T) {
 	group := cb.NewConfigGroup()
 	group.Values["foo"] = &cb.ConfigValue{}
 	_, err := NewManagerImpl(
-		&cb.ConfigEnvelope{Config: &cb.Config{Channel: group}},
+		&cb.Envelope{Payload: utils.MarshalOrPanic(&cb.Payload{Data: utils.MarshalOrPanic(&cb.ConfigEnvelope{Config: &cb.Config{ChannelGroup: group}})})},
 		defaultInitializer(), nil)
 
 	if err == nil {
@@ -402,7 +411,7 @@ func TestMissingHeader(t *testing.T) {
 // TestMissingChainID checks that a config item with a missing chainID causes the config to be rejected
 func TestMissingChainID(t *testing.T) {
 	_, err := NewManagerImpl(
-		makeConfigEnvelope("", makeConfigPair("foo", "foo", 0, []byte("foo"))),
+		makeEnvelopeConfig("", makeConfigPair("foo", "foo", 0, []byte("foo"))),
 		defaultInitializer(), nil)
 
 	if err == nil {

diff --git a/common/configtx/template.go b/common/configtx/template.go
@@ -70,11 +70,8 @@ func NewSimpleTemplate(configGroups ...*cb.ConfigGroup) Template {
 // Envelope returns a ConfigUpdateEnvelope for the given chainID
 func (st *simpleTemplate) Envelope(chainID string) (*cb.ConfigUpdateEnvelope, error) {
 	config, err := proto.Marshal(&cb.ConfigUpdate{
-		Header: &cb.ChannelHeader{
-			ChannelId: chainID,
-			Type:      int32(cb.HeaderType_CONFIG),
-		},
-		WriteSet: st.configGroup,
+		ChannelId: chainID,
+		WriteSet:  st.configGroup,
 	})
 
 	if err != nil {
@@ -146,11 +143,8 @@ func (ct *compositeTemplate) Envelope(chainID string) (*cb.ConfigUpdateEnvelope,
 	}
 
 	marshaledConfig, err := proto.Marshal(&cb.ConfigUpdate{
-		Header: &cb.ChannelHeader{
-			ChannelId: chainID,
-			Type:      int32(cb.HeaderType_CONFIG),
-		},
-		WriteSet: channel,
+		ChannelId: chainID,
+		WriteSet:  channel,
 	})
 	if err != nil {
 		return nil, err

diff --git a/common/configtx/update.go b/common/configtx/update.go
@@ -36,8 +36,8 @@ func (cm *configManager) authorizeUpdate(configUpdateEnv *cb.ConfigUpdateEnvelop
 		return nil, err
 	}
 
-	if config.Header == nil {
-		return nil, fmt.Errorf("Must have header set")
+	if config.ChannelId != cm.chainID {
+		return nil, fmt.Errorf("Update not for correct channel: %s for %s", config.ChannelId, cm.chainID)
 	}
 
 	seq := computeSequence(config.WriteSet)
@@ -55,11 +55,6 @@ func (cm *configManager) authorizeUpdate(configUpdateEnv *cb.ConfigUpdateEnvelop
 		return nil, fmt.Errorf("Config sequence number jumped from %d to %d", cm.sequence, seq)
 	}
 
-	// Verify config is intended for this globally unique chain ID
-	if config.Header.ChannelId != cm.chainID {
-		return nil, fmt.Errorf("Config is for the wrong chain, expected %s, got %s", cm.chainID, config.Header.ChannelId)
-	}
-
 	configMap, err := mapConfig(config.WriteSet)
 	if err != nil {
 		return nil, err
@@ -149,7 +144,7 @@ func envelopeToConfigUpdate(configtx *cb.Envelope) (*cb.ConfigUpdateEnvelope, er
 		return nil, err
 	}
 
-	if payload.Header == nil /* || payload.Header.ChannelHeader == nil */ {
+	if payload.Header == nil {
 		return nil, fmt.Errorf("Envelope must have a Header")
 	}
 

diff --git a/common/configtx/util.go b/common/configtx/util.go
@@ -17,10 +17,7 @@ limitations under the License.
 package configtx
 
 import (
-	"fmt"
-
 	cb "github.com/hyperledger/fabric/protos/common"
-	"github.com/hyperledger/fabric/protos/utils"
 
 	"github.com/golang/protobuf/proto"
 )
@@ -100,22 +97,3 @@ func UnmarshalConfigEnvelopeOrPanic(data []byte) *cb.ConfigEnvelope {
 	}
 	return result
 }
-
-// ConfigEnvelopeFromBlock extract the config envelope from a config block
-func ConfigEnvelopeFromBlock(block *cb.Block) (*cb.ConfigEnvelope, error) {
-	if block.Data == nil || len(block.Data.Data) != 1 {
-		return nil, fmt.Errorf("Not a config block, must contain exactly one tx")
-	}
-
-	envelope, err := utils.UnmarshalEnvelope(block.Data.Data[0])
-	if err != nil {
-		return nil, err
-	}
-
-	payload, err := utils.UnmarshalPayload(envelope.Payload)
-	if err != nil {
-		return nil, err
-	}
-
-	return UnmarshalConfigEnvelope(payload.Data)
-}
diff --git a/common/genesis/genesis.go b/common/genesis/genesis.go
@@ -58,7 +58,7 @@ func (f *factory) Block(chainID string) (*cb.Block, error) {
 	payloadChannelHeader := utils.MakeChannelHeader(cb.HeaderType_CONFIG, msgVersion, chainID, epoch)
 	payloadSignatureHeader := utils.MakeSignatureHeader(nil, utils.CreateNonceOrPanic())
 	payloadHeader := utils.MakePayloadHeader(payloadChannelHeader, payloadSignatureHeader)
-	payload := &cb.Payload{Header: payloadHeader, Data: utils.MarshalOrPanic(&cb.ConfigEnvelope{Config: &cb.Config{Header: configUpdate.Header, Channel: configUpdate.WriteSet}})}
+	payload := &cb.Payload{Header: payloadHeader, Data: utils.MarshalOrPanic(&cb.ConfigEnvelope{Config: &cb.Config{ChannelGroup: configUpdate.WriteSet}})}
 	envelope := &cb.Envelope{Payload: utils.MarshalOrPanic(payload), Signature: nil}
 
 	block := cb.NewBlock(0, nil)

diff --git a/core/peer/peer.go b/core/peer/peer.go
@@ -164,7 +164,7 @@ func getCurrConfigBlockFromLedger(ledger ledger.PeerLedger) (*common.Block, erro
 // createChain creates a new chain object and insert it into the chains
 func createChain(cid string, ledger ledger.PeerLedger, cb *common.Block) error {
 
-	configEnvelope, err := configtx.ConfigEnvelopeFromBlock(cb)
+	envelopeConfig, err := utils.ExtractEnvelope(cb, 0)
 	if err != nil {
 		return err
 	}
@@ -181,7 +181,7 @@ func createChain(cid string, ledger ledger.PeerLedger, cb *common.Block) error {
 	}
 
 	configtxManager, err := configtx.NewManagerImpl(
-		configEnvelope,
+		envelopeConfig,
 		configtxInitializer,
 		[]func(cm configtxapi.Manager){gossipCallbackWrapper},
 	)

diff --git a/orderer/configupdate/configupdate.go b/orderer/configupdate/configupdate.go
@@ -136,8 +136,7 @@ func createInitialConfig(envConfigUpdate *cb.Envelope) (*cb.ConfigEnvelope, erro
 
 	return &cb.ConfigEnvelope{
 		Config: &cb.Config{
-			Header:  configUpdate.Header,
-			Channel: configUpdate.WriteSet,
+			ChannelGroup: configUpdate.WriteSet,
 		},
 
 		LastUpdate: envConfigUpdate,