_verifyTerminatedStateAccountsCommon doesn't need to be inside loop

framework/src/modules/interoperability/base_interoperability_module.ts

@@ -35,12 +35,7 @@ import { OwnChainAccountStore } from './stores/own_chain_account';
 import { RegisteredNamesStore } from './stores/registered_names';
 import { TerminatedOutboxStore } from './stores/terminated_outbox';
 import { TerminatedStateStore } from './stores/terminated_state';
-import {
-	ChainInfo,
-	GenesisInteroperability,
-	OwnChainAccount,
-	TerminatedStateAccountWithChainID,
-} from './types';
+import { ChainInfo, GenesisInteroperability, OwnChainAccount } from './types';
 import { computeValidatorsHash, getTokenIDLSK } from './utils';
 import { genesisInteroperabilitySchema } from './schemas';
 import { CcmProcessedEvent } from './events/ccm_processed';
@@ -133,21 +128,18 @@ export abstract class BaseInteroperabilityModule extends BaseInteroperableModule
 		}
 
 		// activeValidators must be ordered lexicographically by blsKey property
-		const sortedByBlsKeys = [...activeValidators].sort((a, b) => a.blsKey.compare(b.blsKey));
-		for (let i = 0; i < activeValidators.length; i += 1) {
-			if (!activeValidators[i].blsKey.equals(sortedByBlsKeys[i].blsKey)) {
-				throw new Error('activeValidators must be ordered lexicographically by blsKey property.');
-			}
+		const blsKeys = activeValidators.map(v => v.blsKey);
+		if (!objectUtils.isBufferArrayOrdered(blsKeys)) {
+			throw new Error('activeValidators must be ordered lexicographically by blsKey property.');
 		}
 
 		// all blsKey properties must be pairwise distinct
-		const blsKeys = activeValidators.map(v => v.blsKey);
 		if (!objectUtils.bufferArrayUniqueItems(blsKeys)) {
 			throw new Error(`All blsKey properties must be pairwise distinct.`);
 		}
 
 		// for each validator in activeValidators, validator.bftWeight > 0 must hold
-		if (activeValidators.filter(v => v.bftWeight <= 0).length > 0) {
+		if (activeValidators.filter(v => v.bftWeight <= BigInt(0)).length > 0) {
 			throw new Error(`validator.bftWeight must be > 0.`);
 		}
 
@@ -189,28 +181,15 @@ export abstract class BaseInteroperabilityModule extends BaseInteroperableModule
 		}
 	}
 
-	protected _verifyTerminatedStateAccountsCommon(
-		terminatedStateAccounts: TerminatedStateAccountWithChainID[],
-		mainchainID: Buffer,
-	) {
+	protected _verifyTerminatedStateAccountsIDs(chainIDs: Buffer[]) {
 		// Each entry stateAccount in terminatedStateAccounts has a unique stateAccount.chainID
-		const chainIDs = terminatedStateAccounts.map(a => a.chainID);
 		if (!objectUtils.bufferArrayUniqueItems(chainIDs)) {
 			throw new Error(`terminatedStateAccounts don't hold unique chainID.`);
 		}
 
 		// terminatedStateAccounts is ordered lexicographically by stateAccount.chainID
-		const sortedByChainID = [...terminatedStateAccounts].sort((a, b) =>
-			a.chainID.compare(b.chainID),
-		);
-
-		for (let i = 0; i < terminatedStateAccounts.length; i += 1) {
-			const stateAccountWithChainID = terminatedStateAccounts[i];
-			if (!stateAccountWithChainID.chainID.equals(sortedByChainID[i].chainID)) {
-				throw new Error('terminatedStateAccounts must be ordered lexicographically by chainID.');
-			}
-
-			this._verifyChainID(stateAccountWithChainID.chainID, mainchainID, 'stateAccount.');
+		if (!objectUtils.isBufferArrayOrdered(chainIDs)) {
+			throw new Error('terminatedStateAccounts must be ordered lexicographically by chainID.');
 		}
 	}
 

framework/src/modules/interoperability/mainchain/module.ts

@@ -265,11 +265,11 @@ export class MainchainInteroperabilityModule extends BaseInteroperabilityModule
 		// If chainInfos is non-empty, ownChainNonce > 0
 		if (chainInfos.length === 0 && ownChainNonce !== BigInt(0)) {
 			throw new Error(`ownChainNonce must be 0 if chainInfos is empty.`);
-		} else if (chainInfos.length !== 0 && ownChainNonce <= 0) {
+		} else if (chainInfos.length !== 0 && ownChainNonce <= BigInt(0)) {
 			throw new Error(`ownChainNonce must be positive if chainInfos is not empty.`);
 		}
 
-		this._verifyChainInfos(ctx, chainInfos);
+		this._verifyChainInfos(ctx, chainInfos, terminatedStateAccounts);
 		this._verifyTerminatedStateAccounts(chainInfos, terminatedStateAccounts, mainchainID);
 		this._verifyTerminatedOutboxAccounts(
 			chainInfos,
@@ -281,19 +281,20 @@ export class MainchainInteroperabilityModule extends BaseInteroperabilityModule
 	}
 
 	// https://github.com/LiskHQ/lips/blob/main/proposals/lip-0045.md#mainchain
-	private _verifyChainInfos(ctx: GenesisBlockExecuteContext, chainInfos: ChainInfo[]) {
+	private _verifyChainInfos(
+		ctx: GenesisBlockExecuteContext,
+		chainInfos: ChainInfo[],
+		terminatedStateAccounts: TerminatedStateAccountWithChainID[],
+	) {
 		// Each entry chainInfo in chainInfos has a unique chainInfo.chainID
 		const chainIDs = chainInfos.map(info => info.chainID);
 		if (!objectUtils.bufferArrayUniqueItems(chainIDs)) {
 			throw new Error(`chainInfos doesn't hold unique chainID.`);
 		}
 
 		// chainInfos should be ordered lexicographically by chainInfo.chainID
-		const sortedByChainID = [...chainInfos].sort((a, b) => a.chainID.compare(b.chainID));
-		for (let i = 0; i < chainInfos.length; i += 1) {
-			if (!chainInfos[i].chainID.equals(sortedByChainID[i].chainID)) {
-				throw new Error('chainInfos is not ordered lexicographically by chainID.');
-			}
+		if (!objectUtils.isBufferArrayOrdered(chainIDs)) {
+			throw new Error('chainInfos is not ordered lexicographically by chainID.');
 		}
 
 		// The entries chainData.name must be pairwise distinct
@@ -307,13 +308,17 @@ export class MainchainInteroperabilityModule extends BaseInteroperabilityModule
 		// verify root level properties
 		for (const chainInfo of chainInfos) {
 			this._verifyChainID(chainInfo.chainID, mainchainID, 'chainInfo.');
-			this._verifyChainData(ctx, chainInfo);
+			this._verifyChainData(ctx, chainInfo, terminatedStateAccounts);
 			this._verifyChannelData(ctx, chainInfo);
 			this._verifyChainValidators(chainInfo);
 		}
 	}
 
-	private _verifyChainData(ctx: GenesisBlockExecuteContext, chainInfo: ChainInfo) {
+	private _verifyChainData(
+		ctx: GenesisBlockExecuteContext,
+		chainInfo: ChainInfo,
+		terminatedStateAccounts: TerminatedStateAccountWithChainID[],
+	) {
 		const validStatuses = [ChainStatus.REGISTERED, ChainStatus.ACTIVE, ChainStatus.TERMINATED];
 
 		const { chainData } = chainInfo;
@@ -332,6 +337,17 @@ export class MainchainInteroperabilityModule extends BaseInteroperabilityModule
 		if (!validStatuses.includes(chainData.status)) {
 			throw new Error(`chainData.status must be one of ${validStatuses.join(', ')}`);
 		}
+
+		if (chainData.status === ChainStatus.TERMINATED) {
+			const accountWithChainID = terminatedStateAccounts.find(accountWithChainIDTemp =>
+				accountWithChainIDTemp.chainID.equals(chainInfo.chainID),
+			);
+			if (!accountWithChainID) {
+				throw new Error(
+					'For each chainInfo with status terminated there should be a corresponding entry in terminatedStateAccounts.',
+				);
+			}
+		}
 	}
 
 	// https://github.com/LiskHQ/lips/blob/main/proposals/lip-0045.md#mainchain
@@ -340,10 +356,14 @@ export class MainchainInteroperabilityModule extends BaseInteroperabilityModule
 		terminatedStateAccounts: TerminatedStateAccountWithChainID[],
 		mainchainID: Buffer,
 	) {
+		this._verifyTerminatedStateAccountsIDs(terminatedStateAccounts.map(a => a.chainID));
+
 		// Sanity check to fulfill if-and-only-if situation
-		for (const account of terminatedStateAccounts) {
+		for (const terminatedStateAccountWithChainID of terminatedStateAccounts) {
+			this._verifyChainID(terminatedStateAccountWithChainID.chainID, mainchainID, 'stateAccount.');
+
 			const correspondingChainInfo = chainInfos.find(chainInfo =>
-				chainInfo.chainID.equals(account.chainID),
+				chainInfo.chainID.equals(terminatedStateAccountWithChainID.chainID),
 			);
 			if (
 				!correspondingChainInfo ||
@@ -353,46 +373,29 @@ export class MainchainInteroperabilityModule extends BaseInteroperabilityModule
 					'For each terminatedStateAccount there should be a corresponding chainInfo at TERMINATED state.',
 				);
 			}
-		}
 
-		for (const chainInfo of chainInfos) {
-			// For each entry chainInfo in chainInfos, chainInfo.chainData.status == CHAIN_STATUS_TERMINATED
-			// if and only if a corresponding entry (i.e., with chainID == chainInfo.chainID) exists in terminatedStateAccounts.
-			if (chainInfo.chainData.status === ChainStatus.TERMINATED) {
-				const terminatedAccount = terminatedStateAccounts.find(tAccount =>
-					tAccount.chainID.equals(chainInfo.chainID),
+			const stateAccount = terminatedStateAccountWithChainID.terminatedStateAccount;
+			// For each entry stateAccount in terminatedStateAccounts holds
+			// stateAccount.stateRoot == chainData.lastCertificate.stateRoot,
+			// stateAccount.mainchainStateRoot == EMPTY_HASH, and
+			// stateAccount.initialized == True.
+			// Here chainData is the corresponding entry (i.e., with chainID == stateAccount.chainID) in chainInfos.
+			if (
+				!stateAccount.stateRoot.equals(correspondingChainInfo.chainData.lastCertificate.stateRoot)
+			) {
+				throw new Error(
+					"stateAccount.stateRoot doesn't match chainInfo.chainData.lastCertificate.stateRoot.",
+				);
+			}
+
+			if (!stateAccount.mainchainStateRoot.equals(EMPTY_HASH)) {
+				throw new Error(
+					`stateAccount.mainchainStateRoot is not equal to ${EMPTY_HASH.toString('hex')}.`,
 				);
-				if (!terminatedAccount) {
-					throw new Error(
-						'For each chainInfo with status terminated there should be a corresponding entry in terminatedStateAccounts.',
-					);
-				}
-
-				this._verifyTerminatedStateAccountsCommon(terminatedStateAccounts, mainchainID);
-
-				// For each entry stateAccount in terminatedStateAccounts holds
-				// stateAccount.stateRoot == chainData.lastCertificate.stateRoot,
-				// stateAccount.mainchainStateRoot == EMPTY_HASH, and
-				// stateAccount.initialized == True.
-				// Here chainData is the corresponding entry (i.e., with chainID == stateAccount.chainID) in chainInfos.
-				const stateAccount = terminatedAccount.terminatedStateAccount;
-				if (stateAccount) {
-					if (!stateAccount.stateRoot.equals(chainInfo.chainData.lastCertificate.stateRoot)) {
-						throw new Error(
-							"stateAccount.stateRoot doesn't match chainInfo.chainData.lastCertificate.stateRoot.",
-						);
-					}
-
-					if (!stateAccount.mainchainStateRoot.equals(EMPTY_HASH)) {
-						throw new Error(
-							`stateAccount.mainchainStateRoot is not equal to ${EMPTY_HASH.toString('hex')}.`,
-						);
-					}
-
-					if (!stateAccount.initialized) {
-						throw new Error('stateAccount is not initialized.');
-					}
-				}
+			}
+
+			if (!stateAccount.initialized) {
+				throw new Error('stateAccount is not initialized.');
 			}
 		}
 	}
@@ -410,13 +413,8 @@ export class MainchainInteroperabilityModule extends BaseInteroperabilityModule
 		}
 
 		// terminatedOutboxAccounts is ordered lexicographically by outboxAccount.chainID
-		const sortedByChainID = [...terminatedOutboxAccounts].sort((a, b) =>
-			a.chainID.compare(b.chainID),
-		);
-		for (let i = 0; i < terminatedOutboxAccounts.length; i += 1) {
-			if (!terminatedOutboxAccounts[i].chainID.equals(sortedByChainID[i].chainID)) {
-				throw new Error('terminatedOutboxAccounts must be ordered lexicographically by chainID.');
-			}
+		if (!objectUtils.isBufferArrayOrdered(chainIDs)) {
+			throw new Error('terminatedOutboxAccounts must be ordered lexicographically by chainID.');
 		}
 
 		// Furthermore, an entry outboxAccount in terminatedOutboxAccounts must have a corresponding entry

framework/src/modules/interoperability/sidechain/module.ts

@@ -290,7 +290,7 @@ export class SidechainInteroperabilityModule extends BaseInteroperabilityModule
 			}
 			// mainchainInfo.chainID == getMainchainID();
 			const mainchainInfo = chainInfos[0];
-			const mainchainID = getMainchainID(mainchainInfo.chainID);
+			const mainchainID = getMainchainID(ctx.chainID);
 			if (!mainchainInfo.chainID.equals(mainchainID)) {
 				throw new Error(`mainchainInfo.chainID must be equal to ${mainchainID.toString('hex')}.`);
 			}
@@ -320,9 +320,11 @@ export class SidechainInteroperabilityModule extends BaseInteroperabilityModule
 		terminatedStateAccounts: TerminatedStateAccountWithChainID[],
 		mainchainID: Buffer,
 	) {
-		this._verifyTerminatedStateAccountsCommon(terminatedStateAccounts, mainchainID);
+		this._verifyTerminatedStateAccountsIDs(terminatedStateAccounts.map(a => a.chainID));
 
 		for (const stateAccount of terminatedStateAccounts) {
+			this._verifyChainID(stateAccount.chainID, mainchainID, 'stateAccount.');
+
 			// and stateAccount.chainID != OWN_CHAIN_ID.
 			if (stateAccount.chainID.equals(ctx.chainID)) {
 				throw new Error(`stateAccount.chainID must not be equal to OWN_CHAIN_ID.`);

framework/test/unit/modules/interoperability/base_interoperability_module.spec.ts

@@ -445,7 +445,7 @@ must NOT have more than ${MAX_NUM_VALIDATORS} items`,
 		});
 	});
 
-	describe('_verifyTerminatedStateAccountsCommon', () => {
+	describe('_verifyTerminatedStateAccountsIDs', () => {
 		certificateThreshold = BigInt(10);
 		const validChainInfos = [
 			{

framework/test/unit/modules/interoperability/mainchain/module.spec.ts

@@ -300,6 +300,38 @@ describe('initGenesisState', () => {
 					].join(', ')}`,
 				);
 			});
+
+			it('should throw if chainInfo.chainData.status === TERMINATED exists but no terminateStateAccount', async () => {
+				const context = createInitGenesisStateContext(
+					{
+						...genesisInteroperability,
+						chainInfos: [
+							{
+								...chainInfo,
+								chainData: {
+									...chainData,
+									status: ChainStatus.TERMINATED,
+									lastCertificate: {
+										...lastCertificate,
+										validatorsHash: computeValidatorsHash(activeValidators, certificateThreshold),
+									},
+								},
+								chainValidators: {
+									activeValidators,
+									certificateThreshold,
+								},
+							},
+						],
+						// No terminatedStateAccount
+						terminatedStateAccounts: [],
+					},
+					params,
+				);
+
+				await expect(interopMod.initGenesisState(context)).rejects.toThrow(
+					`For each chainInfo with status terminated there should be a corresponding entry in terminatedStateAccounts.`,
+				);
+			});
 		});
 
 		describe('terminatedStateAccounts', () => {
@@ -332,7 +364,9 @@ describe('initGenesisState', () => {
 				await expect(interopMod.initGenesisState(context)).resolves.not.toThrow();
 			});
 
-			it('should throw if chainInfo.chainData.status===TERMINATED exists but no terminateStateAccount', async () => {
+			it('should call _verifyTerminatedStateAccountsIDs', async () => {
+				jest.spyOn(interopMod, '_verifyTerminatedStateAccountsIDs' as any);
+
 				const context = createInitGenesisStateContext(
 					{
 						...genesisInteroperability,
@@ -353,18 +387,21 @@ describe('initGenesisState', () => {
 								},
 							},
 						],
-						// No terminatedStateAccount
-						terminatedStateAccounts: [],
+						terminatedStateAccounts: [
+							{
+								chainID: chainInfo.chainID,
+								terminatedStateAccount,
+							},
+						],
 					},
 					params,
 				);
 
-				await expect(interopMod.initGenesisState(context)).rejects.toThrow(
-					`For each chainInfo with status terminated there should be a corresponding entry in terminatedStateAccounts.`,
-				);
+				await expect(interopMod.initGenesisState(context)).resolves.toBeUndefined();
+				expect(interopMod['_verifyTerminatedStateAccountsIDs']).toHaveBeenCalledTimes(1);
 			});
 
-			it('should throw if there is an entry in terminateStateAccounts for a chainID that is ACTIVE in chainInfos', async () => {
+			it('should throw error if chainInfo.chainID exists in terminatedStateAccounts & chainInfo.chainData.status is ACTIVE', async () => {
 				const context = createInitGenesisStateContext(
 					{
 						...genesisInteroperability,
@@ -400,7 +437,7 @@ describe('initGenesisState', () => {
 				);
 			});
 
-			it('should throw error if chainInfo.chainID exists in terminatedStateAccounts & chainInfo.chainData.status !== CHAIN_STATUS_TERMINATED', async () => {
+			it('should throw error if chainInfo.chainID exists in terminatedStateAccounts & chainInfo.chainData.status is REGISTERED', async () => {
 				const context = createInitGenesisStateContext(
 					{
 						...genesisInteroperability,
@@ -450,7 +487,6 @@ describe('initGenesisState', () => {
 										...lastCertificate,
 										validatorsHash: computeValidatorsHash(activeValidators, certificateThreshold),
 									},
-									status: ChainStatus.TERMINATED,
 								},
 								chainValidators: {
 									activeValidators,