From b77cba15c14a03287c639d4e4716d4c89a0ae2c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Boban=20Milo=C5=A1evi=C4=87?= Date: Mon, 15 May 2023 17:48:39 +0800 Subject: [PATCH] Ban peers after failed block verification (#8445) * Ban peers after failed block verification * Rename private functions to use `Block` instead of `Assets` * Revert deep cloning of previous block after it is deleted --- framework/src/engine/consensus/consensus.ts | 68 +++++++++++-------- .../unit/engine/consensus/consensus.spec.ts | 65 +++++++++++++----- 2 files changed, 86 insertions(+), 47 deletions(-) diff --git a/framework/src/engine/consensus/consensus.ts b/framework/src/engine/consensus/consensus.ts index b911d7189c8..406c17e6b0f 100644 --- a/framework/src/engine/consensus/consensus.ts +++ b/framework/src/engine/consensus/consensus.ts @@ -441,6 +441,7 @@ export class Consensus { }); return; } + if (forkStatus === ForkStatus.IDENTICAL_BLOCK) { this._logger.debug( { id: block.header.id, height: block.header.height }, @@ -448,6 +449,7 @@ export class Consensus { ); return; } + if (forkStatus === ForkStatus.DOUBLE_FORGING) { this._logger.warn( { @@ -472,6 +474,7 @@ export class Consensus { }); return; } + // Discard block and move to different chain if (forkStatus === ForkStatus.DIFFERENT_CHAIN) { this._logger.debug( @@ -496,6 +499,7 @@ export class Consensus { await this._sync(block, peerID); return; } + // Replacing a block if (forkStatus === ForkStatus.TIE_BREAK) { this._logger.info( @@ -516,9 +520,14 @@ export class Consensus { block, }); - this._chain.validateBlock(block, { - version: BLOCK_VERSION, - }); + try { + this._chain.validateBlock(block, { + version: BLOCK_VERSION, + }); + } catch (error) { + throw new ApplyPenaltyError(error); + } + const previousLastBlock = objects.cloneDeep(lastBlock); await this._deleteBlock(lastBlock); try { @@ -543,9 +552,13 @@ export class Consensus { { id: block.header.id, height: block.header.height }, 'Processing valid block', ); - this._chain.validateBlock(block, { - version: BLOCK_VERSION, - }); + try { + this._chain.validateBlock(block, { + version: BLOCK_VERSION, + }); + } catch (error) { + throw new ApplyPenaltyError(error); + } await this._executeValidated(block); // Since legacy property is optional we don't need to send it here @@ -570,8 +583,14 @@ export class Consensus { } = {}, ): Promise { const stateStore = new StateStore(this._db); - await this._verify(block); - const contextID = await this._verifyAssets(block); + let contextID: Buffer; + + try { + await this._verify(block); + contextID = await this._verifyAssets(block); + } catch (error) { + throw new ApplyPenaltyError(error); + } if (!options.skipBroadcast) { this._network.send({ @@ -643,25 +662,12 @@ export class Consensus { private async _verify(block: Block): Promise { const stateStore = new StateStore(this._db); - // Verify timestamp this._verifyTimestamp(block); - - // Verify height - this._verifyAssetsHeight(block); - - // Verify previousBlockID + this._verifyBlockHeight(block); this._verifyPreviousBlockID(block); - - // Verify generatorAddress await this._verifyGeneratorAddress(stateStore, block); - - // Verify BFT Properties await this._verifyBFTProperties(stateStore, block); - - // verify Block signature - await this._verifyAssetsSignature(stateStore, block); - - // verify aggregate commits + await this._verifyBlockSignature(stateStore, block); await this._verifyAggregateCommit(stateStore, block); } @@ -678,7 +684,7 @@ export class Consensus { ); } - // Check that block slot is strictly larger than the block slot of previousBlock + // Check that the provided block slot is strictly higher than the block slot of previousBlock const { lastBlock } = this._chain; const previousBlockSlotNumber = this._bft.method.getSlotNumber(lastBlock.header.timestamp); if (blockSlotNumber <= previousBlockSlotNumber) { @@ -702,7 +708,7 @@ export class Consensus { } } - private _verifyAssetsHeight(block: Block): void { + private _verifyBlockHeight(block: Block): void { const { lastBlock } = this._chain; if (block.header.height !== lastBlock.header.height + 1) { @@ -723,13 +729,14 @@ export class Consensus { )} of the block with id: ${block.header.id.toString('hex')}`, ); } - const generator = await this._bft.method.getGeneratorAtTimestamp( + const expectedGenerator = await this._bft.method.getGeneratorAtTimestamp( stateStore, block.header.height, block.header.timestamp, ); + // Check that the block generator is eligible to generate in this block slot. - if (!block.header.generatorAddress.equals(generator.address)) { + if (!block.header.generatorAddress.equals(expectedGenerator.address)) { throw new Error( `Generator with address ${block.header.generatorAddress.toString( 'hex', @@ -750,6 +757,7 @@ export class Consensus { } of the block with id: ${block.header.id.toString('hex')}`, ); } + const isContradictingHeaders = await this._bft.method.isHeaderContradictingChain( stateStore, block.header, @@ -759,13 +767,14 @@ export class Consensus { `Contradicting headers for the block with id: ${block.header.id.toString('hex')}`, ); } + const implyMaxPrevote = await this._bft.method.impliesMaximalPrevotes(stateStore, block.header); if (block.header.impliesMaxPrevotes !== implyMaxPrevote) { throw new Error('Invalid imply max prevote.'); } } - private async _verifyAssetsSignature(stateStore: StateStore, block: Block): Promise { + private async _verifyBlockSignature(stateStore: StateStore, block: Block): Promise { const bftParams = await this._bft.method.getBFTParameters(stateStore, block.header.height); const generator = bftParams.validators.find(validator => validator.address.equals(block.header.generatorAddress), @@ -795,6 +804,7 @@ export class Consensus { `Aggregate Commit is "undefined" for the block with id: ${block.header.id.toString('hex')}`, ); } + const isVerified = await this._commitPool.verifyAggregateCommit( stateStore, block.header.aggregateCommit, @@ -812,11 +822,11 @@ export class Consensus { `Validators hash is "undefined" for the block with id: ${block.header.id.toString('hex')}`, ); } + const { validatorsHash } = await this._bft.method.getBFTParameters( methodContext, block.header.height + 1, ); - if (!block.header.validatorsHash.equals(validatorsHash)) { throw new Error( `Invalid validatorsHash ${block.header.validatorsHash?.toString( diff --git a/framework/test/unit/engine/consensus/consensus.spec.ts b/framework/test/unit/engine/consensus/consensus.spec.ts index f776314f0af..53350492fa6 100644 --- a/framework/test/unit/engine/consensus/consensus.spec.ts +++ b/framework/test/unit/engine/consensus/consensus.spec.ts @@ -38,7 +38,6 @@ import { } from '../../../fixtures'; import * as forkchoice from '../../../../src/engine/consensus/fork_choice/fork_choice_rule'; import { postBlockEventSchema } from '../../../../src/engine/consensus/schema'; -import { fakeLogger } from '../../../utils/mocks'; import { BFTModule } from '../../../../src/engine/bft'; import { ABI, TransactionExecutionResult, TransactionVerifyResult } from '../../../../src/abi'; import { @@ -267,7 +266,7 @@ describe('consensus', () => { expect(network.applyPenaltyOnPeer).not.toHaveBeenCalled(); }); - it('should apply when data is not Buffer', async () => { + it('should apply penalty when data is not Buffer', async () => { await consensus.onBlockReceive('not buffer', peerID); expect(network.applyPenaltyOnPeer).toHaveBeenCalledTimes(1); }); @@ -597,12 +596,7 @@ describe('consensus', () => { let stateStore: StateStore; beforeEach(async () => { - await consensus.init({ - logger: loggerMock, - db: dbMock, - genesisBlock: genesis, - legacyDB: dbMock, - }); + await initConsensus(); jest.spyOn(abi, 'verifyTransaction').mockResolvedValue({ result: TransactionVerifyResult.OK, errorMessage: '', @@ -725,12 +719,7 @@ describe('consensus', () => { consensus['_db'] = dbMock; consensus['_verifyEventRoot'] = jest.fn().mockReturnValue(undefined); - await consensus.init({ - db: dbMock, - genesisBlock: genesis, - logger: fakeLogger, - legacyDB: dbMock, - }); + await initConsensus(); jest.spyOn(consensus['_commitPool'], 'verifyAggregateCommit'); }); @@ -777,14 +766,14 @@ describe('consensus', () => { const invalidBlock = { ...block }; (invalidBlock.header as any).height = consensus['_chain'].lastBlock.header.height; - expect(() => consensus['_verifyAssetsHeight'](invalidBlock as any)).toThrow( + expect(() => consensus['_verifyBlockHeight'](invalidBlock as any)).toThrow( `Invalid height ${ invalidBlock.header.height } of the block with id: ${invalidBlock.header.id.toString('hex')}`, ); }); it('should be success when block has [height===lastBlock.height+1]', () => { - expect(consensus['_verifyAssetsHeight'](block as any)).toBeUndefined(); + expect(consensus['_verifyBlockHeight'](block as any)).toBeUndefined(); }); }); @@ -902,7 +891,7 @@ describe('consensus', () => { } as never); await expect( - consensus['_verifyAssetsSignature'](stateStore, block as any), + consensus['_verifyBlockSignature'](stateStore, block as any), ).rejects.toThrow( `Invalid signature ${block.header.signature.toString( 'hex', @@ -932,7 +921,7 @@ describe('consensus', () => { } as never); await expect( - consensus['_verifyAssetsSignature'](stateStore, validBlock as any), + consensus['_verifyBlockSignature'](stateStore, validBlock as any), ).resolves.toBeUndefined(); }); }); @@ -1047,4 +1036,44 @@ describe('consensus', () => { }); }); }); + + describe('when a block is invalid', () => { + const invalidBlockID = Buffer.from('invalid'); + let invalidBlock: Block; + + beforeEach(async () => { + await initConsensus(); + invalidBlock = await createValidDefaultBlock({ + header: { + height: 2, + previousBlockID: invalidBlockID, + timestamp: consensus['_chain'].lastBlock.header.timestamp + 10, + }, + }); + }); + + it('should throw ApplyPenaltyError from _executeValidated()', async () => { + const errorMessage = `Error: Invalid previousBlockID ${invalidBlockID.toString( + 'hex', + )} of the block with id: ${invalidBlock.header.id.toString('hex')}`; + + await expect(consensus['_executeValidated'](invalidBlock)).rejects.toThrow( + new ApplyPenaltyError(errorMessage), + ); + }); + + it('should throw ApplyPenaltyError from _execute() when fork status is VALID_BLOCK or TIE_BREAK', async () => { + jest.spyOn(chain, 'validateBlock').mockImplementation(() => { + throw new Error(); + }); + + const peerID = 'peer'; + + jest.spyOn(forkchoice, 'forkChoice').mockReturnValue(forkchoice.ForkStatus.VALID_BLOCK); + await expect(consensus['_execute'](invalidBlock, peerID)).rejects.toThrow(ApplyPenaltyError); + + jest.spyOn(forkchoice, 'forkChoice').mockReturnValue(forkchoice.ForkStatus.TIE_BREAK); + await expect(consensus['_execute'](invalidBlock, peerID)).rejects.toThrow(ApplyPenaltyError); + }); + }); });