Add check for verifying block.Header.PreviousHash field

In the current code, before adding a block to the ledger,
ledger verifies the correct sequence number of the block
just to double check that gossip is delivering the right
sequence of blocks. In addition, before block is delivered
to ledger, the signatures of the ordering service node(s)
are verified that fulfills the requirement of the trust model.

Though the above checks are sufficient for keeping an unintended
block from getting committed to ledger. Still, it may not be a
bad idea to add an additional check that verifies that the field
`block.Header.PreviousHash` present in the block matches with
the hash of the last committed block.

This check is a simple bytes comparison and hence does not
cause any observable performance penalty and may help in
detecting a rare scenario if there is any bug in the
ordering service.

FAB-12033 #done

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

common/ledger/blkstorage/fsblkstorage/blockfile_mgr.go

@@ -7,13 +7,13 @@ SPDX-License-Identifier: Apache-2.0
 package fsblkstorage
 
 import (
+	"bytes"
 	"fmt"
 	"math"
 	"sync"
 	"sync/atomic"
 
 	"github.com/davecgh/go-spew/spew"
-
 	"github.com/golang/protobuf/proto"
 	"github.com/hyperledger/fabric/common/flogging"
 	"github.com/hyperledger/fabric/common/ledger/blkstorage"
@@ -238,8 +238,23 @@ func (mgr *blockfileMgr) moveToNextFile() {
 }
 
 func (mgr *blockfileMgr) addBlock(block *common.Block) error {
-	if block.Header.Number != mgr.getBlockchainInfo().Height {
-		return errors.Errorf("block number should have been %d but was %d", mgr.getBlockchainInfo().Height, block.Header.Number)
+	bcInfo := mgr.getBlockchainInfo()
+	if block.Header.Number != bcInfo.Height {
+		return errors.Errorf(
+			"block number should have been %d but was %d",
+			mgr.getBlockchainInfo().Height, block.Header.Number,
+		)
+	}
+
+	// Add the previous hash check - Though, not essential but may not be a bad idea to
+	// verify the field `block.Header.PreviousHash` present in the block.
+	// This check is a simple bytes comparison and hence does not cause any observable performance penalty
+	// and may help in detecting a rare scenario if there is any bug in the ordering service.
+	if !bytes.Equal(block.Header.PreviousHash, bcInfo.CurrentBlockHash) {
+		return errors.Errorf(
+			"unexpected Previous block hash. Expected PreviousHash = [%x], PreviousHash referred in the latest block= [%x]",
+			bcInfo.CurrentBlockHash, block.Header.PreviousHash,
+		)
 	}
 	blockBytes, info, err := serializeBlock(block)
 	if err != nil {

common/ledger/blkstorage/fsblkstorage/blockfile_mgr_test.go

@@ -18,6 +18,7 @@ package fsblkstorage
 
 import (
 	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/golang/protobuf/proto"
@@ -40,6 +41,21 @@ func TestBlockfileMgrBlockReadWrite(t *testing.T) {
 	blkfileMgrWrapper.testGetBlockByNumber(blocks, 0)
 }
 
+func TestAddBlockWithWrongHash(t *testing.T) {
+	env := newTestEnv(t, NewConf(testPath(), 0))
+	defer env.Cleanup()
+	blkfileMgrWrapper := newTestBlockfileWrapper(env, "testLedger")
+	defer blkfileMgrWrapper.close()
+	blocks := testutil.ConstructTestBlocks(t, 10)
+	blkfileMgrWrapper.addBlocks(blocks[0:9])
+	lastBlock := blocks[9]
+	lastBlock.Header.PreviousHash = []byte("someJunkHash") // set the hash to something unexpected
+	err := blkfileMgrWrapper.blockfileMgr.addBlock(lastBlock)
+	testutil.AssertError(t, err, "An error is expected when adding a block with some unexpected hash")
+	testutil.AssertEquals(t, strings.Contains(err.Error(), "unexpected Previous block hash. Expected PreviousHash"), true)
+	t.Logf("err = %s", err)
+}
+
 func TestBlockfileMgrCrashDuringWriting(t *testing.T) {
 	testBlockfileMgrCrashDuringWriting(t, 10, 2, 1000, 10)
 	testBlockfileMgrCrashDuringWriting(t, 10, 2, 1000, 1)

core/chaincode/exectransaction_test.go

@@ -304,7 +304,11 @@ func endTxSimulation(chainID string, ccid *pb.ChaincodeID, txsim ledger.TxSimula
 			}
 
 			//create the block with 1 transaction
-			block := common.NewBlock(blockNumber, []byte{})
+			bcInfo, err := lgr.GetBlockchainInfo()
+			if err != nil {
+				return err
+			}
+			block := common.NewBlock(blockNumber, bcInfo.CurrentBlockHash)
 			block.Data.Data = [][]byte{envBytes}
 			txsFilter := cut.NewTxValidationFlagsSetValue(len(block.Data.Data), pb.TxValidationCode_VALID)
 			block.Metadata.Metadata[common.BlockMetadataIndex_TRANSACTIONS_FILTER] = txsFilter

core/committer/txvalidator/validator_test.go

@@ -182,7 +182,9 @@ func putCCInfoWithVSCCAndVer(theLedger ledger.PeerLedger, ccname, vscc, ver stri
 	assert.NoError(t, err)
 	pubSimulationBytes, err := simRes.GetPubSimulationBytes()
 	assert.NoError(t, err)
-	block0 := testutil.ConstructBlock(t, 1, []byte("hash"), [][]byte{pubSimulationBytes}, true)
+	bcInfo, err := theLedger.GetBlockchainInfo()
+	assert.NoError(t, err)
+	block0 := testutil.ConstructBlock(t, 1, bcInfo.CurrentBlockHash, [][]byte{pubSimulationBytes}, true)
 	err = theLedger.CommitWithPvtData(&ledger.BlockAndPvtData{
 		Block: block0,
 	})
@@ -202,7 +204,9 @@ func putSBEP(theLedger ledger.PeerLedger, cc, key string, policy []byte, t *test
 	assert.NoError(t, err)
 	pubSimulationBytes, err := simRes.GetPubSimulationBytes()
 	assert.NoError(t, err)
-	block0 := testutil.ConstructBlock(t, 2, []byte("hash"), [][]byte{pubSimulationBytes}, true)
+	bcInfo, err := theLedger.GetBlockchainInfo()
+	assert.NoError(t, err)
+	block0 := testutil.ConstructBlock(t, 2, bcInfo.CurrentBlockHash, [][]byte{pubSimulationBytes}, true)
 	err = theLedger.CommitWithPvtData(&ledger.BlockAndPvtData{
 		Block: block0,
 	})

core/ledger/kvledger/tests/committer.go

@@ -71,7 +71,7 @@ func newBlockGenerator(lgr ledger.PeerLedger, t *testing.T) *blkGenerator {
 	assert := assert.New(t)
 	info, err := lgr.GetBlockchainInfo()
 	assert.NoError(err)
-	return &blkGenerator{info.Height - 1, info.PreviousBlockHash, assert}
+	return &blkGenerator{info.Height - 1, info.CurrentBlockHash, assert}
 }
 
 // nextBlockAndPvtdata cuts the next block

core/scc/qscc/query_test.go

@@ -329,7 +329,6 @@ func TestQueryGeneratedBlock(t *testing.T) {
 }
 
 func addBlockForTesting(t *testing.T, chainid string) *common.Block {
-	bg, _ := testutil.NewBlockGenerator(t, chainid, false)
 	ledger := peer.GetLedger(chainid)
 	defer ledger.Close()
 
@@ -351,9 +350,10 @@ func addBlockForTesting(t *testing.T, chainid string) *common.Block {
 	simRes2, _ := simulator.GetTxSimulationResults()
 	pubSimResBytes2, _ := simRes2.GetPubSimulationBytes()
 
-	block1 := bg.NextBlock([][]byte{pubSimResBytes1, pubSimResBytes2})
+	bcInfo, err := ledger.GetBlockchainInfo()
+	assert.NoError(t, err)
+	block1 := testutil.ConstructBlock(t, 1, bcInfo.CurrentBlockHash, [][]byte{pubSimResBytes1, pubSimResBytes2}, false)
 	ledger.CommitWithPvtData(&ledger2.BlockAndPvtData{Block: block1})
-
 	return block1
 }