From f36dad589777ee84dd4636710bc30f97f3f9bb8b Mon Sep 17 00:00:00 2001 From: shuse2 Date: Fri, 27 Jan 2023 20:55:36 +0100 Subject: [PATCH 1/4] :bug: Fix range of single commit generation in init --- framework/src/engine/consensus/consensus.ts | 7 ----- framework/src/engine/generator/generator.ts | 28 ++++++------------- framework/src/engine/generator/types.ts | 1 - framework/test/unit/engine/engine.spec.ts | 1 - .../unit/engine/generator/generator.spec.ts | 10 +++---- 5 files changed, 12 insertions(+), 35 deletions(-) diff --git a/framework/src/engine/consensus/consensus.ts b/framework/src/engine/consensus/consensus.ts index b4fcab9e4bf..e832e57ab09 100644 --- a/framework/src/engine/consensus/consensus.ts +++ b/framework/src/engine/consensus/consensus.ts @@ -366,13 +366,6 @@ export class Consensus { this._commitPool.addCommit(singleCommit, true); } - public async getMaxRemovalHeight(): Promise { - const finalizedBlockHeader = await this._chain.dataAccess.getBlockHeaderByHeight( - this._chain.finalizedHeight, - ); - return finalizedBlockHeader.aggregateCommit.height; - } - public isSynced(height: number, maxHeightPrevoted: number): boolean { const lastBlockHeader = this._chain.lastBlock.header; if (lastBlockHeader.version === 0) { diff --git a/framework/src/engine/generator/generator.ts b/framework/src/engine/generator/generator.ts index 6a8c9c21970..8c832304ea2 100644 --- a/framework/src/engine/generator/generator.ts +++ b/framework/src/engine/generator/generator.ts @@ -199,9 +199,12 @@ export class Generator { }); const stateStore = new StateStore(this._blockchainDB); - const maxRemovalHeight = await this._consensus.getMaxRemovalHeight(); - const { maxHeightPrecommitted } = await this._bft.method.getBFTHeights(stateStore); - await Promise.all(this._handleFinalizedHeightChanged(maxRemovalHeight, maxHeightPrecommitted)); + const { maxHeightPrecommitted, maxHeightCertified } = await this._bft.method.getBFTHeights( + stateStore, + ); + await Promise.all( + this._handleFinalizedHeightChanged(maxHeightCertified + 1, maxHeightPrecommitted), + ); } public get endpoint(): Endpoint { @@ -625,7 +628,7 @@ export class Generator { const promises = []; const stateStore = new StateStore(this._blockchainDB); for (const [address, pairs] of this._keypairs.entries()) { - for (let height = from + 1; height <= to; height += 1) { + for (let height = from + 1; height < to; height += 1) { promises.push( this._certifySingleCommitForChangedHeight( stateStore, @@ -635,9 +638,7 @@ export class Generator { ), ); } - promises.push( - this._certifySingleCommitForFinalizedHeight(stateStore, to, address, pairs.blsSecretKey), - ); + promises.push(this._certifySingleCommit(stateStore, to, address, pairs.blsSecretKey)); } return promises; } @@ -655,19 +656,6 @@ export class Generator { await this._certifySingleCommit(stateStore, height, generatorAddress, blsSK); } - private async _certifySingleCommitForFinalizedHeight( - stateStore: StateStore, - height: number, - generatorAddress: Buffer, - blsSK: Buffer, - ): Promise { - const paramExist = await this._bft.method.existBFTParameters(stateStore, height + 1); - if (paramExist) { - return; - } - await this._certifySingleCommit(stateStore, height, generatorAddress, blsSK); - } - private async _certifySingleCommit( stateStore: StateStore, height: number, diff --git a/framework/src/engine/generator/types.ts b/framework/src/engine/generator/types.ts index 4b0d28471ce..9d64d08b37d 100644 --- a/framework/src/engine/generator/types.ts +++ b/framework/src/engine/generator/types.ts @@ -42,7 +42,6 @@ export interface Consensus { finalizedHeight: () => number; getAggregateCommit: (stateStore: StateStore) => Promise; certifySingleCommit: (blockHeader: BlockHeader, validatorInfo: ValidatorInfo) => void; - getMaxRemovalHeight: () => Promise; readonly events: EventEmitter; } diff --git a/framework/test/unit/engine/engine.spec.ts b/framework/test/unit/engine/engine.spec.ts index e57ae6f793e..f50c0708f17 100644 --- a/framework/test/unit/engine/engine.spec.ts +++ b/framework/test/unit/engine/engine.spec.ts @@ -53,7 +53,6 @@ describe('engine', () => { jest .spyOn(Chain.prototype, 'lastBlock', 'get') .mockReturnValue({ header: { height: 300 } } as never); - jest.spyOn(Consensus.prototype, 'getMaxRemovalHeight').mockResolvedValue(0); jest .spyOn(BFTMethod.prototype, 'getBFTHeights') .mockResolvedValue({ maxHeightPrecommitted: 0 } as never); diff --git a/framework/test/unit/engine/generator/generator.spec.ts b/framework/test/unit/engine/generator/generator.spec.ts index 4acfad26404..7f3316080f6 100644 --- a/framework/test/unit/engine/generator/generator.spec.ts +++ b/framework/test/unit/engine/generator/generator.spec.ts @@ -88,10 +88,8 @@ describe('generator', () => { consensusEvent = new EventEmitter(); consensus = { execute: jest.fn(), - getAggregateCommit: jest.fn(), certifySingleCommit: jest.fn(), - getMaxRemovalHeight: jest.fn().mockResolvedValue(0), getConsensusParams: jest.fn().mockResolvedValue({ currentValidators: [], implyMaxPrevote: true, @@ -199,21 +197,21 @@ describe('generator', () => { expect(generator['_keypairs'].values()).toHaveLength(103); }); - it('should handle finalized height change between max removal height and max height precommitted', async () => { + it('should handle finalized height change between maxHeightCertified + 1 and max height precommitted', async () => { jest.spyOn(generator, '_handleFinalizedHeightChanged' as any).mockReturnValue([] as never); - jest.spyOn(generator['_consensus'], 'getMaxRemovalHeight').mockResolvedValue(313); jest .spyOn(generator['_bft'].method, 'getBFTHeights') - .mockResolvedValue({ maxHeightPrecommitted: 515 } as never); + .mockResolvedValue({ maxHeightPrecommitted: 515, maxHeightCertified: 313 } as never); await generator.init({ blockchainDB, generatorDB, logger, }); - expect(generator['_handleFinalizedHeightChanged']).toHaveBeenCalledWith(313, 515); + expect(generator['_handleFinalizedHeightChanged']).toHaveBeenCalledWith(314, 515); }); }); + describe('saveKeysFromFile', () => { it('should not store any data when generator.keys.fromFile is not defined', async () => { generator['_config'].generator = { keys: {} }; From 0f003d0552d06f3e6d7bbfff58391233f3978707 Mon Sep 17 00:00:00 2001 From: shuse2 Date: Sat, 28 Jan 2023 13:15:15 +0100 Subject: [PATCH 2/4] :recycle: Add check for input range --- framework/src/engine/generator/generator.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/framework/src/engine/generator/generator.ts b/framework/src/engine/generator/generator.ts index 8c832304ea2..9f3a00ae3f8 100644 --- a/framework/src/engine/generator/generator.ts +++ b/framework/src/engine/generator/generator.ts @@ -625,6 +625,9 @@ export class Generator { } private _handleFinalizedHeightChanged(from: number, to: number): Promise[] { + if (from >= to) { + return []; + } const promises = []; const stateStore = new StateStore(this._blockchainDB); for (const [address, pairs] of this._keypairs.entries()) { From e1ce2650c28431b494f95c0f650ecec59dbd5a0c Mon Sep 17 00:00:00 2001 From: shuse2 Date: Mon, 30 Jan 2023 22:00:01 +0100 Subject: [PATCH 3/4] :bug: Fix the range in itit --- framework/src/engine/generator/generator.ts | 2 +- framework/test/unit/engine/generator/generator.spec.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/framework/src/engine/generator/generator.ts b/framework/src/engine/generator/generator.ts index 9f3a00ae3f8..a87a0b33115 100644 --- a/framework/src/engine/generator/generator.ts +++ b/framework/src/engine/generator/generator.ts @@ -203,7 +203,7 @@ export class Generator { stateStore, ); await Promise.all( - this._handleFinalizedHeightChanged(maxHeightCertified + 1, maxHeightPrecommitted), + this._handleFinalizedHeightChanged(maxHeightCertified, maxHeightPrecommitted), ); } diff --git a/framework/test/unit/engine/generator/generator.spec.ts b/framework/test/unit/engine/generator/generator.spec.ts index 7f3316080f6..ffe08744bbb 100644 --- a/framework/test/unit/engine/generator/generator.spec.ts +++ b/framework/test/unit/engine/generator/generator.spec.ts @@ -208,7 +208,7 @@ describe('generator', () => { generatorDB, logger, }); - expect(generator['_handleFinalizedHeightChanged']).toHaveBeenCalledWith(314, 515); + expect(generator['_handleFinalizedHeightChanged']).toHaveBeenCalledWith(313, 515); }); }); From 81d55f2065f6f65891e0ee393b26107d88d08b63 Mon Sep 17 00:00:00 2001 From: shuse2 Date: Tue, 31 Jan 2023 12:00:21 +0100 Subject: [PATCH 4/4] :recycle: Add comment for the initialization --- framework/src/engine/generator/generator.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/framework/src/engine/generator/generator.ts b/framework/src/engine/generator/generator.ts index a87a0b33115..899f4bdfa01 100644 --- a/framework/src/engine/generator/generator.ts +++ b/framework/src/engine/generator/generator.ts @@ -199,6 +199,11 @@ export class Generator { }); const stateStore = new StateStore(this._blockchainDB); + + // On node start, it re generates certificate from maxHeightCertified to maxHeightPrecommitted. + // in the _handleFinalizedHeightChanged, it loops between maxHeightCertified + 1 and maxHeightPrecommitted. + // maxHeightCertified is skipped because it has been already certified. + // @see https://github.com/LiskHQ/lips/blob/main/proposals/lip-0061.md#initial-single-commit-creation const { maxHeightPrecommitted, maxHeightCertified } = await this._bft.method.getBFTHeights( stateStore, );