From 035a4a8257e7520053b03e2a1fc263e2c0acaffd Mon Sep 17 00:00:00 2001 From: Sebastijan K <58827427+sebastijankuzner@users.noreply.github.com> Date: Mon, 26 Feb 2024 16:55:36 +0100 Subject: [PATCH] refactor(processor): log verification errors (#452) * Remove console logs * Remove forger errors * Remove UnexpectedError * Remove extra exceptions * Add ValidatorExceptions * Handler throws errors * Improve logs * style: resolve style guide violations --------- Co-authored-by: sebastijankuzner --- packages/api-http/test/helpers/request.ts | 2 - packages/bootstrap/source/bootstrapper.ts | 2 +- .../source/generators/genesis-block.ts | 1 - .../contracts/source/contracts/processor.ts | 4 +- packages/contracts/source/exceptions/base.ts | 13 ------ packages/contracts/source/exceptions/cache.ts | 5 --- .../contracts/source/exceptions/forger.ts | 13 ------ packages/contracts/source/exceptions/index.ts | 3 +- .../contracts/source/exceptions/processor.ts | 42 +++++++++++++++++++ .../contracts/source/exceptions/runtime.ts | 8 ---- packages/processor/source/block-processor.ts | 7 +--- packages/processor/source/block-verifier.ts | 28 ++++--------- .../source/verifiers/chained-verifier.ts | 10 +++-- .../source/verifiers/generator-verifier.ts | 10 +++-- .../incompatible-transactions-verifier.ts | 8 ++-- .../source/verifiers/nonce-verifier.ts | 17 ++------ .../source/verifiers/timestamp-verifier.ts | 10 ++--- .../source/verifiers/verify-block-verifier.ts | 21 ++-------- 18 files changed, 81 insertions(+), 123 deletions(-) delete mode 100644 packages/contracts/source/exceptions/cache.ts delete mode 100644 packages/contracts/source/exceptions/forger.ts create mode 100644 packages/contracts/source/exceptions/processor.ts diff --git a/packages/api-http/test/helpers/request.ts b/packages/api-http/test/helpers/request.ts index b783b98f5..5fa706ce3 100644 --- a/packages/api-http/test/helpers/request.ts +++ b/packages/api-http/test/helpers/request.ts @@ -13,8 +13,6 @@ export const request = async >( } const response = await got(`http://localhost:4003/api/${path}${transform}`); - // console.log(response); - const { statusCode, headers, body } = response; return { statusCode, headers, data: JSON.parse(body) as T }; }; diff --git a/packages/bootstrap/source/bootstrapper.ts b/packages/bootstrap/source/bootstrapper.ts index c5c56c836..4067fd8f5 100644 --- a/packages/bootstrap/source/bootstrapper.ts +++ b/packages/bootstrap/source/bootstrapper.ts @@ -172,7 +172,7 @@ export class Bootstrapper { const commitState = this.commitStateFactory(commit); const result = await this.blockProcessor.process(commitState); if (result === false) { - throw new Error(`Cannot process block`); + throw new Error(`Block is not processed.`); } await this.blockProcessor.commit(commitState); } catch (error) { diff --git a/packages/configuration-generator/source/generators/genesis-block.ts b/packages/configuration-generator/source/generators/genesis-block.ts index 7f1bf1394..595664a13 100644 --- a/packages/configuration-generator/source/generators/genesis-block.ts +++ b/packages/configuration-generator/source/generators/genesis-block.ts @@ -247,7 +247,6 @@ export class GenesisBlockGenerator extends Generator { const verified = await this.blockVerifier.verify(genesis.block); if (!verified.verified) { - console.log(verified); throw new Error("failed to generate genesis block"); } } diff --git a/packages/contracts/source/contracts/processor.ts b/packages/contracts/source/contracts/processor.ts index 5d8a44b61..d91e460d6 100644 --- a/packages/contracts/source/contracts/processor.ts +++ b/packages/contracts/source/contracts/processor.ts @@ -15,7 +15,7 @@ export interface ProcessableUnit { } export interface Handler { - execute(unit: ProcessableUnit): Promise; + execute(unit: ProcessableUnit): Promise; } export interface BlockProcessor { @@ -28,5 +28,5 @@ export interface TransactionProcessor { } export interface Verifier { - verify(unit: ProcessableUnit): Promise; + verify(unit: ProcessableUnit): Promise; } diff --git a/packages/contracts/source/exceptions/base.ts b/packages/contracts/source/exceptions/base.ts index 4980d52f3..613da43e4 100644 --- a/packages/contracts/source/exceptions/base.ts +++ b/packages/contracts/source/exceptions/base.ts @@ -15,16 +15,3 @@ export class Exception extends Error { Error.captureStackTrace(this, this.constructor); } } - -export class UnexpectedError extends Exception { - public constructor( - public readonly error: Error, - public readonly path: string[], - ) { - super( - path.length > 0 - ? `Unexpected error '${error.message}' (${error.constructor.name}) at '${path.join(".")}'` - : `Unexpected error '${error.message}' (${error.constructor.name})`, - ); - } -} diff --git a/packages/contracts/source/exceptions/cache.ts b/packages/contracts/source/exceptions/cache.ts deleted file mode 100644 index f440afec0..000000000 --- a/packages/contracts/source/exceptions/cache.ts +++ /dev/null @@ -1,5 +0,0 @@ -import { Exception } from "./base"; - -export class CacheException extends Exception {} - -export class InvalidArgument extends CacheException {} diff --git a/packages/contracts/source/exceptions/forger.ts b/packages/contracts/source/exceptions/forger.ts deleted file mode 100644 index 6cb837aee..000000000 --- a/packages/contracts/source/exceptions/forger.ts +++ /dev/null @@ -1,13 +0,0 @@ -import { Exception } from "./base"; - -export class RelayCommunicationError extends Exception { - public constructor(endpoint: string, message: string) { - super(`Request to ${endpoint} failed, because of '${message}'.`); - } -} - -export class HostNoResponseError extends Exception { - public constructor(host: string) { - super(`${host} didn't respond. Trying again later.`); - } -} diff --git a/packages/contracts/source/exceptions/index.ts b/packages/contracts/source/exceptions/index.ts index e63e21772..85f77582c 100644 --- a/packages/contracts/source/exceptions/index.ts +++ b/packages/contracts/source/exceptions/index.ts @@ -1,16 +1,15 @@ export * from "./base"; -export * from "./cache"; export * from "./cli"; export * from "./config"; export * from "./consensus"; export * from "./container"; export * from "./crypto"; export * from "./filesystem"; -export * from "./forger"; export * from "./logic"; export * from "./p2p"; export * from "./plugins"; export * from "./pool"; +export * from "./processor"; export * from "./runtime"; export * from "./state"; export * from "./validation"; diff --git a/packages/contracts/source/exceptions/processor.ts b/packages/contracts/source/exceptions/processor.ts new file mode 100644 index 000000000..5cfc8d1dd --- /dev/null +++ b/packages/contracts/source/exceptions/processor.ts @@ -0,0 +1,42 @@ +import { Block } from "../contracts/crypto"; +import { Exception } from "./base"; + +export class ValidatorException extends Exception {} + +export class BlockNotChained extends ValidatorException { + public constructor(block: Block) { + super(`Block ${block.data.id} is not chained.`); + } +} + +export class BlockNotVerified extends ValidatorException { + public constructor(block: Block, reason: string) { + super(`Block ${block.data.id} is not verified, because: ${reason}.`); + } +} + +export class InvalidTimestamp extends ValidatorException { + public constructor(block: Block) { + super(`Block ${block.data.id} timestamp is too low.`); + } +} + +export class InvalidGenerator extends ValidatorException { + public constructor(block: Block, expectedValidator: string) { + super( + `Block ${block.data.id} has invalid generator. Block generator is ${block.data.generatorPublicKey} instead ${expectedValidator}.`, + ); + } +} + +export class IncompatibleTransactions extends ValidatorException { + public constructor(block: Block) { + super(`Block ${block.data.id} contains incompatible transaction.`); + } +} + +export class InvalidNonce extends ValidatorException { + public constructor(block: Block, sender: string) { + super(`Block ${block.data.id} contains invalid nonce for sender ${sender}.`); + } +} diff --git a/packages/contracts/source/exceptions/runtime.ts b/packages/contracts/source/exceptions/runtime.ts index f45cd7ae1..2c6d38d78 100644 --- a/packages/contracts/source/exceptions/runtime.ts +++ b/packages/contracts/source/exceptions/runtime.ts @@ -2,14 +2,6 @@ import { Exception } from "./base"; export class RuntimeException extends Exception {} -export class OverflowException extends RuntimeException {} - -export class RangeException extends RuntimeException {} - -export class UnderflowException extends RuntimeException {} - -export class UnexpectedValueException extends RuntimeException {} - export class NotImplemented extends RuntimeException { public constructor(method: string, klass: string) { super(`Method [${method}] is not implemented in [${klass}].`); diff --git a/packages/processor/source/block-processor.ts b/packages/processor/source/block-processor.ts index 30b6b1565..e7fd07687 100644 --- a/packages/processor/source/block-processor.ts +++ b/packages/processor/source/block-processor.ts @@ -53,9 +53,7 @@ export class BlockProcessor implements Contracts.Processor.BlockProcessor { public async process(unit: Contracts.Processor.ProcessableUnit): Promise { try { - if (!(await this.verifier.verify(unit))) { - return false; - } + await this.verifier.verify(unit); for (const transaction of unit.getBlock().transactions) { await this.transactionProcessor.process(unit.store.walletRepository, transaction); @@ -65,8 +63,7 @@ export class BlockProcessor implements Contracts.Processor.BlockProcessor { return true; } catch (error) { - console.log(error); - this.logger.error(`Cannot process block, because: ${error.message}`); + this.logger.error(`Cannot process block because: ${error.message}`); } return false; diff --git a/packages/processor/source/block-verifier.ts b/packages/processor/source/block-verifier.ts index 09e17e465..9d5178ffb 100644 --- a/packages/processor/source/block-verifier.ts +++ b/packages/processor/source/block-verifier.ts @@ -15,31 +15,17 @@ export class BlockVerifier implements Contracts.Processor.Verifier { @inject(Identifiers.Application.Instance) protected readonly app!: Contracts.Kernel.Application; - public async verify(unit: Contracts.Processor.ProcessableUnit): Promise { - if (!(await this.app.resolve(ChainedVerifier).execute(unit))) { - return false; - } + public async verify(unit: Contracts.Processor.ProcessableUnit): Promise { + await this.app.resolve(ChainedVerifier).execute(unit); - if (!(await this.app.resolve(TimestampVerifier).execute(unit))) { - return false; - } + await this.app.resolve(TimestampVerifier).execute(unit); - if (!(await this.app.resolve(GeneratorVerifier).execute(unit))) { - return false; - } + await this.app.resolve(GeneratorVerifier).execute(unit); - if (!(await this.app.resolve(VerifyBlockVerifier).execute(unit))) { - return false; - } + await this.app.resolve(VerifyBlockVerifier).execute(unit); - if (!(await this.app.resolve(IncompatibleTransactionsVerifier).execute(unit))) { - return false; - } + await this.app.resolve(IncompatibleTransactionsVerifier).execute(unit); - if (!(await this.app.resolve(NonceVerifier).execute(unit))) { - return false; - } - - return true; + await this.app.resolve(NonceVerifier).execute(unit); } } diff --git a/packages/processor/source/verifiers/chained-verifier.ts b/packages/processor/source/verifiers/chained-verifier.ts index 1ee60fa54..cd2873333 100644 --- a/packages/processor/source/verifiers/chained-verifier.ts +++ b/packages/processor/source/verifiers/chained-verifier.ts @@ -1,5 +1,5 @@ import { inject, injectable } from "@mainsail/container"; -import { Contracts, Identifiers } from "@mainsail/contracts"; +import { Contracts, Exceptions, Identifiers } from "@mainsail/contracts"; import { Utils } from "@mainsail/kernel"; @injectable() @@ -10,11 +10,13 @@ export class ChainedVerifier implements Contracts.Processor.Handler { @inject(Identifiers.State.Service) private readonly stateService!: Contracts.State.Service; - public async execute(unit: Contracts.Processor.ProcessableUnit): Promise { + public async execute(unit: Contracts.Processor.ProcessableUnit): Promise { if (unit.getBlock().data.height === 0) { - return true; + return; } - return Utils.isBlockChained(this.stateService.getStore().getLastBlock().data, unit.getBlock().data); + if (!Utils.isBlockChained(this.stateService.getStore().getLastBlock().data, unit.getBlock().data)) { + throw new Exceptions.BlockNotChained(unit.getBlock()); + } } } diff --git a/packages/processor/source/verifiers/generator-verifier.ts b/packages/processor/source/verifiers/generator-verifier.ts index 567e605bf..ddf1f3bf2 100644 --- a/packages/processor/source/verifiers/generator-verifier.ts +++ b/packages/processor/source/verifiers/generator-verifier.ts @@ -1,5 +1,5 @@ import { inject, injectable } from "@mainsail/container"; -import { Contracts, Identifiers } from "@mainsail/contracts"; +import { Contracts, Exceptions, Identifiers } from "@mainsail/contracts"; @injectable() export class GeneratorVerifier implements Contracts.Processor.Handler { @@ -12,14 +12,16 @@ export class GeneratorVerifier implements Contracts.Processor.Handler { @inject(Identifiers.ValidatorSet.Service) private readonly validatorSet!: Contracts.ValidatorSet.Service; - public async execute(unit: Contracts.Processor.ProcessableUnit): Promise { + public async execute(unit: Contracts.Processor.ProcessableUnit): Promise { if (unit.getBlock().data.height === 0) { - return true; + return; } const validatorIndex = this.proposerSelector.getValidatorIndex(unit.getBlock().data.round); const validator = this.validatorSet.getValidator(validatorIndex); - return unit.getBlock().data.generatorPublicKey === validator.getWalletPublicKey(); + if (unit.getBlock().data.generatorPublicKey !== validator.getWalletPublicKey()) { + throw new Exceptions.InvalidGenerator(unit.getBlock(), validator.getWalletPublicKey()); + } } } diff --git a/packages/processor/source/verifiers/incompatible-transactions-verifier.ts b/packages/processor/source/verifiers/incompatible-transactions-verifier.ts index d404181b8..35575d8f9 100644 --- a/packages/processor/source/verifiers/incompatible-transactions-verifier.ts +++ b/packages/processor/source/verifiers/incompatible-transactions-verifier.ts @@ -1,17 +1,15 @@ import { injectable } from "@mainsail/container"; -import { Contracts } from "@mainsail/contracts"; +import { Contracts, Exceptions } from "@mainsail/contracts"; @injectable() export class IncompatibleTransactionsVerifier implements Contracts.Processor.Handler { - public async execute(unit: Contracts.Processor.ProcessableUnit): Promise { + public async execute(unit: Contracts.Processor.ProcessableUnit): Promise { const block = unit.getBlock(); for (let index = 1; index < block.transactions.length; index++) { if (block.transactions[index].data.version !== block.transactions[0].data.version) { - return false; + throw new Exceptions.IncompatibleTransactions(block); } } - - return true; } } diff --git a/packages/processor/source/verifiers/nonce-verifier.ts b/packages/processor/source/verifiers/nonce-verifier.ts index a5c2a64a5..661520a68 100644 --- a/packages/processor/source/verifiers/nonce-verifier.ts +++ b/packages/processor/source/verifiers/nonce-verifier.ts @@ -1,5 +1,5 @@ import { inject, injectable } from "@mainsail/container"; -import { Contracts, Identifiers } from "@mainsail/contracts"; +import { Contracts, Exceptions, Identifiers } from "@mainsail/contracts"; import { Utils } from "@mainsail/kernel"; import { BigNumber } from "@mainsail/utils"; @@ -8,10 +8,7 @@ export class NonceVerifier implements Contracts.Processor.Handler { @inject(Identifiers.Application.Instance) protected readonly app!: Contracts.Kernel.Application; - @inject(Identifiers.Services.Log.Service) - private readonly logger!: Contracts.Kernel.Logger; - - public async execute(unit: Contracts.Processor.ProcessableUnit): Promise { + public async execute(unit: Contracts.Processor.ProcessableUnit): Promise { const block = unit.getBlock(); const nonceBySender = {}; @@ -33,18 +30,10 @@ export class NonceVerifier implements Contracts.Processor.Handler { const nonce: BigNumber = BigNumber.make(data.nonce); if (!nonceBySender[sender].plus(1).isEqualTo(nonce)) { - this.logger.warning( - `Block { height: ${block.data.height.toLocaleString()}, id: ${block.data.id} } ` + - `not accepted: invalid nonce order for sender ${sender}: ` + - `preceding nonce: ${nonceBySender[sender].toFixed(0)}, ` + - `transaction ${data.id} has nonce ${nonce.toFixed()}.`, - ); - return false; + throw new Exceptions.InvalidNonce(block, sender); } nonceBySender[sender] = nonce; } - - return true; } } diff --git a/packages/processor/source/verifiers/timestamp-verifier.ts b/packages/processor/source/verifiers/timestamp-verifier.ts index e43cdbd85..f8009cd48 100644 --- a/packages/processor/source/verifiers/timestamp-verifier.ts +++ b/packages/processor/source/verifiers/timestamp-verifier.ts @@ -1,5 +1,5 @@ import { inject, injectable } from "@mainsail/container"; -import { Contracts, Identifiers } from "@mainsail/contracts"; +import { Contracts, Exceptions, Identifiers } from "@mainsail/contracts"; import { Utils } from "@mainsail/kernel"; @injectable() @@ -16,9 +16,9 @@ export class TimestampVerifier implements Contracts.Processor.Handler { @inject(Identifiers.Services.Log.Service) private readonly logger!: Contracts.Kernel.Logger; - public async execute(unit: Contracts.Processor.ProcessableUnit): Promise { + public async execute(unit: Contracts.Processor.ProcessableUnit): Promise { if (unit.getBlock().data.height === 0) { - return true; + return; } if ( @@ -33,9 +33,7 @@ export class TimestampVerifier implements Contracts.Processor.Handler { `Block ${unit.getBlock().data.height.toLocaleString()} disregarded, because it's timestamp is too low`, ); - return false; + throw new Exceptions.InvalidTimestamp(unit.getBlock()); } - - return true; } } diff --git a/packages/processor/source/verifiers/verify-block-verifier.ts b/packages/processor/source/verifiers/verify-block-verifier.ts index 294713b67..b8f4a46a5 100644 --- a/packages/processor/source/verifiers/verify-block-verifier.ts +++ b/packages/processor/source/verifiers/verify-block-verifier.ts @@ -1,5 +1,5 @@ import { inject, injectable } from "@mainsail/container"; -import { Contracts, Identifiers } from "@mainsail/contracts"; +import { Contracts, Exceptions, Identifiers } from "@mainsail/contracts"; @injectable() export class VerifyBlockVerifier implements Contracts.Processor.Handler { @@ -12,10 +12,7 @@ export class VerifyBlockVerifier implements Contracts.Processor.Handler { @inject(Identifiers.Transaction.Handler.Registry) private readonly handlerRegistry!: Contracts.Transactions.TransactionHandlerRegistry; - @inject(Identifiers.Services.Log.Service) - private readonly logger!: Contracts.Kernel.Logger; - - public async execute(unit: Contracts.Processor.ProcessableUnit): Promise { + public async execute(unit: Contracts.Processor.ProcessableUnit): Promise { const block = unit.getBlock(); let verification: Contracts.Crypto.BlockVerification = await this.blockVerifier.verify(block); @@ -30,22 +27,12 @@ export class VerifyBlockVerifier implements Contracts.Processor.Handler { // @TODO: check if we can remove this duplicate verification verification = await this.blockVerifier.verify(block); } catch (error) { - this.logger.warning(`Failed to verify block, because: ${error.message}`); + throw new Exceptions.BlockNotVerified(block, error.message); } } if (!verification.verified) { - this.logger.warning( - `Block ${block.data.height.toLocaleString()} (${ - block.data.id - }) disregarded because verification failed`, - ); - - this.logger.warning(JSON.stringify(verification, undefined, 4)); - - return false; + throw new Exceptions.BlockNotVerified(block, verification.errors.join(", ")); } - - return true; } }