From b34b8bdec58e693a764959f9977944d5b4c6767c Mon Sep 17 00:00:00 2001 From: Ishan Date: Tue, 2 Nov 2021 03:53:46 +0100 Subject: [PATCH 1/4] =?UTF-8?q?=F0=9F=8C=B1=20Add=20block=20verification?= =?UTF-8?q?=20-=20Verify=20the=20timestamp,=20height,=20previousBlockID,?= =?UTF-8?q?=20generatorAddress,=20maxHeightPrevoted,=20maxHeightGenerated,?= =?UTF-8?q?=20and=20signature=20properties=20-=20Update=20interface=20for?= =?UTF-8?q?=20bft=20and=20validator=20module=20API?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- framework/src/node/consensus/consensus.ts | 130 +++++++++++++++++++++- framework/src/node/consensus/types.ts | 8 +- 2 files changed, 134 insertions(+), 4 deletions(-) diff --git a/framework/src/node/consensus/consensus.ts b/framework/src/node/consensus/consensus.ts index f0726a116f0..96feb30f154 100644 --- a/framework/src/node/consensus/consensus.ts +++ b/framework/src/node/consensus/consensus.ts @@ -45,7 +45,7 @@ import { } from './constants'; import { GenesisConfig } from '../../types'; import { ValidatorAPI, BFTAPI } from './types'; -import { createAPIContext } from '../state_machine'; +import { APIContext, createAPIContext } from '../state_machine'; import { forkChoice, ForkStatus } from './fork_choice/fork_choice_rule'; import { createNewAPIContext } from '../state_machine/api_context'; @@ -465,10 +465,134 @@ export class Consensus { if (block.header.version !== BLOCK_VERSION) { throw new ApplyPenaltyError(`Block version must be ${BLOCK_VERSION}`); } + const apiContext = createNewAPIContext(this._db); + const isValidTimestamp = await this._verifyTimestamp(apiContext, block); + // Verify timestamp + if (!isValidTimestamp) { + throw new Error( + `Invalid timestamp ${ + block.header.timestamp + } of the block with id: ${block.header.id.toString('hex')}`, + ); + } + // Verify height + if (!this._verifyBlockHeight(block)) { + throw new Error( + `Invalid height ${block.header.height} of the block with id: ${block.header.id.toString( + 'hex', + )}`, + ); + } + // Verify previousBlockID + if (!this._verifyPreviousBlockID(block)) { + throw new Error( + `Invalid previousBlockID ${block.header.previousBlockID.toString( + 'hex', + )} of the block with id: ${block.header.id.toString('hex')}`, + ); + } + // Verify generatorAddress + const isValidGeneratorAddress = await this._verifyGeneratorAddress(apiContext, block); + if (!isValidGeneratorAddress) { + throw new Error( + `Invalid generatorAddress ${block.header.generatorAddress.toString( + 'hex', + )} of the block with id: ${block.header.id.toString('hex')}`, + ); + } + + // Verify BFT Properties + const isValidBFTProperties = await this._verifyBFTProperties(apiContext, block); + if (!isValidBFTProperties) { + throw new Error( + `Invalid maxHeightPrevoted ${block.header.maxHeightPrevoted} or maxHeightGenerated ${ + block.header.maxHeightGenerated + } of the block with id: ${block.header.id.toString('hex')}`, + ); + } + // verify Block signature + const isSignatureValid = await this._verifyBlockSignature(apiContext, block); + if (!isSignatureValid) { + throw new Error( + `Invalid signature ${block.header.signature.toString( + 'hex', + )} of the block with id: ${block.header.id.toString('hex')}`, + ); + } + } + + private async _verifyTimestamp(apiContext: APIContext, block: Block): Promise { + const blockSlotNumber = await this._validatorAPI.getSlotNumber( + apiContext, + block.header.timestamp, + ); + // Check that block is not from the future + const currentTimestamp = Math.floor(Date.now() / 1000); + const currentSlotNumber = await this._validatorAPI.getSlotNumber(apiContext, currentTimestamp); + if (blockSlotNumber > currentSlotNumber) { + return false; + } + + // Check that block slot is strictly larger than the block slot of previousBlock + const { lastBlock } = this._chain; + const previousBlockSlotNumber = await this._validatorAPI.getSlotNumber( + apiContext, + lastBlock.header.timestamp, + ); + if (blockSlotNumber <= previousBlockSlotNumber) { + return false; + } + + return true; + } + + private _verifyPreviousBlockID(block: Block): boolean { + const { lastBlock } = this._chain; + + return block.header.previousBlockID.equals(lastBlock.header.id); + } + + private _verifyBlockHeight(block: Block): boolean { + const { lastBlock } = this._chain; + + return block.header.height === lastBlock.header.height + 1; + } + + private async _verifyGeneratorAddress(apiContext: APIContext, block: Block): Promise { + // Check that the generatorAddress has the correct length of 20 bytes + if (block.header.generatorAddress.length !== 20) { + return false; + } + const generatorAddress = await this._validatorAPI.getGeneratorAtTimestamp( + apiContext, + block.header.timestamp, + ); + // Check that the block generator is eligible to generate in this block slot. + return block.header.generatorAddress.equals(generatorAddress); + } + + private async _verifyBFTProperties(apiContext: APIContext, block: Block): Promise { + const bftParams = await this._bftAPI.getBFTHeights(apiContext); + + if (block.header.maxHeightPrevoted !== bftParams.maxHeightPrevoted) { + return false; + } + + return this._bftAPI.isHeaderContradictingChain(apiContext, block.header); + } + + private async _verifyBlockSignature(apiContext: APIContext, block: Block): Promise { + const { generatorKey } = await this._validatorAPI.getValidatorAccount( + apiContext, + block.header.generatorAddress, + ); + try { - await this._chain.verifyBlock(block); + block.header.validateSignature(generatorKey, this._chain.networkIdentifier); + + return true; } catch (error) { - throw new ApplyPenaltyError((error as Error).message ?? 'Invalid block to be processed'); + return false; } } diff --git a/framework/src/node/consensus/types.ts b/framework/src/node/consensus/types.ts index 87e80bb583f..6e4bfaf9369 100644 --- a/framework/src/node/consensus/types.ts +++ b/framework/src/node/consensus/types.ts @@ -13,7 +13,8 @@ */ import { BFTHeights } from '../../modules/bft/types'; -import { ImmutableAPIContext } from '../state_machine'; +import { ValidatorKeys } from '../../modules/validators/types'; +import { BlockHeader, ImmutableAPIContext } from '../state_machine'; export interface BFTHeader { id: Buffer; @@ -35,9 +36,14 @@ export interface ValidatorAPI { getGeneratorAtTimestamp: (apiContext: ImmutableAPIContext, timestamp: number) => Promise; getSlotNumber: (apiContext: ImmutableAPIContext, timestamp: number) => Promise; getSlotTime: (apiContext: ImmutableAPIContext, slot: number) => Promise; + getValidatorAccount: (apiContext: ImmutableAPIContext, address: Buffer) => Promise; } export interface BFTAPI { getCurrentValidators: (apiContext: ImmutableAPIContext) => Promise; getBFTHeights: (apiClient: ImmutableAPIContext) => Promise; + isHeaderContradictingChain: ( + apiClient: ImmutableAPIContext, + header: BlockHeader, + ) => Promise; } From 2ec8bba3bbb85079180e1f380adb6fb13fe28b4e Mon Sep 17 00:00:00 2001 From: Ishan Date: Tue, 2 Nov 2021 03:55:12 +0100 Subject: [PATCH 2/4] =?UTF-8?q?=E2=9C=85=20Add=20all=20tests=20for=20block?= =?UTF-8?q?=20verification?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../unit/node/consensus/consensus.spec.ts | 226 +++++++++++++++++- 1 file changed, 217 insertions(+), 9 deletions(-) diff --git a/framework/test/unit/node/consensus/consensus.spec.ts b/framework/test/unit/node/consensus/consensus.spec.ts index ad843a7f7bb..ea27eab28d1 100644 --- a/framework/test/unit/node/consensus/consensus.spec.ts +++ b/framework/test/unit/node/consensus/consensus.spec.ts @@ -12,9 +12,12 @@ * Removal or modification of this copyright notice is prohibited. */ -import { Block, Chain } from '@liskhq/lisk-chain'; +import { Block, BlockAssets, Chain } from '@liskhq/lisk-chain'; import { KVStore } from '@liskhq/lisk-db'; import { codec } from '@liskhq/lisk-codec'; +import * as cryptography from '@liskhq/lisk-cryptography'; +import { when } from 'jest-when'; +import { Mnemonic } from '@liskhq/lisk-passphrase'; import { ApplyPenaltyError } from '../../../../src/errors'; import { CONSENSUS_EVENT_BLOCK_BROADCAST, @@ -33,9 +36,16 @@ import { import { Network } from '../../../../src/node/network'; import { StateMachine } from '../../../../src/node/state_machine/state_machine'; import { loggerMock } from '../../../../src/testing/mocks'; -import { createValidDefaultBlock, genesisBlock } from '../../../fixtures'; +import { + createFakeBlockHeader, + createValidDefaultBlock, + defaultNetworkIdentifier, + genesisBlock, +} from '../../../fixtures'; import * as forkchoice from '../../../../src/node/consensus/fork_choice/fork_choice_rule'; import { postBlockEventSchema } from '../../../../src/node/consensus/schema'; +import { APIContext } from '../../../../src/node/state_machine'; +import { createTransientAPIContext } from '../../../../src/testing'; describe('consensus', () => { const genesis = (genesisBlock() as unknown) as Block; @@ -79,8 +89,13 @@ describe('consensus', () => { getBFTHeights: jest .fn() .mockResolvedValue({ maxHeghgtPrevoted: 0, maxHeightPrecommitted: 0 }), + isHeaderContradictingChain: jest.fn(), + } as never; + validatorAPI = { + getGeneratorAtTimestamp: jest.fn(), + getValidatorAccount: jest.fn(), + getSlotNumber: jest.fn(), } as never; - validatorAPI = {} as never; consensus = new Consensus({ chain, network, @@ -254,7 +269,7 @@ describe('consensus', () => { .spyOn(forkchoice, 'forkChoice') .mockReturnValue(forkchoice.ForkStatus.DOUBLE_FORGING); jest.spyOn(consensus.events, 'emit'); - jest.spyOn(consensus, '_verify' as any); + jest.spyOn(consensus, '_verify' as any).mockResolvedValue(true); jest.spyOn(consensus, '_executeValidated' as any); await consensus.onBlockReceive(input, peerID); }); @@ -276,7 +291,7 @@ describe('consensus', () => { beforeEach(async () => { jest.spyOn(forkchoice, 'forkChoice').mockReturnValue(forkchoice.ForkStatus.TIE_BREAK); jest.spyOn(consensus.events, 'emit'); - jest.spyOn(consensus, '_verify' as any); + jest.spyOn(consensus, '_verify' as any).mockResolvedValue(true); jest.spyOn(consensus, '_executeValidated' as any).mockResolvedValue(undefined); jest.spyOn(consensus, 'finalizedHeight').mockReturnValue(0); await consensus.onBlockReceive(input, peerID); @@ -303,7 +318,7 @@ describe('consensus', () => { beforeEach(async () => { jest.spyOn(forkchoice, 'forkChoice').mockReturnValue(forkchoice.ForkStatus.TIE_BREAK); jest.spyOn(consensus.events, 'emit'); - jest.spyOn(consensus, '_verify' as any); + jest.spyOn(consensus, '_verify' as any).mockResolvedValue(true); jest .spyOn(consensus, '_executeValidated' as any) .mockRejectedValueOnce(new Error('invalid block')); @@ -338,7 +353,7 @@ describe('consensus', () => { .spyOn(forkchoice, 'forkChoice') .mockReturnValue(forkchoice.ForkStatus.DIFFERENT_CHAIN); jest.spyOn(consensus.events, 'emit'); - jest.spyOn(consensus, '_verify' as any); + jest.spyOn(consensus, '_verify' as any).mockResolvedValue(true); jest.spyOn(consensus, '_executeValidated' as any); jest.spyOn(consensus['_synchronizer'], 'run').mockResolvedValue(undefined); await consensus.onBlockReceive(input, peerID); @@ -447,7 +462,7 @@ describe('consensus', () => { beforeEach(async () => { jest.spyOn(forkchoice, 'forkChoice').mockReturnValue(forkchoice.ForkStatus.DISCARD); jest.spyOn(consensus.events, 'emit'); - jest.spyOn(consensus, '_verify' as any); + jest.spyOn(consensus, '_verify' as any).mockResolvedValue(true); jest.spyOn(consensus, '_executeValidated' as any).mockResolvedValue(undefined); await consensus.onBlockReceive(input, peerID); }); @@ -469,7 +484,7 @@ describe('consensus', () => { beforeEach(async () => { jest.spyOn(forkchoice, 'forkChoice').mockReturnValue(forkchoice.ForkStatus.VALID_BLOCK); jest.spyOn(consensus.events, 'emit'); - jest.spyOn(consensus, '_verify' as any); + jest.spyOn(consensus, '_verify' as any).mockResolvedValue(true); jest.spyOn(consensus, '_executeValidated' as any).mockResolvedValue(undefined); await consensus.onBlockReceive(input, peerID); }); @@ -488,9 +503,11 @@ describe('consensus', () => { describe('execute', () => { let block: Block; + let apiContext: APIContext; beforeEach(async () => { block = await createValidDefaultBlock({ header: { height: 2 } }); + apiContext = createTransientAPIContext({}); jest.spyOn(chain, 'saveBlock').mockResolvedValue(); jest.spyOn(consensus, 'finalizedHeight').mockReturnValue(0); @@ -501,6 +518,197 @@ describe('consensus', () => { await consensus.execute(block); }); + describe('block verification', () => { + describe('timestamp', () => { + it('should return false when block timestamp is from future', async () => { + const invalidBlock = { ...block }; + const now = Date.now(); + + Date.now = jest.fn(() => now); + + (invalidBlock.header as any).timestamp = Math.floor((Date.now() + 10000) / 1000); + when(consensus['_validatorAPI'].getSlotNumber as any) + .calledWith(apiContext, (invalidBlock.header as any).timestamp) + .mockResolvedValue(10 as never) + .calledWith(apiContext, Math.floor(now / 1000)) + .mockResolvedValue(5 as never); + + await expect( + consensus['_verifyTimestamp'](apiContext, invalidBlock as any), + ).resolves.toEqual(false); + }); + + it('should return false when block slot is less than previous block slot', async () => { + const invalidBlock = { ...block }; + const now = Date.now(); + + Date.now = jest.fn(() => now); + + (invalidBlock.header as any).timestamp = Math.floor(Date.now() / 1000); + when(consensus['_validatorAPI'].getSlotNumber as any) + .calledWith(apiContext, (invalidBlock.header as any).timestamp) + .mockResolvedValue(10 as never) + .calledWith(apiContext, Math.floor(now / 1000)) + .mockResolvedValue(10 as never); + + (consensus['_chain'].lastBlock.header as any).timestamp = Math.floor(Date.now() / 1000); + + await expect( + consensus['_verifyTimestamp'](apiContext, invalidBlock as any), + ).resolves.toEqual(false); + }); + + it('should return true when valid block timestamp', async () => { + const now = Date.now(); + + Date.now = jest.fn(() => now); + + (block.header as any).timestamp = Math.floor(Date.now() / 1000); + when(consensus['_validatorAPI'].getSlotNumber as any) + .calledWith(apiContext, (block.header as any).timestamp) + .mockResolvedValue(10 as never) + .calledWith(apiContext, Math.floor(now / 1000)) + .mockResolvedValue(10 as never); + + await expect(consensus['_verifyTimestamp'](apiContext, block as any)).resolves.toEqual( + true, + ); + }); + }); + + describe('height', () => { + it('should return false when block height is equal to last block height', () => { + const invalidBlock = { ...block }; + (invalidBlock.header as any).height = consensus['_chain'].lastBlock.header.height; + + expect(consensus['_verifyBlockHeight'](block as any)).toEqual(false); + }); + it('should return true when block has [height===lastBlock.height+1]', () => { + expect(consensus['_verifyBlockHeight'](block as any)).toEqual(true); + }); + }); + + describe('previousBlockID', () => { + it('should return false for invalid previousBlockID', () => { + const invalidBlock = { ...block }; + (invalidBlock.header as any).previousBlockID = cryptography.getRandomBytes(64); + expect(consensus['_verifyPreviousBlockID'](block as any)).toEqual(false); + }); + it('should return true when previousBlockID is equal to lastBlock ID', async () => { + const validBlock = await createValidDefaultBlock({ + header: { height: 2, previousBlockID: consensus['_chain'].lastBlock.header.id }, + }); + expect(consensus['_verifyPreviousBlockID'](validBlock as any)).toEqual(true); + }); + }); + + describe('generatorAddress', () => { + it('should return false if [generatorAddress.lenght !== 20]', async () => { + const invalidBlock = { ...block }; + (invalidBlock.header as any).generatorAddress = cryptography.getRandomBytes(64); + await expect( + consensus['_verifyGeneratorAddress'](apiContext, block as any), + ).resolves.toEqual(false); + }); + + it('should return false if generatorAddress has wrong block slot', async () => { + when(consensus['_validatorAPI'].getGeneratorAtTimestamp as never) + .calledWith(apiContext, (block.header as any).timestamp) + .mockResolvedValue(cryptography.getRandomBytes(20) as never); + + await expect( + consensus['_verifyGeneratorAddress'](apiContext, block as any), + ).resolves.toEqual(false); + }); + + it('should return true if generatorAddress is valid and has right block slot', async () => { + when(consensus['_validatorAPI'].getGeneratorAtTimestamp as never) + .calledWith(apiContext, (block.header as any).timestamp) + .mockResolvedValue(block.header.generatorAddress as never); + + await expect( + consensus['_verifyGeneratorAddress'](apiContext, block as any), + ).resolves.toEqual(true); + }); + }); + + describe('bftProperties', () => { + it('should return false for invalid maxHeightPrevoted', async () => { + when(consensus['_bftAPI'].getBFTHeights as never) + .calledWith(apiContext) + .mockResolvedValue({ maxHeightPrevoted: block.header.maxHeightPrevoted + 1 } as never); + + await expect( + consensus['_verifyBFTProperties'](apiContext, block as any), + ).resolves.toEqual(false); + }); + + it('should return false if the header is contradicting', async () => { + when(consensus['_bftAPI'].getBFTHeights as never) + .calledWith(apiContext) + .mockResolvedValue({ maxHeightPrevoted: block.header.maxHeightPrevoted } as never); + + when(consensus['_bftAPI'].isHeaderContradictingChain as never) + .calledWith(apiContext, block.header) + .mockResolvedValue(false as never); + + await expect( + consensus['_verifyBFTProperties'](apiContext, block as any), + ).resolves.toEqual(false); + }); + + it('should return true if maxHeightPrevoted is valid and header is not contradicting', async () => { + when(consensus['_bftAPI'].getBFTHeights as never) + .calledWith(apiContext) + .mockResolvedValue({ maxHeightPrevoted: block.header.maxHeightPrevoted } as never); + + when(consensus['_bftAPI'].isHeaderContradictingChain as never) + .calledWith(apiContext, block.header) + .mockResolvedValue(true as never); + + await expect( + consensus['_verifyBFTProperties'](apiContext, block as any), + ).resolves.toEqual(true); + }); + }); + + describe('signature', () => { + it('should return false for invalid signature', async () => { + const generatorKey = cryptography.getRandomBytes(32); + + when(consensus['_validatorAPI'].getValidatorAccount as never) + .calledWith(apiContext, block.header.generatorAddress) + .mockResolvedValue({ generatorKey } as never); + + await expect( + consensus['_verifyBlockSignature'](apiContext, block as any), + ).resolves.toEqual(false); + }); + + it('should return true when valid signature', async () => { + const passphrase = Mnemonic.generateMnemonic(); + const keyPair = cryptography.getPrivateAndPublicKeyFromPassphrase(passphrase); + + const blockHeader = createFakeBlockHeader(); + (blockHeader as any).generatorAddress = cryptography.getAddressFromPublicKey( + keyPair.publicKey, + ); + (consensus['_chain'] as any).networkIdentifier = defaultNetworkIdentifier; + + blockHeader.sign(consensus['_chain'].networkIdentifier, keyPair.privateKey); + const validBlock = new Block(blockHeader, [], new BlockAssets()); + + when(consensus['_validatorAPI'].getValidatorAccount as never) + .calledWith(apiContext, validBlock.header.generatorAddress) + .mockResolvedValue({ generatorKey: keyPair.publicKey } as never); + + await expect( + consensus['_verifyBlockSignature'](apiContext, validBlock as any), + ).resolves.toEqual(true); + }); + }); + }); + it('should verify block using state machine', () => { expect(stateMachine.verifyBlock).toHaveBeenCalledTimes(1); }); From 8a3e199860d84ac281b4a11e258669ac7a798466 Mon Sep 17 00:00:00 2001 From: Ishan Date: Tue, 2 Nov 2021 13:01:12 +0100 Subject: [PATCH 3/4] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20=20Throw=20error=20fro?= =?UTF-8?q?m=20individual=20block=20validation=20funcs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- framework/src/node/consensus/consensus.ts | 138 +++++++++--------- .../unit/node/consensus/consensus.spec.ts | 109 +++++++++----- 2 files changed, 142 insertions(+), 105 deletions(-) diff --git a/framework/src/node/consensus/consensus.ts b/framework/src/node/consensus/consensus.ts index 96feb30f154..7bf8a998203 100644 --- a/framework/src/node/consensus/consensus.ts +++ b/framework/src/node/consensus/consensus.ts @@ -466,62 +466,27 @@ export class Consensus { throw new ApplyPenaltyError(`Block version must be ${BLOCK_VERSION}`); } const apiContext = createNewAPIContext(this._db); - const isValidTimestamp = await this._verifyTimestamp(apiContext, block); + // Verify timestamp - if (!isValidTimestamp) { - throw new Error( - `Invalid timestamp ${ - block.header.timestamp - } of the block with id: ${block.header.id.toString('hex')}`, - ); - } + await this._verifyTimestamp(apiContext, block); + // Verify height - if (!this._verifyBlockHeight(block)) { - throw new Error( - `Invalid height ${block.header.height} of the block with id: ${block.header.id.toString( - 'hex', - )}`, - ); - } + this._verifyBlockHeight(block); + // Verify previousBlockID - if (!this._verifyPreviousBlockID(block)) { - throw new Error( - `Invalid previousBlockID ${block.header.previousBlockID.toString( - 'hex', - )} of the block with id: ${block.header.id.toString('hex')}`, - ); - } + this._verifyPreviousBlockID(block); + // Verify generatorAddress - const isValidGeneratorAddress = await this._verifyGeneratorAddress(apiContext, block); - if (!isValidGeneratorAddress) { - throw new Error( - `Invalid generatorAddress ${block.header.generatorAddress.toString( - 'hex', - )} of the block with id: ${block.header.id.toString('hex')}`, - ); - } + await this._verifyGeneratorAddress(apiContext, block); // Verify BFT Properties - const isValidBFTProperties = await this._verifyBFTProperties(apiContext, block); - if (!isValidBFTProperties) { - throw new Error( - `Invalid maxHeightPrevoted ${block.header.maxHeightPrevoted} or maxHeightGenerated ${ - block.header.maxHeightGenerated - } of the block with id: ${block.header.id.toString('hex')}`, - ); - } + await this._verifyBFTProperties(apiContext, block); + // verify Block signature - const isSignatureValid = await this._verifyBlockSignature(apiContext, block); - if (!isSignatureValid) { - throw new Error( - `Invalid signature ${block.header.signature.toString( - 'hex', - )} of the block with id: ${block.header.id.toString('hex')}`, - ); - } + await this._verifyBlockSignature(apiContext, block); } - private async _verifyTimestamp(apiContext: APIContext, block: Block): Promise { + private async _verifyTimestamp(apiContext: APIContext, block: Block): Promise { const blockSlotNumber = await this._validatorAPI.getSlotNumber( apiContext, block.header.timestamp, @@ -530,7 +495,11 @@ export class Consensus { const currentTimestamp = Math.floor(Date.now() / 1000); const currentSlotNumber = await this._validatorAPI.getSlotNumber(apiContext, currentTimestamp); if (blockSlotNumber > currentSlotNumber) { - return false; + throw new Error( + `Invalid timestamp ${ + block.header.timestamp + } of the block with id: ${block.header.id.toString('hex')}`, + ); } // Check that block slot is strictly larger than the block slot of previousBlock @@ -540,48 +509,83 @@ export class Consensus { lastBlock.header.timestamp, ); if (blockSlotNumber <= previousBlockSlotNumber) { - return false; + throw new Error( + `Invalid timestamp ${ + block.header.timestamp + } of the block with id: ${block.header.id.toString('hex')}`, + ); } - - return true; } - private _verifyPreviousBlockID(block: Block): boolean { + private _verifyPreviousBlockID(block: Block): void { const { lastBlock } = this._chain; - return block.header.previousBlockID.equals(lastBlock.header.id); + if (!block.header.previousBlockID.equals(lastBlock.header.id)) { + throw new Error( + `Invalid previousBlockID ${block.header.previousBlockID.toString( + 'hex', + )} of the block with id: ${block.header.id.toString('hex')}`, + ); + } } - private _verifyBlockHeight(block: Block): boolean { + private _verifyBlockHeight(block: Block): void { const { lastBlock } = this._chain; - return block.header.height === lastBlock.header.height + 1; + if (block.header.height !== lastBlock.header.height + 1) { + throw new Error( + `Invalid height ${block.header.height} of the block with id: ${block.header.id.toString( + 'hex', + )}`, + ); + } } - private async _verifyGeneratorAddress(apiContext: APIContext, block: Block): Promise { + private async _verifyGeneratorAddress(apiContext: APIContext, block: Block): Promise { // Check that the generatorAddress has the correct length of 20 bytes if (block.header.generatorAddress.length !== 20) { - return false; + throw new Error( + `Invalid length of generatorAddress ${block.header.generatorAddress.toString( + 'hex', + )} of the block with id: ${block.header.id.toString('hex')}`, + ); } const generatorAddress = await this._validatorAPI.getGeneratorAtTimestamp( apiContext, block.header.timestamp, ); // Check that the block generator is eligible to generate in this block slot. - return block.header.generatorAddress.equals(generatorAddress); + if (!block.header.generatorAddress.equals(generatorAddress)) { + throw new Error( + `Not valid generatorAddress for current block slot ${block.header.generatorAddress.toString( + 'hex', + )} of the block with id: ${block.header.id.toString('hex')}`, + ); + } } - private async _verifyBFTProperties(apiContext: APIContext, block: Block): Promise { + private async _verifyBFTProperties(apiContext: APIContext, block: Block): Promise { const bftParams = await this._bftAPI.getBFTHeights(apiContext); if (block.header.maxHeightPrevoted !== bftParams.maxHeightPrevoted) { - return false; + throw new Error( + `Invalid maxHeightPrevoted ${ + block.header.maxHeightPrevoted + } of the block with id: ${block.header.id.toString('hex')}`, + ); + } + const isContradictingHeaders = await this._bftAPI.isHeaderContradictingChain( + apiContext, + block.header, + ); + if (isContradictingHeaders) { + throw new Error( + `Contradicting headers for the block with id: ${block.header.id.toString('hex')}`, + ); } - - return this._bftAPI.isHeaderContradictingChain(apiContext, block.header); } - private async _verifyBlockSignature(apiContext: APIContext, block: Block): Promise { + private async _verifyBlockSignature(apiContext: APIContext, block: Block): Promise { const { generatorKey } = await this._validatorAPI.getValidatorAccount( apiContext, block.header.generatorAddress, @@ -589,10 +593,12 @@ export class Consensus { try { block.header.validateSignature(generatorKey, this._chain.networkIdentifier); - - return true; } catch (error) { - return false; + throw new Error( + `Invalid signature ${block.header.signature.toString( + 'hex', + )} of the block with id: ${block.header.id.toString('hex')}`, + ); } } diff --git a/framework/test/unit/node/consensus/consensus.spec.ts b/framework/test/unit/node/consensus/consensus.spec.ts index ea27eab28d1..a8aebcf61f2 100644 --- a/framework/test/unit/node/consensus/consensus.spec.ts +++ b/framework/test/unit/node/consensus/consensus.spec.ts @@ -520,7 +520,7 @@ describe('consensus', () => { describe('block verification', () => { describe('timestamp', () => { - it('should return false when block timestamp is from future', async () => { + it('should throw error when block timestamp is from future', async () => { const invalidBlock = { ...block }; const now = Date.now(); @@ -535,10 +535,14 @@ describe('consensus', () => { await expect( consensus['_verifyTimestamp'](apiContext, invalidBlock as any), - ).resolves.toEqual(false); + ).rejects.toThrow( + `Invalid timestamp ${ + invalidBlock.header.timestamp + } of the block with id: ${invalidBlock.header.id.toString('hex')}`, + ); }); - it('should return false when block slot is less than previous block slot', async () => { + it('should throw error when block slot is less than previous block slot', async () => { const invalidBlock = { ...block }; const now = Date.now(); @@ -555,10 +559,14 @@ describe('consensus', () => { await expect( consensus['_verifyTimestamp'](apiContext, invalidBlock as any), - ).resolves.toEqual(false); + ).rejects.toThrow( + `Invalid timestamp ${ + invalidBlock.header.timestamp + } of the block with id: ${invalidBlock.header.id.toString('hex')}`, + ); }); - it('should return true when valid block timestamp', async () => { + it('should be success when valid block timestamp', async () => { const now = Date.now(); Date.now = jest.fn(() => now); @@ -570,110 +578,129 @@ describe('consensus', () => { .calledWith(apiContext, Math.floor(now / 1000)) .mockResolvedValue(10 as never); - await expect(consensus['_verifyTimestamp'](apiContext, block as any)).resolves.toEqual( - true, - ); + await expect( + consensus['_verifyTimestamp'](apiContext, block as any), + ).resolves.toBeUndefined(); }); }); describe('height', () => { - it('should return false when block height is equal to last block height', () => { + it('should throw error when block height is equal to last block height', () => { const invalidBlock = { ...block }; (invalidBlock.header as any).height = consensus['_chain'].lastBlock.header.height; - expect(consensus['_verifyBlockHeight'](block as any)).toEqual(false); + expect(() => consensus['_verifyBlockHeight'](invalidBlock as any)).toThrow( + `Invalid height ${ + invalidBlock.header.height + } of the block with id: ${invalidBlock.header.id.toString('hex')}`, + ); }); - it('should return true when block has [height===lastBlock.height+1]', () => { - expect(consensus['_verifyBlockHeight'](block as any)).toEqual(true); + it('should be success when block has [height===lastBlock.height+1]', () => { + expect(consensus['_verifyBlockHeight'](block as any)).toBeUndefined(); }); }); describe('previousBlockID', () => { - it('should return false for invalid previousBlockID', () => { + it('should throw error for invalid previousBlockID', () => { const invalidBlock = { ...block }; (invalidBlock.header as any).previousBlockID = cryptography.getRandomBytes(64); - expect(consensus['_verifyPreviousBlockID'](block as any)).toEqual(false); + + expect(() => consensus['_verifyPreviousBlockID'](block as any)).toThrow( + `Invalid previousBlockID ${invalidBlock.header.previousBlockID.toString( + 'hex', + )} of the block with id: ${invalidBlock.header.id.toString('hex')}`, + ); }); - it('should return true when previousBlockID is equal to lastBlock ID', async () => { + it('should be success when previousBlockID is equal to lastBlock ID', async () => { const validBlock = await createValidDefaultBlock({ header: { height: 2, previousBlockID: consensus['_chain'].lastBlock.header.id }, }); - expect(consensus['_verifyPreviousBlockID'](validBlock as any)).toEqual(true); + expect(consensus['_verifyPreviousBlockID'](validBlock as any)).toBeUndefined(); }); }); describe('generatorAddress', () => { - it('should return false if [generatorAddress.lenght !== 20]', async () => { + it('should throw error if [generatorAddress.lenght !== 20]', async () => { const invalidBlock = { ...block }; (invalidBlock.header as any).generatorAddress = cryptography.getRandomBytes(64); await expect( - consensus['_verifyGeneratorAddress'](apiContext, block as any), - ).resolves.toEqual(false); + consensus['_verifyGeneratorAddress'](apiContext, invalidBlock as any), + ).rejects.toThrow( + `Invalid length of generatorAddress ${invalidBlock.header.generatorAddress.toString( + 'hex', + )} of the block with id: ${invalidBlock.header.id.toString('hex')}`, + ); }); - it('should return false if generatorAddress has wrong block slot', async () => { + it('should throw error if generatorAddress has wrong block slot', async () => { when(consensus['_validatorAPI'].getGeneratorAtTimestamp as never) .calledWith(apiContext, (block.header as any).timestamp) .mockResolvedValue(cryptography.getRandomBytes(20) as never); await expect( consensus['_verifyGeneratorAddress'](apiContext, block as any), - ).resolves.toEqual(false); + ).rejects.toThrow( + `Not valid generatorAddress for current block slot ${block.header.generatorAddress.toString( + 'hex', + )} of the block with id: ${block.header.id.toString('hex')}`, + ); }); - it('should return true if generatorAddress is valid and has right block slot', async () => { + it('should be success if generatorAddress is valid and has right block slot', async () => { when(consensus['_validatorAPI'].getGeneratorAtTimestamp as never) .calledWith(apiContext, (block.header as any).timestamp) .mockResolvedValue(block.header.generatorAddress as never); await expect( consensus['_verifyGeneratorAddress'](apiContext, block as any), - ).resolves.toEqual(true); + ).resolves.toBeUndefined(); }); }); describe('bftProperties', () => { - it('should return false for invalid maxHeightPrevoted', async () => { + it('should throw error for invalid maxHeightPrevoted', async () => { when(consensus['_bftAPI'].getBFTHeights as never) .calledWith(apiContext) .mockResolvedValue({ maxHeightPrevoted: block.header.maxHeightPrevoted + 1 } as never); - await expect( - consensus['_verifyBFTProperties'](apiContext, block as any), - ).resolves.toEqual(false); + await expect(consensus['_verifyBFTProperties'](apiContext, block as any)).rejects.toThrow( + `Invalid maxHeightPrevoted ${ + block.header.maxHeightPrevoted + } of the block with id: ${block.header.id.toString('hex')}`, + ); }); - it('should return false if the header is contradicting', async () => { + it('should throw error if the header is contradicting', async () => { when(consensus['_bftAPI'].getBFTHeights as never) .calledWith(apiContext) .mockResolvedValue({ maxHeightPrevoted: block.header.maxHeightPrevoted } as never); when(consensus['_bftAPI'].isHeaderContradictingChain as never) .calledWith(apiContext, block.header) - .mockResolvedValue(false as never); + .mockResolvedValue(true as never); - await expect( - consensus['_verifyBFTProperties'](apiContext, block as any), - ).resolves.toEqual(false); + await expect(consensus['_verifyBFTProperties'](apiContext, block as any)).rejects.toThrow( + `Contradicting headers for the block with id: ${block.header.id.toString('hex')}`, + ); }); - it('should return true if maxHeightPrevoted is valid and header is not contradicting', async () => { + it('should be success if maxHeightPrevoted is valid and header is not contradicting', async () => { when(consensus['_bftAPI'].getBFTHeights as never) .calledWith(apiContext) .mockResolvedValue({ maxHeightPrevoted: block.header.maxHeightPrevoted } as never); when(consensus['_bftAPI'].isHeaderContradictingChain as never) .calledWith(apiContext, block.header) - .mockResolvedValue(true as never); + .mockResolvedValue(false as never); await expect( consensus['_verifyBFTProperties'](apiContext, block as any), - ).resolves.toEqual(true); + ).resolves.toBeUndefined(); }); }); describe('signature', () => { - it('should return false for invalid signature', async () => { + it('should throw error for invalid signature', async () => { const generatorKey = cryptography.getRandomBytes(32); when(consensus['_validatorAPI'].getValidatorAccount as never) @@ -682,10 +709,14 @@ describe('consensus', () => { await expect( consensus['_verifyBlockSignature'](apiContext, block as any), - ).resolves.toEqual(false); + ).rejects.toThrow( + `Invalid signature ${block.header.signature.toString( + 'hex', + )} of the block with id: ${block.header.id.toString('hex')}`, + ); }); - it('should return true when valid signature', async () => { + it('should be success when valid signature', async () => { const passphrase = Mnemonic.generateMnemonic(); const keyPair = cryptography.getPrivateAndPublicKeyFromPassphrase(passphrase); @@ -704,7 +735,7 @@ describe('consensus', () => { await expect( consensus['_verifyBlockSignature'](apiContext, validBlock as any), - ).resolves.toEqual(true); + ).resolves.toBeUndefined(); }); }); }); From 88ecc350d2f2d4d1c3cea3eef87874cc6266f977 Mon Sep 17 00:00:00 2001 From: !shan Date: Thu, 4 Nov 2021 21:05:59 +0100 Subject: [PATCH 4/4] =?UTF-8?q?=F0=9F=92=85=20Improve=20error=20message?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Incede <33103370+Incede@users.noreply.github.com> --- framework/src/node/consensus/consensus.ts | 6 ++++-- framework/test/unit/node/consensus/consensus.spec.ts | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/framework/src/node/consensus/consensus.ts b/framework/src/node/consensus/consensus.ts index 7bf8a998203..28e50dd85f4 100644 --- a/framework/src/node/consensus/consensus.ts +++ b/framework/src/node/consensus/consensus.ts @@ -557,9 +557,11 @@ export class Consensus { // Check that the block generator is eligible to generate in this block slot. if (!block.header.generatorAddress.equals(generatorAddress)) { throw new Error( - `Not valid generatorAddress for current block slot ${block.header.generatorAddress.toString( + `Generator with address ${block.header.generatorAddress.toString( 'hex', - )} of the block with id: ${block.header.id.toString('hex')}`, + )} of the block with id: ${block.header.id.toString( + 'hex', + )} is ineligible to generate block for the current slot`, ); } } diff --git a/framework/test/unit/node/consensus/consensus.spec.ts b/framework/test/unit/node/consensus/consensus.spec.ts index a8aebcf61f2..1dbfc85abd3 100644 --- a/framework/test/unit/node/consensus/consensus.spec.ts +++ b/framework/test/unit/node/consensus/consensus.spec.ts @@ -640,9 +640,11 @@ describe('consensus', () => { await expect( consensus['_verifyGeneratorAddress'](apiContext, block as any), ).rejects.toThrow( - `Not valid generatorAddress for current block slot ${block.header.generatorAddress.toString( + `Generator with address ${block.header.generatorAddress.toString( 'hex', - )} of the block with id: ${block.header.id.toString('hex')}`, + )} of the block with id: ${block.header.id.toString( + 'hex', + )} is ineligible to generate block for the current slot`, ); });