Migration from Raft to BFT test #4561

MayRosenbaum · 2023-11-30T16:27:53Z

Type of change

Test Improvement
Test update

Related Issue

#3773

Signed-off-by: Yacov Manevich <yacov.manevich@ibm.com>

orderer/consensus/etcdraft/chain.go

orderer/consensus/etcdraft/util.go

tock-ibm · 2023-12-11T15:46:19Z

orderer/consensus/etcdraft/chain_test.go

add a test case that emulates what happens when a raft chain receives the config block that changes from raft to BFT

orderer/consensus/etcdraft/util.go

tock-ibm · 2023-12-11T15:51:10Z

orderer/consensus/etcdraft/util.go

@@ -84,22 +86,32 @@ func MetadataHasDuplication(md *etcdraft.ConfigMetadata) error {
 }

 // MetadataFromConfigValue reads and translates configuration updates from config value into raft metadata
-func MetadataFromConfigValue(configValue *common.ConfigValue) (*etcdraft.ConfigMetadata, error) {
+func MetadataFromConfigValue(configValue *common.ConfigValue) (*etcdraft.ConfigMetadata, *orderer.ConsensusType, error) {


add a unit test

tock-ibm · 2023-12-11T15:57:27Z

orderer/consensus/etcdraft/chain.go

+			c.logger.Panicf("illegal consensus type detected during consensus metadata validation: %s", newOrdererConfig.ConsensusType())
+			panic("illegal consensus type detected during consensus metadata validation")


why panic? return error

orderer/common/msgprocessor/maintenancefilter.go

Signed-off-by: May Rosenbaum <mayro1595@gmail.com>

yacovm · 2024-01-09T14:37:03Z

orderer/common/msgprocessor/maintenancefilter.go

+			}
+
+			_, err := validateBFTMetadataOptions(1, updatedMetadata)
+			if updatedMetadata.XXX_unrecognized != nil || err != nil {


what is this XXX field thing check? This is prorobuf implementation specific, isn't it?

Once we upgrade the protobuf version, we might not have this field anymore.

That's right, we can skip this check, err check is enough.

True, the XXX fields are not required to preserve backwards compatibility going forward, so we better not use them.

verifying with validateBFTMetadataOptions should be enough, but see comment on verifying the consenters map as well.

yacovm · 2024-01-09T14:37:43Z

orderer/consensus/etcdraft/chain.go

+			c.logger.Infof("Detected migration to %s", consensusType.Type)
+			return nil
+		} else {
+			c.logger.Panicf("illegal consensus type detected: %s", consensusType.Type)


this is post consensus. Do we have a validation check for this condition pre-consensus anywhere?

otherwise, you know what will happen once we reach this point ;-)

We have one more check in the maintenance filter.

Yes, May is correct, we prevent it the maintenance filter.

tock-ibm · 2024-01-14T11:24:20Z

orderer/common/msgprocessor/maintenancefilter.go

+			}
+
+			_, err := validateBFTMetadataOptions(1, updatedMetadata)
+			if updatedMetadata.XXX_unrecognized != nil || err != nil {


True, the XXX fields are not required to preserve backwards compatibility going forward, so we better not use them.

orderer/common/msgprocessor/maintenancefilter.go

tock-ibm · 2024-01-14T11:27:41Z

orderer/common/msgprocessor/maintenancefilter.go

+			}
+
+			_, err := validateBFTMetadataOptions(1, updatedMetadata)
+			if updatedMetadata.XXX_unrecognized != nil || err != nil {


verifying with validateBFTMetadataOptions should be enough, but see comment on verifying the consenters map as well.

orderer/common/msgprocessor/maintenancefilter.go

tock-ibm · 2024-01-14T12:28:25Z

integration/raft/migration_test.go

+			block := FetchBlock(network, o1, 1, "testchannel")
+			Expect(block).NotTo(BeNil())
+
+			By("Change to maintenance mode")


before each step of the migration, add a numbered comment like:

// === Step 1: Config update, change to MAINTENANCE === By("1) ...")

as is done in the tests below. this helps keep track of what is going on in the logs.
be consistent about how these look.

orderer/common/msgprocessor/maintenancefilter.go

tock-ibm · 2024-01-14T13:04:31Z

orderer/consensus/etcdraft/util.go

+	if consensusTypeValue.Type != "etcdraft" {
+		if consensusTypeValue.Type == "BFT" {
+			return nil, consensusTypeValue, nil
+		}


In case it is not etcdraft just do return nil, consensusTypeValue, nil if it is not BFT, the chain will panic

orderer/consensus/etcdraft/util_test.go

tock-ibm · 2024-01-14T13:16:46Z

orderer/consensus/etcdraft/chain.go

+			c.logger.Infof("Detected migration to %s", consensusType.Type)
+			return nil
+		} else {
+			c.logger.Panicf("illegal consensus type detected: %s", consensusType.Type)


Yes, May is correct, we prevent it the maintenance filter.

tock-ibm · 2024-01-14T13:17:42Z

orderer/consensus/etcdraft/chain.go

+	c.logger.Infof("Detected configuration change: consensusType is: %s, configMetadata is: %v", consensusType, configMetadata)
+
+	if consensusType == nil {
+		c.logger.Infof("ConsensusType is %v", consensusType)


"ConsensusType is nil"

does this ever happen?

In the absence of this check, one of the chain tests fails.
Anyway, to be safe, it is better to check it

Signed-off-by: May Rosenbaum <mayro1595@gmail.com>

yacovm · 2024-01-20T15:58:18Z

@tock-ibm what is left in your opinion so this can be merged?

tock-ibm · 2024-01-21T14:29:16Z

orderer/common/msgprocessor/maintenancefilter.go

+				return errors.Wrap(err, "failed to unmarshal BFT metadata configuration")
+			}
+
+			_, err := validateBFTMetadataOptions(1, updatedMetadata)


Either

remove the first return value, because it is not used here, and the id can be hard coded, or

move the function consensus/smartbft/configFromMetadataOptions to a package consensus/smartbft/util and make it public. This would break the import cycle. I prefer this option, this way we don't duplicate code.

Signed-off-by: May Rosenbaum <mayro1595@gmail.com>

Raft to BFT migration

2b1cd0e

Signed-off-by: Yacov Manevich <yacov.manevich@ibm.com>

MayRosenbaum requested a review from a team as a code owner November 30, 2023 16:27

MayRosenbaum force-pushed the migration branch 10 times, most recently from 58a7b1b to d27fd90 Compare December 4, 2023 08:33

tock-ibm reviewed Dec 11, 2023

View reviewed changes

orderer/consensus/etcdraft/chain.go Show resolved Hide resolved

orderer/consensus/etcdraft/util.go Show resolved Hide resolved

MayRosenbaum force-pushed the migration branch 2 times, most recently from 06ae680 to e2f1c5e Compare December 11, 2023 13:45

tock-ibm reviewed Dec 11, 2023

View reviewed changes

orderer/common/msgprocessor/maintenancefilter.go Show resolved Hide resolved

MayRosenbaum force-pushed the migration branch 3 times, most recently from 0207578 to f2ad291 Compare December 11, 2023 19:00

MayRosenbaum force-pushed the migration branch 9 times, most recently from fa0db9d to b3d82c2 Compare January 8, 2024 07:49

Raft to BFT migration: functions + tests updates

6a21a90

Signed-off-by: May Rosenbaum <mayro1595@gmail.com>

MayRosenbaum force-pushed the migration branch from b3d82c2 to 6a21a90 Compare January 8, 2024 08:02

yacovm reviewed Jan 9, 2024

View reviewed changes

tock-ibm reviewed Jan 14, 2024

View reviewed changes

MayRosenbaum force-pushed the migration branch 2 times, most recently from 1877849 to 29c35a4 Compare January 18, 2024 10:27

Raft to BFT migration: add and edit test cases

7f404af

Signed-off-by: May Rosenbaum <mayro1595@gmail.com>

MayRosenbaum force-pushed the migration branch from 29c35a4 to 7f404af Compare January 18, 2024 10:32

tock-ibm reviewed Jan 21, 2024

View reviewed changes

MayRosenbaum force-pushed the migration branch 2 times, most recently from 8c9a62f to a78393f Compare January 21, 2024 15:14

break import cycle to prevent code duplication

903f2ea

Signed-off-by: May Rosenbaum <mayro1595@gmail.com>

MayRosenbaum force-pushed the migration branch from a78393f to 903f2ea Compare January 21, 2024 15:19

tock-ibm approved these changes Jan 21, 2024

View reviewed changes

tock-ibm merged commit 7054e88 into hyperledger:main Jan 21, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migration from Raft to BFT test #4561

Migration from Raft to BFT test #4561

MayRosenbaum commented Nov 30, 2023 •

edited by tock-ibm

Loading

tock-ibm Dec 11, 2023

tock-ibm Dec 11, 2023

tock-ibm Dec 11, 2023

yacovm Jan 9, 2024

MayRosenbaum Jan 10, 2024

tock-ibm Jan 14, 2024

tock-ibm Jan 14, 2024

yacovm Jan 9, 2024

yacovm Jan 9, 2024

MayRosenbaum Jan 10, 2024

tock-ibm Jan 14, 2024

tock-ibm Jan 14, 2024

tock-ibm Jan 14, 2024

tock-ibm Jan 14, 2024

tock-ibm Jan 14, 2024

tock-ibm Jan 14, 2024

tock-ibm Jan 14, 2024

MayRosenbaum Jan 17, 2024 •

edited

Loading

yacovm commented Jan 20, 2024

tock-ibm Jan 21, 2024

		c.logger.Panicf("illegal consensus type detected during consensus metadata validation: %s", newOrdererConfig.ConsensusType())
		panic("illegal consensus type detected during consensus metadata validation")

Migration from Raft to BFT test #4561

Migration from Raft to BFT test #4561

Conversation

MayRosenbaum commented Nov 30, 2023 • edited by tock-ibm Loading

Type of change

Related Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MayRosenbaum Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

yacovm commented Jan 20, 2024

Choose a reason for hiding this comment

MayRosenbaum commented Nov 30, 2023 •

edited by tock-ibm

Loading

MayRosenbaum Jan 17, 2024 •

edited

Loading