[FAB-2511] Make configtx sequence explicit

https://jira.hyperledger.org/browse/FAB-2511

The previous non-MVCC configuration system required that the modified
version numbers of the configuration elements be equal to the global
configuration sequence lock.

This makes the global sequence explicit, while allowing the individual
elements to move (or not move) their version numbers locally.  This
finally enables the partial update functionality advertised in FAB-1880.

Change-Id: Ic2f554b864e91435a8812d56eb8247cc1fa33720
Signed-off-by: Jason Yellick <[email protected]>

Author: Jason Yellick

common/configtx/api/api.go

@@ -37,9 +37,6 @@ type Manager interface {
 	// Validate attempts to validate a new configtx against the current config state
 	ProposeConfigUpdate(configtx *cb.Envelope) (*cb.ConfigEnvelope, error)
 
-	// ConfigEnvelope returns the *cb.ConfigEnvelope from the last successful Apply
-	ConfigEnvelope() *cb.ConfigEnvelope
-
 	// ChainID retrieves the chain ID associated with this manager
 	ChainID() string
 

common/configtx/config.go

@@ -69,7 +69,7 @@ func (cm *configManager) proposeGroup(name string, group *cb.ConfigGroup, handle
 		i++
 	}
 
-	logger.Debugf("Beginning new config for channel %s and group %s", cm.chainID, name)
+	logger.Debugf("Beginning new config for channel %s and group %s", cm.current.channelID, name)
 	subHandlers, err := handler.BeginValueProposals(subGroups)
 	if err != nil {
 		return nil, err

common/configtx/configmap.go

@@ -37,9 +37,11 @@ const (
 // it takes a ConfigGroup and generates a map of fqPath to comparables (or error on invalid keys)
 func mapConfig(channelGroup *cb.ConfigGroup) (map[string]comparable, error) {
 	result := make(map[string]comparable)
-	err := recurseConfig(result, []string{RootGroupKey}, channelGroup)
-	if err != nil {
-		return nil, err
+	if channelGroup != nil {
+		err := recurseConfig(result, []string{RootGroupKey}, channelGroup)
+		if err != nil {
+			return nil, err
+		}
 	}
 	return result, nil
 }

common/configtx/manager.go

@@ -40,34 +40,17 @@ var (
 	}
 )
 
-// NewConfigItemPolicyKey is the ID of the policy used when no other policy can be resolved, for instance when attempting to create a new config item
-const NewConfigItemPolicyKey = "NewConfigItemPolicy"
+type config struct {
+	channelID string
+	sequence  uint64
+	configMap map[string]comparable
+}
 
 type configManager struct {
 	api.Resources
-	sequence     uint64
-	chainID      string
-	config       map[string]comparable
 	callOnUpdate []func(api.Manager)
 	initializer  api.Initializer
-	configEnv    *cb.ConfigEnvelope
-}
-
-func computeSequence(configGroup *cb.ConfigGroup) uint64 {
-	max := uint64(0)
-	for _, value := range configGroup.Values {
-		if value.Version > max {
-			max = value.Version
-		}
-	}
-
-	for _, group := range configGroup.Groups {
-		if groupMax := computeSequence(group); groupMax > max {
-			max = groupMax
-		}
-	}
-
-	return max
+	current      *config
 }
 
 // validateChainID makes sure that proposed chain IDs (i.e. channel names)
@@ -125,11 +108,13 @@ func NewManagerImpl(envConfig *cb.Envelope, initializer api.Initializer, callOnU
 	}
 
 	cm := &configManager{
-		Resources:    initializer,
-		initializer:  initializer,
-		sequence:     configEnv.Config.Sequence,
-		chainID:      header.ChannelId,
-		config:       configMap,
+		Resources:   initializer,
+		initializer: initializer,
+		current: &config{
+			sequence:  configEnv.Config.Sequence,
+			configMap: configMap,
+			channelID: header.ChannelId,
+		},
 		callOnUpdate: callOnUpdate,
 	}
 
@@ -149,9 +134,13 @@ func (cm *configManager) commitCallbacks() {
 	}
 }
 
-// Validate attempts to validate a new configtx against the current config state
-// It requires an Envelope of type CONFIG_UPDATE
+// ProposeConfigUpdate takes in an Envelope of type CONFIG_UPDATE and produces a
+// ConfigEnvelope to be used as the Envelope Payload Data of a CONFIG message
 func (cm *configManager) ProposeConfigUpdate(configtx *cb.Envelope) (*cb.ConfigEnvelope, error) {
+	return cm.proposeConfigUpdate(configtx)
+}
+
+func (cm *configManager) proposeConfigUpdate(configtx *cb.Envelope) (*cb.ConfigEnvelope, error) {
 	configUpdateEnv, err := envelopeToConfigUpdate(configtx)
 	if err != nil {
 		return nil, err
@@ -176,7 +165,7 @@ func (cm *configManager) ProposeConfigUpdate(configtx *cb.Envelope) (*cb.ConfigE
 
 	return &cb.ConfigEnvelope{
 		Config: &cb.Config{
-			Sequence:     cm.sequence,
+			Sequence:     cm.current.sequence + 1,
 			ChannelGroup: channelGroup,
 		},
 		LastUpdate: configtx,
@@ -188,6 +177,14 @@ func (cm *configManager) prepareApply(configEnv *cb.ConfigEnvelope) (map[string]
 		return nil, nil, fmt.Errorf("Attempted to apply config with nil envelope")
 	}
 
+	if configEnv.Config == nil {
+		return nil, nil, fmt.Errorf("Config cannot be nil")
+	}
+
+	if configEnv.Config.Sequence != cm.current.sequence+1 {
+		return nil, nil, fmt.Errorf("Config at sequence %d, cannot prepare to update to %d", cm.current.sequence, configEnv.Config.Sequence)
+	}
+
 	configUpdateEnv, err := envelopeToConfigUpdate(configEnv.LastUpdate)
 	if err != nil {
 		return nil, nil, err
@@ -203,10 +200,6 @@ func (cm *configManager) prepareApply(configEnv *cb.ConfigEnvelope) (map[string]
 		return nil, nil, fmt.Errorf("Could not turn configMap back to channelGroup: %s", err)
 	}
 
-	if configEnv.Config == nil {
-		return nil, nil, fmt.Errorf("Config cannot be nil")
-	}
-
 	if !reflect.DeepEqual(channelGroup, configEnv.Config.ChannelGroup) {
 		return nil, nil, fmt.Errorf("ConfigEnvelope LastUpdate did not produce the supplied config result")
 	}
@@ -241,23 +234,21 @@ func (cm *configManager) Apply(configEnv *cb.ConfigEnvelope) error {
 	result.commit()
 	cm.commitCallbacks()
 
-	cm.config = configMap
-	cm.sequence++
+	cm.current = &config{
+		configMap: configMap,
+		channelID: cm.current.channelID,
+		sequence:  configEnv.Config.Sequence,
+	}
 
 	return nil
 }
 
-// ConfigEnvelope retrieve the current ConfigEnvelope, generated after the last successfully applied configuration
-func (cm *configManager) ConfigEnvelope() *cb.ConfigEnvelope {
-	return cm.configEnv
-}
-
 // ChainID retrieves the chain ID associated with this manager
 func (cm *configManager) ChainID() string {
-	return cm.chainID
+	return cm.current.channelID
 }
 
 // Sequence returns the current sequence number of the config
 func (cm *configManager) Sequence() uint64 {
-	return cm.sequence
+	return cm.current.sequence
 }

common/configtx/manager_test.go

@@ -25,6 +25,8 @@ import (
 	mockpolicies "github.com/hyperledger/fabric/common/mocks/policies"
 	cb "github.com/hyperledger/fabric/protos/common"
 	"github.com/hyperledger/fabric/protos/utils"
+
+	"github.com/stretchr/testify/assert"
 )
 
 var defaultChain = "DefaultChainID"
@@ -62,9 +64,9 @@ func makeConfigPair(id, modificationPolicy string, lastModified uint64, data []b
 }
 
 func makeEnvelopeConfig(channelID string, configPairs ...*configPair) *cb.Envelope {
-	values := make(map[string]*cb.ConfigValue)
+	channelGroup := cb.NewConfigGroup()
 	for _, pair := range configPairs {
-		values[pair.key] = pair.value
+		channelGroup.Values[pair.key] = pair.value
 	}
 
 	return &cb.Envelope{
@@ -77,27 +79,22 @@ func makeEnvelopeConfig(channelID string, configPairs ...*configPair) *cb.Envelo
 			},
 			Data: utils.MarshalOrPanic(&cb.ConfigEnvelope{
 				Config: &cb.Config{
-					ChannelGroup: &cb.ConfigGroup{
-						Values: values,
-					},
+					ChannelGroup: channelGroup,
 				},
 			}),
 		}),
 	}
 }
 
-func makeConfigUpdateEnvelope(chainID string, configPairs ...*configPair) *cb.Envelope {
-	values := make(map[string]*cb.ConfigValue)
+func makeConfigSet(configPairs ...*configPair) *cb.ConfigGroup {
+	result := cb.NewConfigGroup()
 	for _, pair := range configPairs {
-		values[pair.key] = pair.value
+		result.Values[pair.key] = pair.value
 	}
+	return result
+}
 
-	config := &cb.ConfigUpdate{
-		ChannelId: chainID,
-		WriteSet: &cb.ConfigGroup{
-			Values: values,
-		},
-	}
+func makeConfigUpdateEnvelope(chainID string, readSet, writeSet *cb.ConfigGroup) *cb.Envelope {
 	return &cb.Envelope{
 		Payload: utils.MarshalOrPanic(&cb.Payload{
 			Header: &cb.Header{
@@ -106,7 +103,11 @@ func makeConfigUpdateEnvelope(chainID string, configPairs ...*configPair) *cb.En
 				}),
 			},
 			Data: utils.MarshalOrPanic(&cb.ConfigUpdateEnvelope{
-				ConfigUpdate: utils.MarshalOrPanic(config),
+				ConfigUpdate: utils.MarshalOrPanic(&cb.ConfigUpdate{
+					ChannelId: chainID,
+					ReadSet:   readSet,
+					WriteSet:  writeSet,
+				}),
 			}),
 		}),
 	}
@@ -141,7 +142,7 @@ func TestDifferentChainID(t *testing.T) {
 		t.Fatalf("Error constructing config manager: %s", err)
 	}
 
-	newConfig := makeConfigUpdateEnvelope("wrongChain", makeConfigPair("foo", "foo", 1, []byte("foo")))
+	newConfig := makeConfigUpdateEnvelope("wrongChain", makeConfigSet(), makeConfigSet(makeConfigPair("foo", "foo", 1, []byte("foo"))))
 
 	_, err = cm.ProposeConfigUpdate(newConfig)
 	if err == nil {
@@ -159,7 +160,7 @@ func TestOldConfigReplay(t *testing.T) {
 		t.Fatalf("Error constructing config manager: %s", err)
 	}
 
-	newConfig := makeConfigUpdateEnvelope(defaultChain, makeConfigPair("foo", "foo", 0, []byte("foo")))
+	newConfig := makeConfigUpdateEnvelope(defaultChain, makeConfigSet(), makeConfigSet(makeConfigPair("foo", "foo", 0, []byte("foo"))))
 
 	_, err = cm.ProposeConfigUpdate(newConfig)
 	if err == nil {
@@ -177,7 +178,7 @@ func TestValidConfigChange(t *testing.T) {
 		t.Fatalf("Error constructing config manager: %s", err)
 	}
 
-	newConfig := makeConfigUpdateEnvelope(defaultChain, makeConfigPair("foo", "foo", 1, []byte("foo")))
+	newConfig := makeConfigUpdateEnvelope(defaultChain, makeConfigSet(), makeConfigSet(makeConfigPair("foo", "foo", 1, []byte("foo"))))
 
 	configEnv, err := cm.ProposeConfigUpdate(newConfig)
 	if err != nil {
@@ -208,8 +209,8 @@ func TestConfigChangeRegressedSequence(t *testing.T) {
 
 	newConfig := makeConfigUpdateEnvelope(
 		defaultChain,
-		makeConfigPair("foo", "foo", 0, []byte("foo")),
-		makeConfigPair("bar", "bar", 2, []byte("bar")),
+		makeConfigSet(makeConfigPair("foo", "foo", 0, []byte("foo"))),
+		makeConfigSet(makeConfigPair("bar", "bar", 2, []byte("bar"))),
 	)
 
 	_, err = cm.ProposeConfigUpdate(newConfig)
@@ -231,8 +232,11 @@ func TestConfigChangeOldSequence(t *testing.T) {
 
 	newConfig := makeConfigUpdateEnvelope(
 		defaultChain,
-		makeConfigPair("foo", "foo", 2, []byte("foo")),
-		makeConfigPair("bar", "bar", 1, []byte("bar")),
+		makeConfigSet(),
+		makeConfigSet(
+			makeConfigPair("foo", "foo", 2, []byte("foo")),
+			makeConfigPair("bar", "bar", 1, []byte("bar")),
+		),
 	)
 
 	_, err = cm.ProposeConfigUpdate(newConfig)
@@ -241,9 +245,9 @@ func TestConfigChangeOldSequence(t *testing.T) {
 	}
 }
 
-// TestConfigImplicitDelete tests to make sure that a new config does not implicitly delete config items
-// by omitting them in the new config
-func TestConfigImplicitDelete(t *testing.T) {
+// TestConfigPartialUpdate tests to make sure that a new config can set only part
+// of the config and still be accepted
+func TestConfigPartialUpdate(t *testing.T) {
 	cm, err := NewManagerImpl(
 		makeEnvelopeConfig(
 			defaultChain,
@@ -258,13 +262,12 @@ func TestConfigImplicitDelete(t *testing.T) {
 
 	newConfig := makeConfigUpdateEnvelope(
 		defaultChain,
-		makeConfigPair("bar", "bar", 1, []byte("bar")),
+		makeConfigSet(),
+		makeConfigSet(makeConfigPair("bar", "bar", 1, []byte("bar"))),
 	)
 
 	_, err = cm.ProposeConfigUpdate(newConfig)
-	if err == nil {
-		t.Error("Should have errored proposing config because foo was implicitly deleted")
-	}
+	assert.NoError(t, err, "Should have allowed partial update")
 }
 
 // TestEmptyConfigUpdate tests to make sure that an empty config is rejected as an update
@@ -303,8 +306,11 @@ func TestSilentConfigModification(t *testing.T) {
 
 	newConfig := makeConfigUpdateEnvelope(
 		defaultChain,
-		makeConfigPair("foo", "foo", 0, []byte("different")),
-		makeConfigPair("bar", "bar", 1, []byte("bar")),
+		makeConfigSet(),
+		makeConfigSet(
+			makeConfigPair("foo", "foo", 0, []byte("different")),
+			makeConfigPair("bar", "bar", 1, []byte("bar")),
+		),
 	)
 
 	_, err = cm.ProposeConfigUpdate(newConfig)
@@ -327,7 +333,7 @@ func TestConfigChangeViolatesPolicy(t *testing.T) {
 	// Set the mock policy to error
 	initializer.Resources.PolicyManagerVal.Policy.Err = fmt.Errorf("err")
 
-	newConfig := makeConfigUpdateEnvelope(defaultChain, makeConfigPair("foo", "foo", 1, []byte("foo")))
+	newConfig := makeConfigUpdateEnvelope(defaultChain, makeConfigSet(), makeConfigSet(makeConfigPair("foo", "foo", 1, []byte("foo"))))
 
 	_, err = cm.ProposeConfigUpdate(newConfig)
 	if err == nil {
@@ -353,8 +359,8 @@ func TestUnchangedConfigViolatesPolicy(t *testing.T) {
 
 	newConfig := makeConfigUpdateEnvelope(
 		defaultChain,
-		makeConfigPair("foo", "foo", 0, []byte("foo")),
-		makeConfigPair("bar", "bar", 1, []byte("foo")),
+		makeConfigSet(makeConfigPair("foo", "foo", 0, []byte("foo"))),
+		makeConfigSet(makeConfigPair("bar", "bar", 0, []byte("foo"))),
 	)
 
 	configEnv, err := cm.ProposeConfigUpdate(newConfig)
@@ -387,7 +393,7 @@ func TestInvalidProposal(t *testing.T) {
 
 	initializer.ValueProposerVal = &mockconfigtx.ValueProposer{ErrorForProposeConfig: fmt.Errorf("err")}
 
-	newConfig := makeConfigUpdateEnvelope(defaultChain, makeConfigPair("foo", "foo", 1, []byte("foo")))
+	newConfig := makeConfigUpdateEnvelope(defaultChain, makeConfigSet(), makeConfigSet(makeConfigPair("foo", "foo", 1, []byte("foo"))))
 
 	_, err = cm.ProposeConfigUpdate(newConfig)
 	if err == nil {