Fix the issue of Nil/Zero-length-byte-array value (#2309)

Signed-off-by: manish <manish.sethi@gmail.com>

core/ledger/kvledger/history/db_test.go

@@ -13,8 +13,11 @@ import (
 	"strconv"
 	"testing"
 
+	"github.com/golang/protobuf/proto"
 	"github.com/hyperledger/fabric-protos-go/common"
 	"github.com/hyperledger/fabric-protos-go/ledger/queryresult"
+	"github.com/hyperledger/fabric-protos-go/ledger/rwset"
+	"github.com/hyperledger/fabric-protos-go/ledger/rwset/kvrwset"
 	"github.com/hyperledger/fabric-protos-go/peer"
 	configtxtest "github.com/hyperledger/fabric/common/configtx/test"
 	"github.com/hyperledger/fabric/common/flogging"
@@ -502,6 +505,61 @@ func TestDrop(t *testing.T) {
 	require.EqualError(t, env.testHistoryDBProvider.Drop("ledger2"), "internal leveldb error while obtaining db iterator: leveldb: closed")
 }
 
+// TestHistoryWithKVWriteOfNilValue - See FAB-18386 for details
+func TestHistoryWithKVWriteOfNilValue(t *testing.T) {
+	env := newTestHistoryEnv(t)
+	defer env.cleanup()
+	provider := env.testBlockStorageEnv.provider
+	store, err := provider.Open("ledger1")
+	require.NoError(t, err)
+	defer store.Shutdown()
+
+	bg, gb := testutil.NewBlockGenerator(t, "ledger1", false)
+
+	kvRWSet := &kvrwset.KVRWSet{
+		Writes: []*kvrwset.KVWrite{
+			// explicitly set IsDelete to false while the value to nil. As this will never be generated by simulation
+			{Key: "key1", IsDelete: false, Value: nil},
+		},
+	}
+	kvRWsetBytes, err := proto.Marshal(kvRWSet)
+	require.NoError(t, err)
+
+	txRWSet := &rwset.TxReadWriteSet{
+		NsRwset: []*rwset.NsReadWriteSet{
+			{
+				Namespace: "ns1",
+				Rwset:     kvRWsetBytes,
+			},
+		},
+	}
+
+	txRWSetBytes, err := proto.Marshal(txRWSet)
+	require.NoError(t, err)
+
+	block1 := bg.NextBlockWithTxid([][]byte{txRWSetBytes}, []string{"txid1"})
+
+	historydb := env.testHistoryDBProvider.GetDBHandle("ledger1")
+	require.NoError(t, store.AddBlock(gb))
+	require.NoError(t, historydb.Commit(gb))
+	require.NoError(t, store.AddBlock(block1))
+	require.NoError(t, historydb.Commit(block1))
+
+	historydbQE, err := historydb.NewQueryExecutor(store)
+	require.NoError(t, err)
+	itr, err := historydbQE.GetHistoryForKey("ns1", "key1")
+	require.NoError(t, err)
+	kmod, err := itr.Next()
+	require.NoError(t, err)
+	keyModification := kmod.(*queryresult.KeyModification)
+	// despite IsDelete set to "false" in the write-set, historydb results should set this to "true"
+	require.True(t, keyModification.IsDelete)
+
+	kmod, err = itr.Next()
+	require.NoError(t, err)
+	require.Nil(t, kmod)
+}
+
 // verify history results
 func testutilVerifyResults(t *testing.T, hqe ledger.HistoryQueryExecutor, ns, key string, expectedVals []string) {
 	itr, err := hqe.GetHistoryForKey(ns, key)

core/ledger/kvledger/history/query_executer.go

@@ -135,7 +135,7 @@ func getKeyModificationFromTran(tranEnvelope *common.Envelope, namespace string,
 			for _, kvWrite := range nsRWSet.KvRwSet.Writes {
 				if kvWrite.Key == key {
 					return &queryresult.KeyModification{TxId: txID, Value: kvWrite.Value,
-						Timestamp: timestamp, IsDelete: kvWrite.IsDelete}, nil
+						Timestamp: timestamp, IsDelete: rwsetutil.IsKVWriteDelete(kvWrite)}, nil
 				}
 			} // end keys loop
 			logger.Debugf("key [%s] not found in namespace [%s]'s writeset", key, namespace)

core/ledger/kvledger/tests/client.go

@@ -51,6 +51,10 @@ func (c *client) simulateDataTx(txid string, simulationLogic func(s *simulator))
 	return txAndPvtdata
 }
 
+func (c *client) submitHandCraftedTx(txAndPvtdata *txAndPvtdata) {
+	c.simulatedTrans = append(c.simulatedTrans, txAndPvtdata)
+}
+
 func (c *client) addPostOrderTx(txid string, customTxType common.HeaderType) *txAndPvtdata {
 	if txid == "" {
 		txid = util.GenerateUUID()

core/ledger/kvledger/tests/nilvalue_no_delete_marker_test.go

@@ -0,0 +1,148 @@
+/*
+Copyright IBM Corp. All Rights Reserved.
+
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package tests
+
+import (
+	"testing"
+
+	"github.com/golang/protobuf/proto"
+	"github.com/hyperledger/fabric-protos-go/ledger/rwset"
+	"github.com/hyperledger/fabric-protos-go/ledger/rwset/kvrwset"
+	"github.com/hyperledger/fabric-protos-go/peer"
+	"github.com/hyperledger/fabric/common/util"
+	"github.com/stretchr/testify/require"
+)
+
+// TestNilValNoDeleteMarker tests for a special writeset which carries a nil value and yet the delete marker is set to false.
+// This kind of write-set gets produced in previous versions. See FAB-18386 for more details.
+func TestNilValNoDeleteMarker(t *testing.T) {
+	env := newEnv(t)
+	defer env.cleanup()
+	env.initLedgerMgmt()
+
+	testLedger := env.createTestLedgerFromGenesisBlk("test-ledger")
+	testLedger.simulateDeployTx("cc1", []*collConf{
+		{
+			name: "coll1",
+		},
+	})
+	testLedger.cutBlockAndCommitLegacy()
+
+	testLedger.simulateDataTx("txid1", func(s *simulator) {
+		s.setState("cc1", "pubKey1", "pubValue1")
+		s.setPvtdata("cc1", "coll1", "pvtKey1", "pvtValue1")
+		s.setPvtdata("cc1", "coll1", "pvtKey2", "pvtValue2")
+	})
+	testLedger.cutBlockAndCommitLegacy()
+
+	testLedger.verifyPubState("cc1", "pubKey1", "pubValue1")
+	testLedger.verifyPvtdataHashState("cc1", "coll1", "pvtKey1", util.ComputeSHA256([]byte("pvtValue1")))
+	testLedger.verifyPvtdataHashState("cc1", "coll1", "pvtKey2", util.ComputeSHA256([]byte("pvtValue2")))
+	testLedger.verifyPvtState("cc1", "coll1", "pvtKey1", "pvtValue1")
+	testLedger.verifyPvtState("cc1", "coll1", "pvtKey2", "pvtValue2")
+
+	// Handcraft writeset that includes a delete for each of the above keys. We handcraft here because the one generated by the simulator
+	// will always have IsDelete flag true and we want to test when this flag is set to false and the actual value is nil. See FAB-18386 for
+	// more details.
+	pubWrites := &kvrwset.KVRWSet{
+		Writes: []*kvrwset.KVWrite{
+			{
+				Key:      "pubKey1",
+				IsDelete: false,
+				Value:    nil,
+			},
+		},
+	}
+
+	hashedWrites := &kvrwset.HashedRWSet{
+		HashedWrites: []*kvrwset.KVWriteHash{
+			{
+				KeyHash:   util.ComputeSHA256([]byte("pvtKey1")),
+				IsDelete:  false,
+				ValueHash: nil,
+			},
+			{
+				KeyHash:   util.ComputeSHA256([]byte("pvtKey2")),
+				IsDelete:  false,
+				ValueHash: util.ComputeSHA256([]byte{}),
+			},
+		},
+	}
+
+	pvtWrites := &kvrwset.KVRWSet{
+		Writes: []*kvrwset.KVWrite{
+			{
+				Key:      "pvtKey1",
+				IsDelete: false,
+			},
+			{
+				Key:      "pvtKey2",
+				IsDelete: false,
+			},
+		},
+	}
+
+	pubWritesBytes, err := proto.Marshal(pubWrites)
+	require.NoError(t, err)
+
+	hashedWritesBytes, err := proto.Marshal(hashedWrites)
+	require.NoError(t, err)
+
+	pvtWritesBytes, err := proto.Marshal(pvtWrites)
+	require.NoError(t, err)
+
+	pubRwset := &rwset.TxReadWriteSet{
+		DataModel: rwset.TxReadWriteSet_KV,
+		NsRwset: []*rwset.NsReadWriteSet{
+			{
+				Namespace: "cc1",
+				Rwset:     pubWritesBytes,
+				CollectionHashedRwset: []*rwset.CollectionHashedReadWriteSet{
+					{
+						CollectionName: "coll1",
+						HashedRwset:    hashedWritesBytes,
+						PvtRwsetHash:   util.ComputeSHA256(pvtWritesBytes),
+					},
+				},
+			},
+		},
+	}
+	pubRwsetBytes, err := proto.Marshal(pubRwset)
+	require.NoError(t, err)
+	envelope, err := constructTransaction("txid2", pubRwsetBytes)
+	require.NoError(t, err)
+
+	txAndPvtdata := &txAndPvtdata{
+		Txid:     "txid2",
+		Envelope: envelope,
+		Pvtws: &rwset.TxPvtReadWriteSet{
+			DataModel: rwset.TxReadWriteSet_KV,
+			NsPvtRwset: []*rwset.NsPvtReadWriteSet{
+				{
+					Namespace: "cc1",
+					CollectionPvtRwset: []*rwset.CollectionPvtReadWriteSet{
+						{
+							CollectionName: "coll1",
+							Rwset:          pvtWritesBytes,
+						},
+					},
+				},
+			},
+		},
+	}
+
+	testLedger.submitHandCraftedTx(txAndPvtdata)
+	testLedger.cutBlockAndCommitLegacy()
+
+	testLedger.verifyTxValidationCode("txid2", peer.TxValidationCode_VALID)
+	testLedger.verifyPubState("cc1", "pubKey1", "")
+	testLedger.verifyPvtdataHashState("cc1", "coll1", "pvtKey1", nil)
+	testLedger.verifyPvtdataHashState("cc1", "coll1", "pvtKey2", nil)
+	testLedger.verifyPvtState("cc1", "coll1", "pvtKey1", "")
+	testLedger.verifyPvtState("cc1", "coll1", "pvtKey2", "")
+	testLedger.verifyHistory("cc1", "pubKey1", []string{"", "pubValue1"})
+}

core/ledger/kvledger/txmgmt/rwsetutil/rwset_builder_test.go

@@ -357,3 +357,71 @@ func serializeTestProtoMsg(t *testing.T, protoMsg proto.Message) []byte {
 	require.NoError(t, err)
 	return msgBytes
 }
+
+func TestNilOrZeroLengthByteArrayValueConvertedToDelete(t *testing.T) {
+	t.Run("public_writeset", func(t *testing.T) {
+		rwsetBuilder := NewRWSetBuilder()
+		rwsetBuilder.AddToWriteSet("ns", "key1", nil)
+		rwsetBuilder.AddToWriteSet("ns", "key2", []byte{})
+
+		simulationResults, err := rwsetBuilder.GetTxSimulationResults()
+		require.NoError(t, err)
+		pubRWSet := &kvrwset.KVRWSet{}
+		require.NoError(
+			t,
+			proto.Unmarshal(simulationResults.PubSimulationResults.NsRwset[0].Rwset, pubRWSet),
+		)
+		require.True(t, proto.Equal(
+			&kvrwset.KVRWSet{
+				Writes: []*kvrwset.KVWrite{
+					{Key: "key1", IsDelete: true},
+					{Key: "key2", IsDelete: true},
+				},
+			},
+			pubRWSet,
+		))
+	})
+
+	t.Run("pvtdata_and_hashes_writesets", func(t *testing.T) {
+		rwsetBuilder := NewRWSetBuilder()
+		rwsetBuilder.AddToPvtAndHashedWriteSet("ns", "coll", "key1", nil)
+		rwsetBuilder.AddToPvtAndHashedWriteSet("ns", "coll", "key2", []byte{})
+
+		simulationResults, err := rwsetBuilder.GetTxSimulationResults()
+		require.NoError(t, err)
+
+		t.Run("hashed_writeset", func(t *testing.T) {
+			hashedRWSet := &kvrwset.HashedRWSet{}
+			require.NoError(
+				t,
+				proto.Unmarshal(simulationResults.PubSimulationResults.NsRwset[0].CollectionHashedRwset[0].HashedRwset, hashedRWSet),
+			)
+			require.True(t, proto.Equal(
+				&kvrwset.HashedRWSet{
+					HashedWrites: []*kvrwset.KVWriteHash{
+						{KeyHash: util.ComputeStringHash("key1"), IsDelete: true},
+						{KeyHash: util.ComputeStringHash("key2"), IsDelete: true},
+					},
+				},
+				hashedRWSet,
+			))
+		})
+
+		t.Run("pvtdata_writeset", func(t *testing.T) {
+			pvtWSet := &kvrwset.KVRWSet{}
+			require.NoError(
+				t,
+				proto.Unmarshal(simulationResults.PvtSimulationResults.NsPvtRwset[0].CollectionPvtRwset[0].Rwset, pvtWSet),
+			)
+			require.True(t, proto.Equal(
+				&kvrwset.KVRWSet{
+					Writes: []*kvrwset.KVWrite{
+						{Key: "key1", IsDelete: true},
+						{Key: "key2", IsDelete: true},
+					},
+				},
+				pvtWSet,
+			))
+		})
+	})
+}

core/ledger/kvledger/txmgmt/rwsetutil/rwset_proto_util.go

@@ -17,6 +17,8 @@ limitations under the License.
 package rwsetutil
 
 import (
+	"bytes"
+
 	"github.com/golang/protobuf/proto"
 	"github.com/hyperledger/fabric-protos-go/ledger/rwset"
 	"github.com/hyperledger/fabric-protos-go/ledger/rwset/kvrwset"
@@ -343,7 +345,7 @@ func newProtoVersion(height *version.Height) *kvrwset.Version {
 }
 
 func newKVWrite(key string, value []byte) *kvrwset.KVWrite {
-	return &kvrwset.KVWrite{Key: key, IsDelete: value == nil, Value: value}
+	return &kvrwset.KVWrite{Key: key, IsDelete: len(value) == 0, Value: value}
 }
 
 func newPvtKVReadHash(key string, version *version.Height) *kvrwset.KVReadHash {
@@ -359,3 +361,17 @@ func newPvtKVWriteAndHash(key string, value []byte) (*kvrwset.KVWrite, *kvrwset.
 	}
 	return kvWrite, &kvrwset.KVWriteHash{KeyHash: keyHash, IsDelete: kvWrite.IsDelete, ValueHash: valueHash}
 }
+
+// IsKVWriteDelete returns true if the kvWrite indicates a delete operation. See FAB-18386 for details.
+func IsKVWriteDelete(kvWrite *kvrwset.KVWrite) bool {
+	return kvWrite.IsDelete || len(kvWrite.Value) == 0
+}
+
+var (
+	hashOfZeroLengthByteArray = util.ComputeHash([]byte{})
+)
+
+// IsKVWriteHashDelete returns true if the kvWriteHash indicates a delete operation. See FAB-18386 for details.
+func IsKVWriteHashDelete(kvWriteHash *kvrwset.KVWriteHash) bool {
+	return kvWriteHash.IsDelete || len(kvWriteHash.ValueHash) == 0 || bytes.Equal(hashOfZeroLengthByteArray, kvWriteHash.ValueHash)
+}

core/ledger/kvledger/txmgmt/rwsetutil/rwset_proto_util_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/golang/protobuf/proto"
 	"github.com/hyperledger/fabric-protos-go/ledger/rwset/kvrwset"
 	"github.com/hyperledger/fabric/core/ledger/internal/version"
+	"github.com/hyperledger/fabric/core/ledger/util"
 	"github.com/kr/pretty"
 	"github.com/stretchr/testify/require"
 )
@@ -241,3 +242,33 @@ func TestVersionConversion(t *testing.T) {
 	require.Nil(t, newProtoVersion(nil))
 	require.Equal(t, protoVer, newProtoVersion(internalVer))
 }
+
+func TestIsDelete(t *testing.T) {
+	t.Run("kvWrite", func(t *testing.T) {
+		kvWritesToBeInterpretedAsDelete := []*kvrwset.KVWrite{
+			{Value: nil, IsDelete: true},
+			{Value: nil, IsDelete: false},
+			{Value: []byte{}, IsDelete: true},
+			{Value: []byte{}, IsDelete: false},
+		}
+
+		for _, k := range kvWritesToBeInterpretedAsDelete {
+			require.True(t, IsKVWriteDelete(k))
+		}
+	})
+
+	t.Run("kvhashwrite", func(t *testing.T) {
+		kvHashesWritesToBeInterpretedAsDelete := []*kvrwset.KVWriteHash{
+			{ValueHash: nil, IsDelete: true},
+			{ValueHash: nil, IsDelete: false},
+			{ValueHash: []byte{}, IsDelete: true},
+			{ValueHash: []byte{}, IsDelete: false},
+			{ValueHash: util.ComputeHash([]byte{}), IsDelete: true},
+			{ValueHash: util.ComputeHash([]byte{}), IsDelete: false},
+		}
+
+		for _, k := range kvHashesWritesToBeInterpretedAsDelete {
+			require.True(t, IsKVWriteHashDelete(k))
+		}
+	})
+}