Don't mark bad merkle root as invalid (#1419)

* Don't mark bad merkle root as invalid

* Fix TestBlockStatus

* Move discardAllChanges inside the inner if

domain/consensus/processes/blockprocessor/validateandinsertblock_test.go

@@ -1,6 +1,8 @@
 package blockprocessor_test
 
 import (
+	"github.com/kaspanet/kaspad/domain/consensus/utils/constants"
+	"github.com/kaspanet/kaspad/domain/consensus/utils/merkle"
 	"strings"
 	"testing"
 
@@ -84,10 +86,11 @@ func TestBlockStatus(t *testing.T) {
 		if err != nil {
 			t.Fatalf("AddBlock: %+v", err)
 		}
+		invalidBlock.Transactions[0].Version = constants.MaxTransactionVersion + 1 // This should invalidate the block
 		invalidBlock.Header = blockheader.NewImmutableBlockHeader(
 			disqualifiedBlock.Header.Version(),
 			disqualifiedBlock.Header.ParentHashes(),
-			externalapi.NewDomainHashFromByteArray(&[externalapi.DomainHashSize]byte{}), // This should invalidate the block
+			merkle.CalculateHashMerkleRoot(invalidBlock.Transactions),
 			disqualifiedBlock.Header.AcceptedIDMerkleRoot(),
 			disqualifiedBlock.Header.UTXOCommitment(),
 			disqualifiedBlock.Header.TimeInMilliseconds(),

domain/consensus/processes/blockprocessor/validateblock.go

@@ -53,10 +53,14 @@ func (bp *blockProcessor) validateBlock(block *externalapi.DomainBlock, isPrunin
 	err = bp.validatePostProofOfWork(block, isPruningPoint)
 	if err != nil {
 		if errors.As(err, &ruleerrors.RuleError{}) {
-			bp.discardAllChanges()
 			// If we got ErrMissingParents the block shouldn't be considered as invalid
-			// because it could be added later on when its parents are present.
-			if !errors.As(err, &ruleerrors.ErrMissingParents{}) {
+			// because it could be added later on when its parents are present, and if
+			// we get ErrBadMerkleRoot we shouldn't mark the block as invalid because
+			// later on we can get the block with transactions that fits the merkle
+			// root.
+			if !errors.As(err, &ruleerrors.ErrMissingParents{}) && !errors.Is(err, ruleerrors.ErrBadMerkleRoot) {
+				// Discard all changes so we save only the block status
+				bp.discardAllChanges()
 				hash := consensushashing.BlockHash(block)
 				bp.blockStatusStore.Stage(hash, externalapi.StatusInvalid)
 				commitErr := bp.commitAllChanges()

domain/consensus/processes/blockvalidator/block_body_in_isolation.go

@@ -28,6 +28,11 @@ func (v *blockValidator) ValidateBodyInIsolation(blockHash *externalapi.DomainHa
 		return err
 	}
 
+	err = v.checkBlockHashMerkleRoot(block)
+	if err != nil {
+		return err
+	}
+
 	err = v.checkBlockSize(block)
 	if err != nil {
 		return err
@@ -63,11 +68,6 @@ func (v *blockValidator) ValidateBodyInIsolation(blockHash *externalapi.DomainHa
 		return err
 	}
 
-	err = v.checkBlockHashMerkleRoot(block)
-	if err != nil {
-		return err
-	}
-
 	err = v.checkBlockDuplicateTransactions(block)
 	if err != nil {
 		return err

domain/consensus/processes/blockvalidator/block_body_in_isolation_test.go

@@ -714,10 +714,10 @@ var blockWithWrongTxOrder = externalapi.DomainBlock{
 			}),
 		},
 		externalapi.NewDomainHashFromByteArray(&[externalapi.DomainHashSize]byte{
-			0xac, 0xa4, 0x21, 0xe1, 0xa6, 0xc3, 0xbe, 0x5d,
-			0x52, 0x66, 0xf3, 0x0b, 0x21, 0x87, 0xbc, 0xf3,
-			0xf3, 0x2d, 0xd1, 0x05, 0x64, 0xb5, 0x16, 0x76,
-			0xe4, 0x66, 0x7d, 0x51, 0x53, 0x18, 0x6d, 0xb1,
+			0xaf, 0xba, 0x3c, 0x18, 0x9e, 0x44, 0xac, 0x7c,
+			0xb7, 0xcc, 0x90, 0x0b, 0x75, 0x88, 0x6b, 0x9c,
+			0x3e, 0xc9, 0xea, 0xf1, 0x5c, 0xaf, 0x28, 0x30,
+			0x34, 0x23, 0xf5, 0xeb, 0x18, 0xdf, 0xd1, 0x75,
 		}),
 		externalapi.NewDomainHashFromByteArray(&[externalapi.DomainHashSize]byte{
 			0xa0, 0x69, 0x2d, 0x16, 0xb5, 0xd7, 0xe4, 0xf3,
@@ -1004,3 +1004,33 @@ var blockWithWrongTxOrder = externalapi.DomainBlock{
 		},
 	},
 }
+
+func TestCheckBlockHashMerkleRoot(t *testing.T) {
+	testutils.ForAllNets(t, true, func(t *testing.T, params *dagconfig.Params) {
+		factory := consensus.NewFactory()
+		tc, teardown, err := factory.NewTestConsensus(params, false, "TestCheckBlockHashMerkleRoot")
+		if err != nil {
+			t.Fatalf("Error setting up tc: %+v", err)
+		}
+		defer teardown(false)
+
+		block, _, err := tc.BuildBlockWithParents([]*externalapi.DomainHash{params.GenesisHash}, nil, nil)
+		if err != nil {
+			t.Fatalf("BuildBlockWithParents: %+v", err)
+		}
+		blockWithInvalidMerkleRoot := block.Clone()
+		blockWithInvalidMerkleRoot.Transactions[0].Version += 1
+
+		_, err = tc.ValidateAndInsertBlock(blockWithInvalidMerkleRoot)
+		if !errors.Is(err, ruleerrors.ErrBadMerkleRoot) {
+			t.Fatalf("Unexpected error: %+v", err)
+		}
+
+		// Check that a block with invalid merkle root is not marked as invalid
+		// and can be re-added with the right transactions.
+		_, err = tc.ValidateAndInsertBlock(block)
+		if err != nil {
+			t.Fatalf("ValidateAndInsertBlock: %+v", err)
+		}
+	})
+}