Fix validator unshuffling (#9088)

* Fix validator being unshuffled by the engine

* Add config option to prevent validator shuffling before a certain block height

* Test array equality using toEqual

Co-authored-by: !shan <ishantiw.quasar@gmail.com>

* Move delayed shuffle validator option to exceptions

* Add an intermediate step when sorting the validator list

* Add test case when validators are expected not to be shuffled

* Adapt consensus unit test

* Update application snapshot

---------

Co-authored-by: !shan <ishantiw.quasar@gmail.com>

framework/src/engine/bft/method.ts

@@ -49,14 +49,16 @@ export interface BlockHeaderAsset {
 export class BFTMethod {
 	private _batchSize!: number;
 	private _blockTime!: number;
+	private _shuffleValidatorsFromHeight!: number;
 
 	public blockTime(): number {
 		return this._blockTime;
 	}
 
-	public init(batchSize: number, blockTime: number) {
+	public init(batchSize: number, blockTime: number, shuffleValidatorsFromHeight: number) {
 		this._batchSize = batchSize;
 		this._blockTime = blockTime;
+		this._shuffleValidatorsFromHeight = shuffleValidatorsFromHeight;
 	}
 
 	public areHeadersContradicting(bftHeader1: BlockHeader, bftHeader2: BlockHeader): boolean {
@@ -173,6 +175,7 @@ export class BFTMethod {
 		precommitThreshold: bigint,
 		certificateThreshold: bigint,
 		validators: Validator[],
+		height: number,
 	): Promise<void> {
 		if (validators.length > this._batchSize) {
 			throw new Error(
@@ -218,13 +221,21 @@ export class BFTMethod {
 			throw new Error('Invalid certificateThreshold input.');
 		}
 
-		sortValidatorsByBLSKey(validators);
-		const validatorsHash = computeValidatorsHash(
-			validators
-				.filter(v => v.bftWeight > BigInt(0))
-				.map(v => ({ bftWeight: v.bftWeight, blsKey: v.blsKey })),
-			certificateThreshold,
-		);
+		// Prepare a separate sorted list of validators for computing validatorsHash
+		// without modifying the existing validators array
+		const validatorsWithBFTWeight = validators
+			.filter(validator => validator.bftWeight > BigInt(0))
+			.map(validator => ({ bftWeight: validator.bftWeight, blsKey: validator.blsKey }));
+		sortValidatorsByBLSKey(validatorsWithBFTWeight);
+
+		const validatorsHash = computeValidatorsHash(validatorsWithBFTWeight, certificateThreshold);
+
+		// Ensure that validator list is not shuffled before the configured block height,
+		// to be able to sync with the new version
+		if (height < this._shuffleValidatorsFromHeight) {
+			sortValidatorsByBLSKey(validators);
+		}
+
 		const bftParams: BFTParameters = {
 			prevoteThreshold: (BigInt(2) * aggregateBFTWeight) / BigInt(3) + BigInt(1),
 			precommitThreshold,

framework/src/engine/bft/module.ts

@@ -38,9 +38,13 @@ export class BFTModule {
 	private _maxLengthBlockBFTInfos!: number;
 
 	// eslint-disable-next-line @typescript-eslint/require-await
-	public async init(batchSize: number, blockTime: number): Promise<void> {
+	public async init(
+		batchSize: number,
+		blockTime: number,
+		shuffleValidatorsFromHeight: number,
+	): Promise<void> {
 		this._batchSize = batchSize;
-		this.method.init(this._batchSize, blockTime);
+		this.method.init(this._batchSize, blockTime, shuffleValidatorsFromHeight);
 		this._maxLengthBlockBFTInfos = 3 * this._batchSize;
 	}
 

framework/src/engine/consensus/consensus.ts

@@ -183,7 +183,11 @@ export class Consensus {
 			blockExecutor,
 			mechanisms: [blockSyncMechanism, fastChainSwitchMechanism],
 		});
-		await this._bft.init(this._genesisConfig.bftBatchSize, this._genesisConfig.blockTime);
+		await this._bft.init(
+			this._genesisConfig.bftBatchSize,
+			this._genesisConfig.blockTime,
+			this._genesisConfig.exceptions.shuffleValidatorsFromHeight,
+		);
 
 		this._network.registerEndpoint(NETWORK_LEGACY_GET_BLOCKS_FROM_ID, async ({ data, peerId }) =>
 			this._legacyEndpoint.handleRPCGetLegacyBlocksFromID(data, peerId),
@@ -1028,6 +1032,7 @@ export class Consensus {
 					afterResult.preCommitThreshold,
 					afterResult.certificateThreshold,
 					afterResult.nextValidators,
+					block.header.height,
 				);
 				this.events.emit(CONSENSUS_EVENT_VALIDATORS_CHANGED, {
 					preCommitThreshold: afterResult.preCommitThreshold,
@@ -1081,6 +1086,7 @@ export class Consensus {
 				result.preCommitThreshold,
 				result.certificateThreshold,
 				result.nextValidators,
+				genesisBlock.header.height,
 			);
 			this.events.emit(CONSENSUS_EVENT_VALIDATORS_CHANGED, {
 				preCommitThreshold: result.preCommitThreshold,

framework/src/engine/generator/generator.ts

@@ -596,6 +596,7 @@ export class Generator {
 				afterResult.preCommitThreshold,
 				afterResult.certificateThreshold,
 				afterResult.nextValidators,
+				height,
 			);
 		}
 

framework/src/schema/application_config_schema.ts

@@ -279,6 +279,17 @@ export const applicationConfigSchema = {
 					minimum: 1,
 					description: 'Minimum block height which can be certified',
 				},
+				exceptions: {
+					type: 'object',
+					required: ['shuffleValidatorsFromHeight'],
+					properties: {
+						shuffleValidatorsFromHeight: {
+							type: 'integer',
+							minimum: 0,
+							description: 'Block height from which the validator list will be shuffled',
+						},
+					},
+				},
 			},
 			additionalProperties: false,
 		},
@@ -351,6 +362,9 @@ export const applicationConfigSchema = {
 			bftBatchSize: BFT_BATCH_SIZE,
 			maxTransactionsSize: MAX_TRANSACTIONS_SIZE,
 			minimumCertifyHeight: 1,
+			exceptions: {
+				shuffleValidatorsFromHeight: 0,
+			},
 		},
 		generator: {
 			keys: {},

framework/src/testing/fixtures/config.ts

@@ -34,6 +34,9 @@ export const defaultConfig: ApplicationConfig = {
 		blockTime: 10,
 		chainID: '10000000',
 		maxTransactionsSize: 15 * 1024, // Kilo Bytes
+		exceptions: {
+			shuffleValidatorsFromHeight: 0,
+		},
 	},
 	network: {
 		version: '1.0',

framework/src/types.ts

@@ -65,6 +65,7 @@ export interface GenesisConfig {
 	blockTime: number;
 	bftBatchSize: number;
 	minimumCertifyHeight: number;
+	exceptions: { shuffleValidatorsFromHeight: number };
 }
 
 export interface TransactionPoolConfig {

framework/test/unit/__snapshots__/application.spec.ts.snap

@@ -12,6 +12,9 @@ exports[`Application #constructor should set internal variables 1`] = `
     },
     "blockTime": 10,
     "chainID": "10000000",
+    "exceptions": {
+      "shuffleValidatorsFromHeight": 0,
+    },
     "maxTransactionsSize": 15360,
     "minimumCertifyHeight": 1,
   },

framework/test/unit/engine/bft/bft_processing.spec.ts

@@ -39,6 +39,7 @@ describe('BFT processing', () => {
 		scenario11ValidatorsPartialSwitch,
 	];
 	const blockTime = 10;
+	const shuffleValidatorsFromHeight = 0;
 
 	for (const scenario of bftScenarios) {
 		// eslint-disable-next-line no-loop-func
@@ -49,7 +50,11 @@ describe('BFT processing', () => {
 
 			beforeAll(async () => {
 				bftModule = new BFTModule();
-				await bftModule.init(scenario.config.activeValidators, blockTime);
+				await bftModule.init(
+					scenario.config.activeValidators,
+					blockTime,
+					shuffleValidatorsFromHeight,
+				);
 				db = new InMemoryDatabase();
 				stateStore = new StateStore(db);