diff --git a/framework/src/engine/consensus/certificate_generation/commit_pool.ts b/framework/src/engine/consensus/certificate_generation/commit_pool.ts index 434c8655b2..6ad181c0f0 100644 --- 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 { const { maxHeightCertified, maxHeightPrecommitted } = await this._bftMethod.getBFTHeights( methodContext, @@ -368,7 +375,7 @@ export class CommitPool { } private async _job(methodContext: StateStore): Promise { - 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 index 5a06351b24..6030b233ed 100644 --- 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 { - 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 { 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 index 2b6d26f41e..130c712ea0 100644 --- 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 index 03666f0311..19adf9d4d1 100644 --- 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);