Fix outdated getMaxRemovalHeight (#9065)

* 🐛 Fix outdated getMaxRemovalHeight * ♻️ Fix test description
LiskArchive · Oct 6, 2023 · da598fb · da598fb
1 parent 196c9ec
commit da598fb
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 62 deletions.
diff --git a/framework/src/engine/consensus/certificate_generation/commit_pool.ts b/framework/src/engine/consensus/certificate_generation/commit_pool.ts
@@ -104,7 +104,7 @@ export class CommitPool {
 		}
 
 		// Validation Step 2
-		const maxRemovalHeight = await this._getMaxRemovalHeight();
+		const maxRemovalHeight = await this.getMaxRemovalHeight();
 		if (commit.height <= maxRemovalHeight) {
 			return false;
 		}
@@ -307,6 +307,13 @@ export class CommitPool {
 		};
 	}
 
+	public async getMaxRemovalHeight() {
+		const blockHeader = await this._chain.dataAccess.getBlockHeaderByHeight(
+			this._chain.finalizedHeight,
+		);
+		return Math.max(blockHeader.aggregateCommit.height, this._minCertifyHeight - 1);
+	}
+
 	private async _selectAggregateCommit(methodContext: StateStore): Promise<AggregateCommit> {
 		const { maxHeightCertified, maxHeightPrecommitted } = await this._bftMethod.getBFTHeights(
 			methodContext,
@@ -368,7 +375,7 @@ export class CommitPool {
 	}
 
 	private async _job(methodContext: StateStore): Promise<void> {
-		const removalHeight = await this._getMaxRemovalHeight();
+		const removalHeight = await this.getMaxRemovalHeight();
 		const currentHeight = this._chain.lastBlock.header.height;
 		const { maxHeightPrecommitted } = await this._bftMethod.getBFTHeights(methodContext);
 
@@ -515,13 +522,6 @@ export class CommitPool {
 		return deleteHeights;
 	}
 
-	private async _getMaxRemovalHeight() {
-		const blockHeader = await this._chain.dataAccess.getBlockHeaderByHeight(
-			this._chain.finalizedHeight,
-		);
-		return blockHeader.aggregateCommit.height;
-	}
-
 	private _getAllCommits(): SingleCommit[] {
 		// Flattened list of all the single commits from both gossiped and non gossiped list sorted by ascending order of height
 		return [

diff --git a/framework/src/engine/consensus/consensus.ts b/framework/src/engine/consensus/consensus.ts
@@ -396,13 +396,7 @@ export class Consensus {
 	}
 
 	public async getMaxRemovalHeight(): Promise<number> {
-		const finalizedBlockHeader = await this._chain.dataAccess.getBlockHeaderByHeight(
-			this._chain.finalizedHeight,
-		);
-		return Math.max(
-			finalizedBlockHeader.aggregateCommit.height,
-			this._genesisConfig.minimumCertifyHeight - 1,
-		);
+		return this._commitPool.getMaxRemovalHeight();
 	}
 
 	private async _execute(block: Block, peerID: string): Promise<void> {

diff --git a/framework/test/unit/engine/consensus/certificate_generation/commit_pool.spec.ts b/framework/test/unit/engine/consensus/certificate_generation/commit_pool.spec.ts
@@ -1062,7 +1062,7 @@ describe('CommitPool', () => {
 		it.todo('');
 	});
 
-	describe('_getMaxRemovalHeight', () => {
+	describe('getMaxRemovalHeight', () => {
 		let blockHeader: BlockHeader;
 		const finalizedHeight = 1010;
 
@@ -1085,14 +1085,54 @@ describe('CommitPool', () => {
 			when(getBlockHeaderByHeight).calledWith(finalizedHeight).mockReturnValue(blockHeader);
 		});
 		it('should return successfully for an existing block header at finalizedHeight', async () => {
-			const maxRemovalHeight = await commitPool['_getMaxRemovalHeight']();
+			const maxRemovalHeight = await commitPool.getMaxRemovalHeight();
 
 			expect(maxRemovalHeight).toBe(blockHeader.aggregateCommit.height);
 		});
 		it('should throw an error for non-existent block header at finalizedHeight', async () => {
 			chain.finalizedHeight = finalizedHeight + 1;
 
-			await expect(commitPool['_getMaxRemovalHeight']()).rejects.toThrow(NotFoundError);
+			await expect(commitPool.getMaxRemovalHeight()).rejects.toThrow(NotFoundError);
+		});
+
+		it('should return minCertifyHeight -1 if the finalizedBlock.aggregateCommit.height is smaller', async () => {
+			const finalizedBlockHeader = createFakeBlockHeader({
+				height: 25520,
+				timestamp: finalizedHeight * 10,
+				aggregateCommit: {
+					aggregationBits: Buffer.alloc(0),
+					certificateSignature: Buffer.alloc(0),
+					height: 500,
+				},
+			});
+			when(getBlockHeaderByHeight)
+				.calledWith(finalizedHeight)
+				.mockReturnValue(finalizedBlockHeader);
+			const minimumCertifyHeight = 25519;
+			(commitPool as any)['_minCertifyHeight'] = minimumCertifyHeight;
+
+			await expect(commitPool.getMaxRemovalHeight()).resolves.toEqual(minimumCertifyHeight - 1);
+		});
+
+		it('should return finalizedBlock.aggregateCommit.height if the minCertifyHeight - 1 is smaller', async () => {
+			const finalizedBlockHeader = createFakeBlockHeader({
+				height: 25520,
+				timestamp: finalizedHeight * 10,
+				aggregateCommit: {
+					aggregationBits: Buffer.alloc(0),
+					certificateSignature: Buffer.alloc(0),
+					height: 25520,
+				},
+			});
+			when(getBlockHeaderByHeight)
+				.calledWith(finalizedHeight)
+				.mockReturnValue(finalizedBlockHeader);
+			const minimumCertifyHeight = 25519;
+			(commitPool as any)['_minCertifyHeight'] = minimumCertifyHeight;
+
+			await expect(commitPool.getMaxRemovalHeight()).resolves.toEqual(
+				finalizedBlockHeader.aggregateCommit.height,
+			);
 		});
 	});
 

diff --git a/framework/test/unit/engine/consensus/consensus.spec.ts b/framework/test/unit/engine/consensus/consensus.spec.ts
@@ -222,49 +222,6 @@ describe('consensus', () => {
 		});
 	});
 
-	describe('getMaxRemovalHeight', () => {
-		it('should return minCertifyHeight if the finalizedBlock.aggregateCommit.height is smaller', async () => {
-			const finalizedBlock = await createValidDefaultBlock({
-				header: {
-					height: 1,
-					aggregateCommit: {
-						height: 0,
-						aggregationBits: Buffer.alloc(0),
-						certificateSignature: Buffer.alloc(0),
-					},
-				},
-			});
-			(chain.dataAccess.getBlockHeaderByHeight as jest.Mock).mockResolvedValue(
-				finalizedBlock.header,
-			);
-			const minimumCertifyHeight = 25519;
-			consensus['_genesisConfig'].minimumCertifyHeight = minimumCertifyHeight;
-
-			await expect(consensus.getMaxRemovalHeight()).resolves.toEqual(minimumCertifyHeight - 1);
-		});
-
-		it('should return finalizedBlock.aggregateCommit.height if the minCertifyHeight is smaller', async () => {
-			const finalizedBlock = await createValidDefaultBlock({
-				header: {
-					height: 1,
-					aggregateCommit: {
-						height: 25520,
-						aggregationBits: Buffer.alloc(0),
-						certificateSignature: Buffer.alloc(0),
-					},
-				},
-			});
-			(chain.dataAccess.getBlockHeaderByHeight as jest.Mock).mockResolvedValue(
-				finalizedBlock.header,
-			);
-			consensus['_genesisConfig'].minimumCertifyHeight = 25519;
-
-			await expect(consensus.getMaxRemovalHeight()).resolves.toEqual(
-				finalizedBlock.header.aggregateCommit.height,
-			);
-		});
-	});
-
 	describe('certifySingleCommit', () => {
 		const passphrase = Mnemonic.generateMnemonic(256);
 		const { publicKey } = legacy.getPrivateAndPublicKeyFromPassphrase(passphrase);