Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Commit

Permalink
Ban peers after failed block verification (#8445)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@bobanm
bobanm authored May 15, 2023
1 parent 43b04d6 commit b77cba1
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 47 deletions.
68 changes: 39 additions & 29 deletions framework/src/engine/consensus/consensus.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -441,13 +441,15 @@ export class Consensus {
});
return;
}

if (forkStatus === ForkStatus.IDENTICAL_BLOCK) {
this._logger.debug(
{ id: block.header.id, height: block.header.height },
'Block already processed',
);
return;
}

if (forkStatus === ForkStatus.DOUBLE_FORGING) {
this._logger.warn(
{
Expand All @@ -472,6 +474,7 @@ export class Consensus {
});
return;
}

// Discard block and move to different chain
if (forkStatus === ForkStatus.DIFFERENT_CHAIN) {
this._logger.debug(
Expand All @@ -496,6 +499,7 @@ export class Consensus {
await this._sync(block, peerID);
return;
}

// Replacing a block
if (forkStatus === ForkStatus.TIE_BREAK) {
this._logger.info(
Expand All @@ -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 {
Expand All @@ -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
Expand All @@ -570,8 +583,14 @@ export class Consensus {
} = {},
): Promise<Block> {
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({
Expand Down Expand Up @@ -643,25 +662,12 @@ export class Consensus {
private async _verify(block: Block): Promise<void> {
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);
}

Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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',
Expand All @@ -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,
Expand All @@ -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<void> {
private async _verifyBlockSignature(stateStore: StateStore, block: Block): Promise<void> {
const bftParams = await this._bft.method.getBFTParameters(stateStore, block.header.height);
const generator = bftParams.validators.find(validator =>
validator.address.equals(block.header.generatorAddress),
Expand Down Expand Up @@ -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,
Expand All @@ -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(
Expand Down
65 changes: 47 additions & 18 deletions framework/test/unit/engine/consensus/consensus.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
});
Expand Down Expand Up @@ -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: '',
Expand Down Expand Up @@ -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');
});

Expand Down Expand Up @@ -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();
});
});

Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -932,7 +921,7 @@ describe('consensus', () => {
} as never);

await expect(
consensus['_verifyAssetsSignature'](stateStore, validBlock as any),
consensus['_verifyBlockSignature'](stateStore, validBlock as any),
).resolves.toBeUndefined();
});
});
Expand Down Expand Up @@ -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);
});
});
});

0 comments on commit b77cba1

Please sign in to comment.