consortium-v2: fix wrong parenthesis placement in non-epoch block che…

…ck (#470) * consortium-v2: make error in verifyValidatorFieldsInExtraData more verbose * consortium-v2: add more unit tests for verifyValidatorFieldsInExtraData * consortium-v2: fix wrong parenthesis placement in non-epoch block check Fix wrong parenthesis placement in non-epoch block check in verifyValidatorFieldsInExtraData which leads to incorrect check result.
axieinfinity · Jun 14, 2024 · f615759 · f615759
1 parent 75b849d
commit f615759
Show file tree

Hide file tree

Showing 3 changed files with 120 additions and 21 deletions.
diff --git a/consensus/consortium/common/constants.go b/consensus/consortium/common/constants.go
@@ -53,4 +53,18 @@ var (
 	// ErrWrongDifficulty is returned if the difficulty of a block doesn't match the
 	// turn of the signer.
 	ErrWrongDifficulty = errors.New("wrong difficulty")
+
+	ErrNonEpochExtraData = errors.New("non epoch block contains invalid validator fields")
+
+	ErrAaronEpochExtraData = errors.New("aaron epoch block contains invalid validator fields")
+
+	ErrTrippEpochExtraData = errors.New("tripp epoch block contains invalid validator fields")
+
+	ErrPeriodBlockExtraData = errors.New("period block contains empty checkpoint validator fields")
+
+	ErrNonPeriodBlockExtraData = errors.New("non-period block contains non-empty checkpoint validator fields")
+
+	ErrPreTrippEpochExtraData = errors.New("pre-tripp epoch block contains empty checkpoint validator fields")
+
+	ErrPreTrippEpochProducerExtraData = errors.New("pre-tripp epoch block contains invalid block producer fields")
 )
diff --git a/consensus/consortium/v2/consortium.go b/consensus/consortium/v2/consortium.go
@@ -361,33 +361,68 @@ func (c *Consortium) verifyValidatorFieldsInExtraData(
 	header *types.Header,
 ) error {
 	isEpoch := header.Number.Uint64()%c.config.EpochV2 == 0 || c.chainConfig.IsOnConsortiumV2(header.Number)
-	if !isEpoch && (len(extraData.CheckpointValidators) != 0 || len(extraData.BlockProducers) != 0) || extraData.BlockProducersBitSet != 0 {
-		return consortiumCommon.ErrExtraValidators
+	if !isEpoch {
+		if len(extraData.CheckpointValidators) != 0 || len(extraData.BlockProducers) != 0 || extraData.BlockProducersBitSet != 0 {
+			return fmt.Errorf(
+				"%w: checkpoint validator: %v, block producer: %v, block producer bitset: %v",
+				consortiumCommon.ErrNonEpochExtraData,
+				extraData.CheckpointValidators,
+				extraData.BlockProducers,
+				extraData.BlockProducersBitSet,
+			)
+		}
 	}
 
 	if c.IsTrippEffective(chain, header) {
 		if c.chainConfig.IsAaron(header.Number) {
 			if isEpoch && (extraData.BlockProducersBitSet == 0 || len(extraData.BlockProducers) != 0) {
-				return consortiumCommon.ErrExtraValidators
+				return fmt.Errorf(
+					"%w: block producer: %v, block producer bitset: %v",
+					consortiumCommon.ErrAaronEpochExtraData,
+					extraData.BlockProducers,
+					extraData.BlockProducersBitSet,
+				)
 			}
 		} else if isEpoch && (extraData.BlockProducersBitSet != 0 || len(extraData.BlockProducers) == 0) {
-			return consortiumCommon.ErrExtraValidators
+			return fmt.Errorf(
+				"%w: block producer: %v, block producer bitset: %v",
+				consortiumCommon.ErrTrippEpochExtraData,
+				extraData.BlockProducers,
+				extraData.BlockProducersBitSet,
+			)
 		}
 		if c.IsPeriodBlock(chain, header) {
 			if len(extraData.CheckpointValidators) == 0 {
-				return consortiumCommon.ErrExtraValidators
+				return fmt.Errorf(
+					"%w: checkpoint validator: %v",
+					consortiumCommon.ErrPeriodBlockExtraData,
+					extraData.CheckpointValidators,
+				)
 			}
 		} else {
 			if len(extraData.CheckpointValidators) != 0 {
-				return consortiumCommon.ErrExtraValidators
+				return fmt.Errorf(
+					"%w: checkpoint validator: %v",
+					consortiumCommon.ErrNonPeriodBlockExtraData,
+					extraData.CheckpointValidators,
+				)
 			}
 		}
 	} else {
 		if isEpoch && len(extraData.CheckpointValidators) == 0 {
-			return consortiumCommon.ErrExtraValidators
+			return fmt.Errorf(
+				"%w: checkpoint validator: %v",
+				consortiumCommon.ErrPreTrippEpochExtraData,
+				extraData.CheckpointValidators,
+			)
 		}
 		if len(extraData.BlockProducers) != 0 || extraData.BlockProducersBitSet != 0 {
-			return consortiumCommon.ErrExtraValidators
+			return fmt.Errorf(
+				"%w: block producer: %v, block producer bitset: %v",
+				consortiumCommon.ErrPreTrippEpochProducerExtraData,
+				extraData.BlockProducers,
+				extraData.BlockProducersBitSet,
+			)
 		}
 	}
 	return nil
@@ -1769,10 +1804,6 @@ func (c *Consortium) IsTrippEffective(chain consensus.ChainHeaderReader, header
 		return c.testTrippEffective
 	}
 	if c.chainConfig.IsTripp(header.Number) {
-		if c.testTrippEffective {
-			return true
-		}
-
 		// When Tripp has been effective for long enough, we return true without any additional checks.
 		if header.Number.Uint64() > c.chainConfig.TrippBlock.Uint64()+28800 {
 			return true

diff --git a/consensus/consortium/v2/consortium_test.go b/consensus/consortium/v2/consortium_test.go
@@ -855,6 +855,7 @@ func TestVerifyFinalitySignatureTripp(t *testing.T) {
 			EpochV2: 300,
 		},
 		recents:            recents,
+		isTest:             true,
 		testTrippEffective: true,
 	}
 	snap.Hash = blockHash
@@ -1311,6 +1312,7 @@ func TestAssembleFinalityVoteTripp(t *testing.T) {
 	c := Consortium{
 		chainConfig:        &chainConfig,
 		votePool:           &mock,
+		isTest:             true,
 		testTrippEffective: true,
 	}
 
@@ -1979,6 +1981,7 @@ func TestUpgradeAxieProxyCode(t *testing.T) {
 		config: &params.ConsortiumConfig{
 			EpochV2: 200,
 		},
+		isTest:             true,
 		testTrippEffective: true,
 	}
 
@@ -2386,7 +2389,7 @@ func TestIsTrippEffective(t *testing.T) {
 	}
 }
 
-func TestHeaderExtraDataCheckAfterTripp(t *testing.T) {
+func TestHeaderExtraDataCheck(t *testing.T) {
 	c := Consortium{
 		chainConfig: &params.ChainConfig{
 			TrippBlock: common.Big0,
@@ -2398,29 +2401,56 @@ func TestHeaderExtraDataCheckAfterTripp(t *testing.T) {
 		testTrippEffective: true,
 	}
 
-	// Not an epoch block, every validator field must be empty
+	// Case 1: not an epoch block, every validator field must be empty
+	// non-empty checkpoint validators
 	header := types.Header{Number: big.NewInt(100)}
 	extraData := finality.HeaderExtraData{
 		CheckpointValidators: []finality.ValidatorWithBlsPub{
 			{},
 		},
 	}
 	err := c.verifyValidatorFieldsInExtraData(nil, &extraData, &header)
-	if err == nil {
-		t.Fatalf("Expect an error")
+	if !errors.Is(err, consortiumCommon.ErrNonEpochExtraData) {
+		t.Fatalf("Expect err: %v got: %v", consortiumCommon.ErrNonEpochExtraData, err)
+	}
+
+	// non-empty block producers
+	extraData = finality.HeaderExtraData{
+		BlockProducers: []common.Address{
+			{},
+		},
+	}
+	err = c.verifyValidatorFieldsInExtraData(nil, &extraData, &header)
+	if !errors.Is(err, consortiumCommon.ErrNonEpochExtraData) {
+		t.Fatalf("Expect err: %v got: %v", consortiumCommon.ErrNonEpochExtraData, err)
 	}
 
+	// non-empty block producer bitset
 	extraData = finality.HeaderExtraData{
+		BlockProducersBitSet: 10,
+	}
+	err = c.verifyValidatorFieldsInExtraData(nil, &extraData, &header)
+	if !errors.Is(err, consortiumCommon.ErrNonEpochExtraData) {
+		t.Fatalf("Expect err: %v got: %v", consortiumCommon.ErrNonEpochExtraData, err)
+	}
+
+	// Case 2: Not a period block, checkpoint validators must be empty
+	header = types.Header{Number: big.NewInt(200)}
+	extraData = finality.HeaderExtraData{
+		CheckpointValidators: []finality.ValidatorWithBlsPub{
+			{},
+		},
 		BlockProducers: []common.Address{
 			{},
 		},
 	}
 	err = c.verifyValidatorFieldsInExtraData(nil, &extraData, &header)
-	if err == nil {
-		t.Fatalf("Expect an error")
+	if !errors.Is(err, consortiumCommon.ErrNonPeriodBlockExtraData) {
+		t.Fatalf("Expect err: %v got: %v", consortiumCommon.ErrNonPeriodBlockExtraData, err)
 	}
 
-	// Not a period block, checkpoint validators must be empty
+	// Case 3: Before Tripp effective, block producer, block producer bitset must be empty
+	c.testTrippEffective = false
 	header = types.Header{Number: big.NewInt(200)}
 	extraData = finality.HeaderExtraData{
 		CheckpointValidators: []finality.ValidatorWithBlsPub{
@@ -2431,8 +2461,32 @@ func TestHeaderExtraDataCheckAfterTripp(t *testing.T) {
 		},
 	}
 	err = c.verifyValidatorFieldsInExtraData(nil, &extraData, &header)
-	if err == nil {
-		t.Fatalf("Expect an error")
+	if !errors.Is(err, consortiumCommon.ErrPreTrippEpochProducerExtraData) {
+		t.Fatalf("Expect err: %v got: %v", consortiumCommon.ErrPreTrippEpochProducerExtraData, err)
+	}
+
+	header = types.Header{Number: big.NewInt(200)}
+	extraData = finality.HeaderExtraData{
+		CheckpointValidators: []finality.ValidatorWithBlsPub{
+			{},
+		},
+		BlockProducersBitSet: 5,
+	}
+	err = c.verifyValidatorFieldsInExtraData(nil, &extraData, &header)
+	if !errors.Is(err, consortiumCommon.ErrPreTrippEpochProducerExtraData) {
+		t.Fatalf("Expect err: %v got: %v", consortiumCommon.ErrPreTrippEpochProducerExtraData, err)
+	}
+
+	// Case 4: A valid Aaron epoch block
+	c.chainConfig.AaronBlock = common.Big0
+	c.testTrippEffective = true
+	header = types.Header{Number: big.NewInt(200)}
+	extraData = finality.HeaderExtraData{
+		BlockProducersBitSet: 5,
+	}
+	err = c.verifyValidatorFieldsInExtraData(nil, &extraData, &header)
+	if err != nil {
+		t.Fatalf("Expect no error, got: %v", err)
 	}
 }