[FAB-3493] Fix LAST_CONFIG value on new channels

The recent consortium-related stack of changes touched on the channel
creation path and introduced a bug in the LAST_CONFIG value that is set
on all on blocks between the genesis and the first actual config block.
Specifically, their LAST_CONFIG value is set to 1 when it should be set
to 0 (i.e. point to the genesis block).

This changeset:
1. Extends a test so that this check is performed
2. Fixes this bug in a minimally invasive manner, and modifies all
dependent tests accordingly.

Details behind the bug, as well as the thinking behind this fix can be
found in the JIRA issue.

Change-Id: Ief105744503df21d289b1e2113c9038998bdda92
Signed-off-by: Kostas Christidis <[email protected]>

Author: kchristidis

common/configtx/manager.go

@@ -175,15 +175,24 @@ func (cm *configManager) proposeConfigUpdate(configtx *cb.Envelope) (*cb.ConfigE
 
 func (cm *configManager) prepareApply(configEnv *cb.ConfigEnvelope) (map[string]comparable, *configResult, error) {
 	if configEnv == nil {
-		return nil, nil, fmt.Errorf("Attempted to apply config with nil envelope")
+		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")
+		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)
+	var expectedSequence uint64
+	if configEnv.Config.NewChannel {
+		expectedSequence = FixNewChannelConfig(configEnv).Config.Sequence
+		if configEnv.Config.Sequence != expectedSequence {
+			return nil, nil, fmt.Errorf("should prepare to update to %d (new channel) got %d instead", expectedSequence, configEnv.Config.Sequence)
+		}
+	} else {
+		expectedSequence = cm.current.sequence + 1
+		if configEnv.Config.Sequence != expectedSequence {
+			return nil, nil, fmt.Errorf("expected sequence %d (config at: %d), cannot prepare to update to %d", expectedSequence, cm.current.sequence, configEnv.Config.Sequence)
+		}
 	}
 
 	configUpdateEnv, err := envelopeToConfigUpdate(configEnv.LastUpdate)
@@ -198,11 +207,11 @@ func (cm *configManager) prepareApply(configEnv *cb.ConfigEnvelope) (map[string]
 
 	channelGroup, err := configMapToConfig(configMap)
 	if err != nil {
-		return nil, nil, fmt.Errorf("Could not turn configMap back to channelGroup: %s", err)
+		return nil, nil, fmt.Errorf("could not turn configMap back to channelGroup: %s", err)
 	}
 
 	if !reflect.DeepEqual(channelGroup, configEnv.Config.ChannelGroup) {
-		return nil, nil, fmt.Errorf("ConfigEnvelope LastUpdate did not produce the supplied config result")
+		return nil, nil, fmt.Errorf("configEnvelope LastUpdate did not produce the supplied config result")
 	}
 
 	result, err := cm.processConfig(channelGroup)

common/configtx/util.go

@@ -97,3 +97,18 @@ func UnmarshalConfigEnvelopeOrPanic(data []byte) *cb.ConfigEnvelope {
 	}
 	return result
 }
+
+// FixNewChannelConfig is to be applied on messages of type ConfigEnvelope that
+// are used to create a new channel. It returns a ConfigEnvelope whose
+// Config.Sequence and Config.NewChannel fields are set so that the config
+// manager of the new channel starts at the right sequence number.
+func FixNewChannelConfig(configEnv *cb.ConfigEnvelope) *cb.ConfigEnvelope {
+	properSequence := uint64(0)
+	if configEnv.Config.Sequence != properSequence {
+		logger.Debugf("Received a new channel config env whose sequence is incorrectly set to %d, setting to %d instead",
+			configEnv.Config.Sequence, properSequence)
+	}
+	configEnv.Config.Sequence = properSequence
+	configEnv.Config.NewChannel = true
+	return configEnv
+}

common/configtx/util_test.go

@@ -19,6 +19,10 @@ package configtx
 import (
 	"math/rand"
 	"testing"
+
+	cb "github.com/hyperledger/fabric/protos/common"
+
+	"github.com/stretchr/testify/assert"
 )
 
 // TestValidchainID checks that the constraints on chain IDs are enforced properly
@@ -59,6 +63,20 @@ func TestValidChainID(t *testing.T) {
 	})
 }
 
+// TestFixNewChannelConfig checks that returned config envelope has its Sequence
+// and NewChannel fields set appropriately.
+func TestFixNewChannelConfig(t *testing.T) {
+	expectedSequenceValue := uint64(0)
+	expectedNewChannelValue := true
+
+	sampleConfigEnvelope := &cb.ConfigEnvelope{Config: &cb.Config{Sequence: uint64(rand.Uint32())}}
+	returnedConfigEnvelope := FixNewChannelConfig(sampleConfigEnvelope)
+
+	assert.Equal(t, expectedSequenceValue, returnedConfigEnvelope.Config.Sequence, "Sequence should be zero %d", expectedSequenceValue)
+	assert.NotNil(t, returnedConfigEnvelope.Config.NewChannel, "NewChannel field should be set")
+	assert.Equal(t, expectedNewChannelValue, returnedConfigEnvelope.Config.NewChannel, "NewChannel field should be set to %t", expectedNewChannelValue)
+}
+
 // Helper functions
 
 func randomAlphaString(size int) string {

common/mocks/configtx/configtx.go

@@ -177,7 +177,7 @@ type Manager struct {
 	// ProposeConfigUpdateError is returned as the error value for ProposeConfigUpdate
 	ProposeConfigUpdateError error
 
-	// ProposeConfigUpdateVal is returns as the value for ProposeConfigUpdate
+	// ProposeConfigUpdateVal returns as the value for ProposeConfigUpdate
 	ProposeConfigUpdateVal *cb.ConfigEnvelope
 
 	// ConfigEnvelopeVal is returned as the value for ConfigEnvelope()

orderer/common/broadcast/broadcast.go

@@ -162,7 +162,7 @@ func (bh *handlerImpl) Handle(srv ab.AtomicBroadcast_BroadcastServer) error {
 		}
 
 		if logger.IsEnabledFor(logging.DEBUG) {
-			logger.Debugf("Broadcast has successfully enqueued message of type %d for chain %s", chdr.Type, chdr.ChannelId)
+			logger.Debugf("Broadcast has successfully enqueued message of type %s for chain %s", cb.HeaderType_name[chdr.Type], chdr.ChannelId)
 		}
 
 		err = srv.Send(&ab.BroadcastResponse{Status: cb.Status_SUCCESS})

orderer/configupdate/configupdate.go

@@ -23,6 +23,7 @@ package configupdate
 import (
 	"fmt"
 
+	"github.com/hyperledger/fabric/common/configtx"
 	configtxapi "github.com/hyperledger/fabric/common/configtx/api"
 	"github.com/hyperledger/fabric/common/crypto"
 	cb "github.com/hyperledger/fabric/protos/common"
@@ -141,7 +142,7 @@ func (p *Processor) newChannelConfig(channelID string, envConfigUpdate *cb.Envel
 		return nil, err
 	}
 
-	newChannelEnvConfig, err := utils.CreateSignedEnvelope(cb.HeaderType_CONFIG, channelID, p.signer, newChannelConfigEnv, msgVersion, epoch)
+	newChannelEnvConfig, err := utils.CreateSignedEnvelope(cb.HeaderType_CONFIG, channelID, p.signer, configtx.FixNewChannelConfig(newChannelConfigEnv), msgVersion, epoch)
 	if err != nil {
 		return nil, err
 	}

orderer/configupdate/configupdate_test.go

@@ -58,6 +58,9 @@ func (msm *mockSupportManager) GetChain(chainID string) (Support, bool) {
 func (msm *mockSupportManager) NewChannelConfig(env *cb.Envelope) (configtxapi.Manager, error) {
 	return &mockconfigtx.Manager{
 		ProposeConfigUpdateVal: &cb.ConfigEnvelope{
+			Config: &cb.Config{
+				Sequence: 0,
+			},
 			LastUpdate: env,
 		},
 	}, nil

orderer/multichain/chainsupport.go

@@ -231,6 +231,7 @@ func (cs *chainSupport) addLastConfigSignature(block *cb.Block) {
 	}
 
 	lastConfigValue := utils.MarshalOrPanic(&cb.LastConfig{Index: cs.lastConfig})
+	logger.Debugf("[channel: %s] About to write block, setting its LAST_CONFIG to %d", cs.ChainID(), cs.lastConfig)
 
 	lastConfigSignature.Signature = utils.SignOrPanic(cs.signer, util.ConcatenateBytes(lastConfigValue, lastConfigSignature.SignatureHeader, block.Header.Bytes()))
 
@@ -257,6 +258,8 @@ func (cs *chainSupport) WriteBlock(block *cb.Block, committers []filter.Committe
 	if err != nil {
 		logger.Panicf("[channel: %s] Could not append block: %s", cs.ChainID(), err)
 	}
+
+	logger.Debugf("[channel: %s] Wrote block %d", cs.ChainID(), block.GetHeader().Number)
 	return block
 }
 

orderer/multichain/manager.go

@@ -290,7 +290,7 @@ func (ml *multiLedger) NewChannelConfig(envConfigUpdate *cb.Envelope) (configtxa
 	channelGroup.Groups[config.ApplicationGroupKey] = applicationGroup
 	channelGroup.Values[config.ConsortiumKey] = config.TemplateConsortium(consortium.Name).Values[config.ConsortiumKey]
 
-	templateConfig, err := utils.CreateSignedEnvelope(cb.HeaderType_CONFIG, configUpdate.ChannelId, ml.signer, &cb.ConfigEnvelope{
+	templateConfig, _ := utils.CreateSignedEnvelope(cb.HeaderType_CONFIG, configUpdate.ChannelId, ml.signer, &cb.ConfigEnvelope{
 		Config: &cb.Config{
 			ChannelGroup: channelGroup,
 		},

orderer/multichain/manager_test.go

@@ -21,6 +21,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/golang/protobuf/proto"
 	"github.com/hyperledger/fabric/common/configtx"
 	genesisconfig "github.com/hyperledger/fabric/common/configtx/tool/localconfig"
 	"github.com/hyperledger/fabric/common/configtx/tool/provisional"
@@ -253,7 +254,7 @@ func TestNewChain(t *testing.T) {
 	configEnv, err := cm.ProposeConfigUpdate(envConfigUpdate)
 	assert.NoError(t, err, "Proposing initial update")
 
-	ingressTx, err := utils.CreateSignedEnvelope(cb.HeaderType_CONFIG, newChainID, mockCrypto(), configEnv, msgVersion, epoch)
+	ingressTx, err := utils.CreateSignedEnvelope(cb.HeaderType_CONFIG, newChainID, mockCrypto(), configtx.FixNewChannelConfig(configEnv), msgVersion, epoch)
 	assert.NoError(t, err, "Creating ingresstx")
 
 	wrapped := wrapConfigTx(ingressTx)
@@ -315,6 +316,16 @@ func TestNewChain(t *testing.T) {
 		if status != cb.Status_SUCCESS {
 			t.Fatalf("Could not retrieve block on new chain")
 		}
+
+		// Inspect LAST_CONFIG value
+		metadataItem := &cb.Metadata{}
+		err := proto.Unmarshal(block.Metadata.Metadata[cb.BlockMetadataIndex_LAST_CONFIG], metadataItem)
+		assert.NoError(t, err, "Block should carry LAST_CONFIG metadata item")
+		lastConfig := &cb.LastConfig{}
+		err = proto.Unmarshal(metadataItem.Value, lastConfig)
+		assert.NoError(t, err, "LAST_CONFIG metadata item should carry last config value")
+		assert.Equal(t, uint64(0), lastConfig.Index, "LAST_CONFIG value should point to genesis block")
+
 		for i := 0; i < int(conf.Orderer.BatchSize.MaxMessageCount); i++ {
 			if !reflect.DeepEqual(utils.ExtractEnvelopeOrPanic(block, i), messages[i]) {
 				t.Errorf("Block contents wrong at index %d in new chain", i)

orderer/multichain/systemchain.go

@@ -129,7 +129,7 @@ func (scf *systemChainFilter) authorize(configEnvelope *cb.ConfigEnvelope) (conf
 		return nil, err
 	}
 
-	err = configManager.Apply(newChannelConfigEnv)
+	err = configManager.Apply(configtx.FixNewChannelConfig(newChannelConfigEnv))
 	if err != nil {
 		return nil, err
 	}

orderer/multichain/systemchain_test.go

@@ -32,6 +32,8 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+var newChannelID = "newChannel"
+
 type mockSupport struct {
 	msc *mockconfig.Orderer
 }
@@ -71,96 +73,94 @@ func (mcc *mockChainCreator) NewChannelConfig(envConfigUpdate *cb.Envelope) (con
 	if mcc.NewChannelConfigErr != nil {
 		return nil, mcc.NewChannelConfigErr
 	}
-	confUpdate := configtx.UnmarshalConfigUpdateOrPanic(configtx.UnmarshalConfigUpdateEnvelopeOrPanic(utils.UnmarshalPayloadOrPanic(envConfigUpdate.Payload).Data).ConfigUpdate)
-	return &mockconfigtx.Manager{
-		ConfigEnvelopeVal: &cb.ConfigEnvelope{
-			Config:     &cb.Config{Sequence: 1, ChannelGroup: confUpdate.WriteSet},
-			LastUpdate: envConfigUpdate,
-		},
-	}, nil
+
+	pldConfigUpdate := utils.UnmarshalPayloadOrPanic(envConfigUpdate.Payload)
+	configUpdateEnv := configtx.UnmarshalConfigUpdateEnvelopeOrPanic(pldConfigUpdate.Data)
+	configUpdate := configtx.UnmarshalConfigUpdateOrPanic(configUpdateEnv.ConfigUpdate)
+
+	configEnvelopeVal := &cb.ConfigEnvelope{
+		Config: &cb.Config{ChannelGroup: configUpdate.WriteSet},
+	}
+
+	proposeConfigUpdateVal := configEnvelopeVal
+	proposeConfigUpdateVal.Config.Sequence = 1
+	proposeConfigUpdateVal.LastUpdate = envConfigUpdate
+
+	ctxm := &mockconfigtx.Manager{
+		ConfigEnvelopeVal:      configEnvelopeVal,
+		ProposeConfigUpdateVal: proposeConfigUpdateVal,
+	}
+
+	return ctxm, nil
 }
 
 func TestGoodProposal(t *testing.T) {
-	newChainID := "NewChainID"
-
 	mcc := newMockChainCreator()
 
-	configEnv, err := configtx.NewCompositeTemplate(
+	configUpdateEnv, _ := configtx.NewCompositeTemplate(
 		configtx.NewSimpleTemplate(
 			config.DefaultHashingAlgorithm(),
 			config.DefaultBlockDataHashingStructure(),
 			config.TemplateOrdererAddresses([]string{"foo"}),
 		),
 		configtx.NewChainCreationTemplate("SampleConsortium", []string{}),
-	).Envelope(newChainID)
-	if err != nil {
-		t.Fatalf("Error constructing configtx")
-	}
-	ingressTx := makeConfigTxFromConfigUpdateEnvelope(newChainID, configEnv)
-	wrapped := wrapConfigTx(ingressTx)
+	).Envelope(newChannelID)
 
-	sysFilter := newSystemChainFilter(mcc.ms, mcc)
-	action, committer := sysFilter.Apply(wrapped)
+	ingressTx := makeConfigTxFromConfigUpdateEnvelope(newChannelID, configUpdateEnv, true)
+	ordererTx := wrapConfigTx(ingressTx)
 
+	sysFilter := newSystemChainFilter(mcc.ms, mcc)
+	action, committer := sysFilter.Apply(ordererTx)
 	assert.EqualValues(t, action, filter.Accept, "Did not accept valid transaction")
 	assert.True(t, committer.Isolated(), "Channel creation belong in its own block")
 
 	committer.Commit()
 	assert.Len(t, mcc.newChains, 1, "Proposal should only have created 1 new chain")
-
 	assert.Equal(t, ingressTx, mcc.newChains[0], "New chain should have been created with ingressTx")
 }
 
 func TestProposalRejectedByConfig(t *testing.T) {
-	newChainID := "NewChainID"
-
 	mcc := newMockChainCreator()
 	mcc.NewChannelConfigErr = fmt.Errorf("Error creating channel")
 
-	configEnv, err := configtx.NewCompositeTemplate(
+	configUpdateEnv, _ := configtx.NewCompositeTemplate(
 		configtx.NewSimpleTemplate(
 			config.DefaultHashingAlgorithm(),
 			config.DefaultBlockDataHashingStructure(),
 			config.TemplateOrdererAddresses([]string{"foo"}),
 		),
 		configtx.NewChainCreationTemplate("SampleConsortium", []string{}),
-	).Envelope(newChainID)
-	if err != nil {
-		t.Fatalf("Error constructing configtx")
-	}
-	ingressTx := makeConfigTxFromConfigUpdateEnvelope(newChainID, configEnv)
-	wrapped := wrapConfigTx(ingressTx)
+	).Envelope(newChannelID)
+
+	ingressTx := makeConfigTxFromConfigUpdateEnvelope(newChannelID, configUpdateEnv, true)
+	ordererTx := wrapConfigTx(ingressTx)
 
 	sysFilter := newSystemChainFilter(mcc.ms, mcc)
-	action, _ := sysFilter.Apply(wrapped)
+	action, _ := sysFilter.Apply(ordererTx)
 
-	assert.EqualValues(t, action, filter.Reject, "Did not accept valid transaction")
+	assert.EqualValues(t, action, filter.Reject, "Did not reject invalid transaction")
 	assert.Len(t, mcc.newChains, 0, "Proposal should not have created a new chain")
 }
 
 func TestNumChainsExceeded(t *testing.T) {
-	newChainID := "NewChainID"
-
 	mcc := newMockChainCreator()
 	mcc.ms.msc.MaxChannelsCountVal = 1
 	mcc.newChains = make([]*cb.Envelope, 2)
 
-	configEnv, err := configtx.NewCompositeTemplate(
+	configUpdateEnv, _ := configtx.NewCompositeTemplate(
 		configtx.NewSimpleTemplate(
 			config.DefaultHashingAlgorithm(),
 			config.DefaultBlockDataHashingStructure(),
 			config.TemplateOrdererAddresses([]string{"foo"}),
 		),
 		configtx.NewChainCreationTemplate("SampleConsortium", []string{}),
-	).Envelope(newChainID)
-	if err != nil {
-		t.Fatalf("Error constructing configtx")
-	}
-	ingressTx := makeConfigTxFromConfigUpdateEnvelope(newChainID, configEnv)
-	wrapped := wrapConfigTx(ingressTx)
+	).Envelope(newChannelID)
+
+	ingressTx := makeConfigTxFromConfigUpdateEnvelope(newChannelID, configUpdateEnv, true)
+	ordererTx := wrapConfigTx(ingressTx)
 
 	sysFilter := newSystemChainFilter(mcc.ms, mcc)
-	action, _ := sysFilter.Apply(wrapped)
+	action, _ := sysFilter.Apply(ordererTx)
 
 	assert.EqualValues(t, filter.Reject, action, "Transaction had created too many channels")
 }

orderer/multichain/util_test.go

@@ -93,7 +93,7 @@ func makeConfigTx(chainID string, i int) *cb.Envelope {
 	if err != nil {
 		panic(err)
 	}
-	return makeConfigTxFromConfigUpdateEnvelope(chainID, configEnv)
+	return makeConfigTxFromConfigUpdateEnvelope(chainID, configEnv, false)
 }
 
 func wrapConfigTx(env *cb.Envelope) *cb.Envelope {
@@ -104,18 +104,30 @@ func wrapConfigTx(env *cb.Envelope) *cb.Envelope {
 	return result
 }
 
-func makeConfigTxFromConfigUpdateEnvelope(chainID string, configUpdateEnv *cb.ConfigUpdateEnvelope) *cb.Envelope {
+func makeConfigTxFromConfigUpdateEnvelope(chainID string, configUpdateEnv *cb.ConfigUpdateEnvelope, newChannel bool) *cb.Envelope {
 	configUpdateTx, err := utils.CreateSignedEnvelope(cb.HeaderType_CONFIG_UPDATE, chainID, mockCrypto(), configUpdateEnv, msgVersion, epoch)
 	if err != nil {
 		panic(err)
 	}
-	configTx, err := utils.CreateSignedEnvelope(cb.HeaderType_CONFIG, chainID, mockCrypto(), &cb.ConfigEnvelope{
-		Config:     &cb.Config{Sequence: 1, ChannelGroup: configtx.UnmarshalConfigUpdateOrPanic(configUpdateEnv.ConfigUpdate).WriteSet},
-		LastUpdate: configUpdateTx},
+
+	configEnv := &cb.ConfigEnvelope{
+		Config: &cb.Config{
+			Sequence:     1,
+			ChannelGroup: configtx.UnmarshalConfigUpdateOrPanic(configUpdateEnv.ConfigUpdate).WriteSet,
+		},
+		LastUpdate: configUpdateTx,
+	}
+
+	if newChannel {
+		configEnv = configtx.FixNewChannelConfig(configEnv)
+	}
+
+	configTx, err := utils.CreateSignedEnvelope(cb.HeaderType_CONFIG, chainID, mockCrypto(), configEnv,
 		msgVersion, epoch)
 	if err != nil {
 		panic(err)
 	}
+
 	return configTx
 }