[FAB-11973] failing test in statebased validation

With [FAB-11948], retrieval of state from a collection may return an error
in case the collection configuration isn't defined. The key-level validation
code must handle this case without failing, given that failure in validation
implies halting the processing on the chain. The chosen approach is to treat
the error as a benign retrieval failure, crucially, one that will
deterministically occur at every committing peer. As such, validation can
continue as if that key didn't have state-based endorsement policies. The
validator may therefore mark the transaction as valid, but the ledger will
mark it as invalid given the attempt to write to an undefined collection.

Change-Id: I2e5aaf0d05ec803873edcb92a3b9377e440b1ca1
Signed-off-by: Alessandro Sorniotti <ale.linux@sopit.net>
Signed-off-by: Matthias Neugschwandtner <eug@zurich.ibm.com>

Author: ale-linux

core/committer/txvalidator/validator_test.go

@@ -538,7 +538,6 @@ func TestInvokeOKPvtDataOnly(t *testing.T) {
 	})
 
 	t.Run("V1.3", func(t *testing.T) {
-		t.Skip()
 		l, v := setupLedgerAndValidatorExplicitWithMSP(t, v13Capabilities(), &builtin.DefaultValidation{}, mspmgr)
 		defer ledgermgmt.CleanupTestEnv()
 		defer l.Close()
@@ -627,7 +626,6 @@ func TestInvokeOKPvtMetaUpdateOnly(t *testing.T) {
 	})
 
 	t.Run("V1.3", func(t *testing.T) {
-		t.Skip()
 		l, v := setupLedgerAndValidatorExplicitWithMSP(t, &mockconfig.MockApplicationCapabilities{V1_3ValidationRv: true, V1_2ValidationRv: true, PrivateChannelDataRv: true}, &builtin.DefaultValidation{}, mspmgr)
 		defer ledgermgmt.CleanupTestEnv()
 		defer l.Close()

core/common/validation/statebased/validator_keylevel.go

@@ -12,6 +12,7 @@ import (
 
 	commonerrors "github.com/hyperledger/fabric/common/errors"
 	"github.com/hyperledger/fabric/core/handlers/validation/api/policies"
+	"github.com/hyperledger/fabric/core/ledger"
 	"github.com/hyperledger/fabric/core/ledger/kvledger/txmgmt/rwsetutil"
 	"github.com/hyperledger/fabric/protos/common"
 	"github.com/hyperledger/fabric/protos/peer"
@@ -59,9 +60,29 @@ func (p *policyChecker) checkSBAndCCEP(cc, coll, key string, blockNum, txNum uin
 	// see if there is a key-level validation parameter for this key
 	vp, err := p.vpmgr.GetValidationParameterForKey(cc, coll, key, blockNum, txNum)
 	if err != nil {
-		switch err := err.(type) {
+		// error handling for GetValidationParameterForKey follows this rationale:
+		switch err := errors.Cause(err).(type) {
+		// 1) if there is a conflict because validation params have been updated
+		//    by another transaction in this block, we will get ValidationParameterUpdatedError.
+		//    This should lead to invalidating the transaction by calling policyErr
 		case *ValidationParameterUpdatedError:
 			return policyErr(err)
+		// 2) if the ledger returns "determinstic" errors, that is, errors that
+		//    every peer in the channel will also return (such as errors linked to
+		//    an attempt to retrieve metadata from a non-defined collection) should be
+		//    logged and ignored. The ledger will take the most appropriate action
+		//    when performing its side of the validation.
+		case *ledger.CollConfigNotDefinedError, *ledger.InvalidCollNameError:
+			logger.Warningf(errors.WithMessage(err, "skipping key-level validation").Error())
+			err = nil
+		// 3) any other type of error should return an execution failure which will
+		//    lead to halting the processing on this channel. Note that any non-categorized
+		//    deterministic error would be caught by the default and would lead to
+		//    a processing halt. This would certainly be a bug, but - in the absence of a
+		//    single, well-defined deterministic error returned by the ledger, it is
+		//    best to err on the side of caution and rather halt processing (because a
+		//    deterministic error is treated like an I/O one) rather than risking a fork
+		//    (in case an I/O error is treated as a deterministic one).
 		default:
 			return &commonerrors.VSCCExecutionFailureError{
 				Err: err,

core/common/validation/statebased/validator_keylevel_test.go

@@ -11,6 +11,7 @@ import (
 	"testing"
 
 	"github.com/hyperledger/fabric/common/errors"
+	"github.com/hyperledger/fabric/core/ledger"
 	"github.com/hyperledger/fabric/core/ledger/kvledger/txmgmt/rwsetutil"
 	"github.com/hyperledger/fabric/core/ledger/kvledger/txmgmt/version"
 	"github.com/hyperledger/fabric/protos/common"
@@ -262,6 +263,48 @@ func TestKeylevelValidationPolicyRetrievalFailure(t *testing.T) {
 	assert.IsType(t, &errors.VSCCExecutionFailureError{}, err)
 }
 
+func TestKeylevelValidationLedgerFailures(t *testing.T) {
+	// Scenario: we validate a transaction that updates
+	// the key-level validation parameters for a key.
+	// we simulate the case where we fail to retrieve
+	// the validation parameters from the ledger with
+	// both deterministic and non-deterministic errors
+
+	rwsb := rwsetBytes(t, "cc")
+	prp := []byte("barf")
+
+	t.Run("CollConfigNotDefinedError", func(t *testing.T) {
+		mr := &mockState{GetStateMetadataErr: &ledger.CollConfigNotDefinedError{Ns: "mycc"}}
+		ms := &mockStateFetcher{FetchStateRv: mr}
+		pm := &KeyLevelValidationParameterManagerImpl{StateFetcher: ms}
+		validator := NewKeyLevelValidator(&mockPolicyEvaluator{}, pm)
+
+		err := validator.Validate("cc", 1, 0, rwsb, prp, []byte("CCEP"), []*pb.Endorsement{})
+		assert.NoError(t, err)
+	})
+
+	t.Run("InvalidCollNameError", func(t *testing.T) {
+		mr := &mockState{GetStateMetadataErr: &ledger.InvalidCollNameError{Ns: "mycc", Coll: "mycoll"}}
+		ms := &mockStateFetcher{FetchStateRv: mr}
+		pm := &KeyLevelValidationParameterManagerImpl{StateFetcher: ms}
+		validator := NewKeyLevelValidator(&mockPolicyEvaluator{}, pm)
+
+		err := validator.Validate("cc", 1, 0, rwsb, prp, []byte("CCEP"), []*pb.Endorsement{})
+		assert.NoError(t, err)
+	})
+
+	t.Run("I/O error", func(t *testing.T) {
+		mr := &mockState{GetStateMetadataErr: fmt.Errorf("some I/O error")}
+		ms := &mockStateFetcher{FetchStateRv: mr}
+		pm := &KeyLevelValidationParameterManagerImpl{StateFetcher: ms}
+		validator := NewKeyLevelValidator(&mockPolicyEvaluator{}, pm)
+
+		err := validator.Validate("cc", 1, 0, rwsb, prp, []byte("CCEP"), []*pb.Endorsement{})
+		assert.Error(t, err)
+		assert.IsType(t, &errors.VSCCExecutionFailureError{}, err)
+	})
+}
+
 func TestCCEPValidation(t *testing.T) {
 	t.Parallel()
 

core/ledger/kvledger/txmgmt/txmgr/lockbasedtxmgr/collection_val.go

@@ -6,10 +6,9 @@ SPDX-License-Identifier: Apache-2.0
 package lockbasedtxmgr
 
 import (
-	"fmt"
-
 	"github.com/golang/protobuf/proto"
 	"github.com/hyperledger/fabric/core/common/privdata"
+	"github.com/hyperledger/fabric/core/ledger"
 	"github.com/hyperledger/fabric/protos/common"
 )
 
@@ -38,7 +37,10 @@ func (v *collNameValidator) validateCollName(ns, coll string) error {
 		v.cache.populate(ns, conf)
 	}
 	if !v.cache.containsCollName(ns, coll) {
-		return &errInvalidCollName{ns, coll}
+		return &ledger.InvalidCollNameError{
+			Ns:   ns,
+			Coll: coll,
+		}
 	}
 	logger.Debugf("validateCollName() validated successfully - ns=[%s], coll=[%s]", ns, coll)
 	return nil
@@ -51,7 +53,7 @@ func (v *collNameValidator) retrieveCollConfigFromStateDB(ns string) (*common.Co
 		return nil, err
 	}
 	if configPkgBytes == nil {
-		return nil, &errCollConfigNotDefined{ns}
+		return nil, &ledger.CollConfigNotDefinedError{Ns: ns}
 	}
 	confPkg := &common.CollectionConfigPackage{}
 	if err := proto.Unmarshal(configPkgBytes, confPkg); err != nil {
@@ -91,19 +93,3 @@ func (c collConfigCache) containsCollName(ns, coll string) bool {
 func constructCollectionConfigKey(chaincodeName string) string {
 	return privdata.BuildCollectionKVSKey(chaincodeName)
 }
-
-type errInvalidCollName struct {
-	ns, coll string
-}
-
-func (e *errInvalidCollName) Error() string {
-	return fmt.Sprintf("collection [%s] not defined in the collection config for chaincode [%s]", e.coll, e.ns)
-}
-
-type errCollConfigNotDefined struct {
-	ns string
-}
-
-func (e *errCollConfigNotDefined) Error() string {
-	return fmt.Sprintf("collection config not defined for chaincode [%s], pass the collection configuration upon chaincode definition/instantiation", e.ns)
-}

core/ledger/kvledger/txmgmt/txmgr/lockbasedtxmgr/collection_val_test.go

@@ -8,6 +8,7 @@ package lockbasedtxmgr
 import (
 	"testing"
 
+	"github.com/hyperledger/fabric/core/ledger"
 	"github.com/hyperledger/fabric/core/ledger/kvledger/txmgmt/version"
 	"github.com/stretchr/testify/assert"
 )
@@ -31,19 +32,19 @@ func TestCollectionValidation(t *testing.T) {
 	assert.NoError(t, err)
 
 	_, err = sim.GetPrivateData("ns3", "coll1", "key1")
-	_, ok := err.(*errCollConfigNotDefined)
+	_, ok := err.(*ledger.CollConfigNotDefinedError)
 	assert.True(t, ok)
 
 	err = sim.SetPrivateData("ns3", "coll1", "key1", []byte("val1"))
-	_, ok = err.(*errCollConfigNotDefined)
+	_, ok = err.(*ledger.CollConfigNotDefinedError)
 	assert.True(t, ok)
 
 	_, err = sim.GetPrivateData("ns1", "coll3", "key1")
-	_, ok = err.(*errInvalidCollName)
+	_, ok = err.(*ledger.InvalidCollNameError)
 	assert.True(t, ok)
 
 	err = sim.SetPrivateData("ns1", "coll3", "key1", []byte("val1"))
-	_, ok = err.(*errInvalidCollName)
+	_, ok = err.(*ledger.InvalidCollNameError)
 	assert.True(t, ok)
 
 	err = sim.SetPrivateData("ns1", "coll1", "key1", []byte("val1"))
@@ -60,7 +61,7 @@ func TestPvtGetNoCollection(t *testing.T) {
 	assert.Nil(t, valueHash)
 	assert.Nil(t, metadataBytes)
 	assert.Error(t, err)
-	assert.IsType(t, &errCollConfigNotDefined{}, err)
+	assert.IsType(t, &ledger.CollConfigNotDefinedError{}, err)
 }
 
 func TestPvtPutNoCollection(t *testing.T) {
@@ -72,5 +73,5 @@ func TestPvtPutNoCollection(t *testing.T) {
 	assert.NoError(t, err)
 	err = txsim.SetPrivateDataMetadata("cc", "coll", "key", map[string][]byte{})
 	assert.Error(t, err)
-	assert.IsType(t, &errCollConfigNotDefined{}, err)
+	assert.IsType(t, &ledger.CollConfigNotDefinedError{}, err)
 }

core/ledger/ledger_interface.go

@@ -7,6 +7,8 @@ SPDX-License-Identifier: Apache-2.0
 package ledger
 
 import (
+	"fmt"
+
 	"github.com/golang/protobuf/proto"
 	commonledger "github.com/hyperledger/fabric/common/ledger"
 	"github.com/hyperledger/fabric/protos/common"
@@ -418,6 +420,26 @@ func (NotFoundInIndexErr) Error() string {
 	return "Entry not found in index"
 }
 
+// CollConfigNotDefinedError is returned whenever an operation
+// is requested on a collection whose config has not been defined
+type CollConfigNotDefinedError struct {
+	Ns string
+}
+
+func (e *CollConfigNotDefinedError) Error() string {
+	return fmt.Sprintf("collection config not defined for chaincode [%s], pass the collection configuration upon chaincode definition/instantiation", e.Ns)
+}
+
+// InvalidCollNameError is returned whenever an operation
+// is requested on a collection whose name is invalid
+type InvalidCollNameError struct {
+	Ns, Coll string
+}
+
+func (e *InvalidCollNameError) Error() string {
+	return fmt.Sprintf("collection [%s] not defined in the collection config for chaincode [%s]", e.Coll, e.Ns)
+}
+
 // PvtdataHashMismatch is used when the hash of private write-set
 // does not match the corresponding hash present in the block
 // See function `PeerLedger.CommitPvtData` for the usages