From 5584de59b0020cae8addc85049300433318637c6 Mon Sep 17 00:00:00 2001 From: Scott Sadler Date: Fri, 3 Mar 2017 11:36:50 +0100 Subject: [PATCH 1/7] Make ValidationError a superclass of all validation errors and use it --- bigchaindb/common/exceptions.py | 106 +++++++++++++------------ bigchaindb/core.py | 32 +------- bigchaindb/models.py | 71 +++++------------ bigchaindb/pipelines/block.py | 14 ++-- bigchaindb/pipelines/vote.py | 10 ++- bigchaindb/web/views/transactions.py | 27 +------ tests/assets/test_digital_assets.py | 5 +- tests/db/test_bigchain_api.py | 21 ++--- tests/pipelines/test_block_creation.py | 27 +++---- tests/pipelines/test_vote.py | 15 +++- tests/test_models.py | 8 +- tests/web/test_transactions.py | 10 +-- 12 files changed, 140 insertions(+), 206 deletions(-) diff --git a/bigchaindb/common/exceptions.py b/bigchaindb/common/exceptions.py index 60340492..4b95f84b 100644 --- a/bigchaindb/common/exceptions.py +++ b/bigchaindb/common/exceptions.py @@ -7,40 +7,6 @@ class ConfigurationError(BigchainDBError): """Raised when there is a problem with server configuration""" -class OperationError(BigchainDBError): - """Raised when an operation cannot go through""" - - -class TransactionDoesNotExist(BigchainDBError): - """Raised if the transaction is not in the database""" - - -class TransactionOwnerError(BigchainDBError): - """Raised if a user tries to transfer a transaction they don't own""" - - -class DoubleSpend(BigchainDBError): - """Raised if a double spend is found""" - - -class ValidationError(BigchainDBError): - """Raised if there was an error in validation""" - - -class InvalidHash(ValidationError): - """Raised if there was an error checking the hash for a particular - operation""" - - -class SchemaValidationError(ValidationError): - """Raised if there was any error validating an object's schema""" - - -class InvalidSignature(BigchainDBError): - """Raised if there was an error checking the signature for a particular - operation""" - - class DatabaseAlreadyExists(BigchainDBError): """Raised when trying to create the database but the db is already there""" @@ -49,6 +15,18 @@ class DatabaseDoesNotExist(BigchainDBError): """Raised when trying to delete the database but the db is not there""" +class StartupError(BigchainDBError): + """Raised when there is an error starting up the system""" + + +class GenesisBlockAlreadyExistsError(BigchainDBError): + """Raised when trying to create the already existing genesis block""" + + +class CyclicBlockchainError(BigchainDBError): + """Raised when there is a cycle in the blockchain""" + + class KeypairNotFoundException(BigchainDBError): """Raised if operation cannot proceed because the keypair was not given""" @@ -58,34 +36,64 @@ class KeypairMismatchException(BigchainDBError): current owner(s)""" -class StartupError(BigchainDBError): - """Raised when there is an error starting up the system""" +class OperationError(BigchainDBError): + """Raised when an operation cannot go through""" -class ImproperVoteError(BigchainDBError): +################################################################################ +# Validation errors + + +class ValidationError(BigchainDBError): + """Raised if there was an error in validation""" + + +class DoubleSpend(ValidationError): + """Raised if a double spend is found""" + + +class InvalidHash(ValidationError): + """Raised if there was an error checking the hash for a particular + operation""" + + +class SchemaValidationError(ValidationError): + """Raised if there was any error validating an object's schema""" + + +class InvalidSignature(ValidationError): + """Raised if there was an error checking the signature for a particular + operation""" + + +class ImproperVoteError(ValidationError): """Raised if a vote is not constructed correctly, or signed incorrectly""" -class MultipleVotesError(BigchainDBError): +class MultipleVotesError(ValidationError): """Raised if a voter has voted more than once""" -class GenesisBlockAlreadyExistsError(BigchainDBError): - """Raised when trying to create the already existing genesis block""" - - -class CyclicBlockchainError(BigchainDBError): - """Raised when there is a cycle in the blockchain""" - - -class TransactionNotInValidBlock(BigchainDBError): +class TransactionNotInValidBlock(ValidationError): """Raised when a transfer transaction is attempting to fulfill the outputs of a transaction that is in an invalid or undecided block""" -class AssetIdMismatch(BigchainDBError): +class AssetIdMismatch(ValidationError): """Raised when multiple transaction inputs related to different assets""" -class AmountError(BigchainDBError): +class AmountError(ValidationError): """Raised when there is a problem with a transaction's output amounts""" + + +class TransactionDoesNotExist(ValidationError): + """Raised if the transaction is not in the database""" + + +class TransactionOwnerError(ValidationError): + """Raised if a user tries to transfer a transaction they don't own""" + + +class SybilError(ValidationError): + """If a block or vote comes from an unidentifiable node""" diff --git a/bigchaindb/core.py b/bigchaindb/core.py index 9f93d47a..084df928 100644 --- a/bigchaindb/core.py +++ b/bigchaindb/core.py @@ -162,31 +162,6 @@ class Bigchain(object): return self.consensus.validate_transaction(self, transaction) - def is_valid_transaction(self, transaction): - """Check whether a transaction is valid or invalid. - - Similar to :meth:`~bigchaindb.Bigchain.validate_transaction` - but never raises an exception. It returns :obj:`False` if - the transaction is invalid. - - Args: - transaction (:Class:`~bigchaindb.models.Transaction`): transaction - to check. - - Returns: - The :class:`~bigchaindb.models.Transaction` instance if valid, - otherwise :obj:`False`. - """ - - try: - return self.validate_transaction(transaction) - except (ValueError, exceptions.OperationError, - exceptions.TransactionDoesNotExist, - exceptions.TransactionOwnerError, exceptions.DoubleSpend, - exceptions.InvalidHash, exceptions.InvalidSignature, - exceptions.TransactionNotInValidBlock, exceptions.AmountError): - return False - def is_new_transaction(self, txid, exclude_block_id=None): """ Return True if the transaction does not exist in any @@ -386,10 +361,9 @@ class Bigchain(object): if self.get_transaction(transaction['id']): num_valid_transactions += 1 if num_valid_transactions > 1: - raise exceptions.DoubleSpend(('`{}` was spent more than' - ' once. There is a problem' - ' with the chain') - .format(txid)) + raise exceptions.BigchainDBCritical( + '`{}` was spent more than once. There is a problem' + ' with the chain'.format(txid)) if num_valid_transactions: return Transaction.from_dict(transactions[0]) diff --git a/bigchaindb/models.py b/bigchaindb/models.py index ee7efe8f..fd71f98d 100644 --- a/bigchaindb/models.py +++ b/bigchaindb/models.py @@ -1,9 +1,9 @@ from bigchaindb.common.crypto import hash_data, PublicKey, PrivateKey from bigchaindb.common.exceptions import (InvalidHash, InvalidSignature, - OperationError, DoubleSpend, - TransactionDoesNotExist, + DoubleSpend, TransactionDoesNotExist, TransactionNotInValidBlock, - AssetIdMismatch, AmountError) + AssetIdMismatch, AmountError, + SybilError, ValidationError) from bigchaindb.common.transaction import Transaction from bigchaindb.common.utils import gen_timestamp, serialize from bigchaindb.common.schema import validate_transaction_schema @@ -22,19 +22,10 @@ class Transaction(Transaction): invalid. Raises: - OperationError: if the transaction operation is not supported - TransactionDoesNotExist: if the input of the transaction is not - found - TransactionNotInValidBlock: if the input of the transaction is not - in a valid block - TransactionOwnerError: if the new transaction is using an input it - doesn't own - DoubleSpend: if the transaction is a double spend - InvalidHash: if the hash of the transaction is wrong - InvalidSignature: if the signature of the transaction is wrong + ValidationError: If the transaction is invalid """ if len(self.inputs) == 0: - raise ValueError('Transaction contains no inputs') + raise ValidationError('Transaction contains no inputs') input_conditions = [] inputs_defined = all([input_.fulfills for input_ in self.inputs]) @@ -46,20 +37,20 @@ class Transaction(Transaction): if self.operation in (Transaction.CREATE, Transaction.GENESIS): # validate asset if self.asset['data'] is not None and not isinstance(self.asset['data'], dict): - raise TypeError(('`asset.data` must be a dict instance or ' - 'None for `CREATE` transactions')) + raise ValidationError(('`asset.data` must be a dict instance or ' + 'None for `CREATE` transactions')) # validate inputs if inputs_defined: - raise ValueError('A CREATE operation has no inputs') + raise ValidationError('A CREATE operation has no inputs') elif self.operation == Transaction.TRANSFER: # validate asset if not isinstance(self.asset['id'], str): - raise ValueError(('`asset.id` must be a string for ' - '`TRANSFER` transations')) + raise ValidationError('`asset.id` must be a string for ' + '`TRANSFER` transations') # check inputs if not inputs_defined: - raise ValueError('Only `CREATE` transactions can have null ' - 'inputs') + raise ValidationError('Only `CREATE` transactions can have ' + 'null inputs') # store the inputs so that we can check if the asset ids match input_txs = [] @@ -116,8 +107,8 @@ class Transaction(Transaction): else: allowed_operations = ', '.join(Transaction.ALLOWED_OPERATIONS) - raise TypeError('`operation`: `{}` must be either {}.' - .format(self.operation, allowed_operations)) + raise ValidationError('`operation`: `{}` must be either {}.' + .format(self.operation, allowed_operations)) if not self.inputs_valid(input_conditions): raise InvalidSignature('Transaction signature is invalid.') @@ -205,18 +196,8 @@ class Block(object): raised. Raises: - OperationError: If a non-federation node signed the Block. - InvalidSignature: If a Block's signature is invalid or if the - block contains a transaction with an invalid signature. - OperationError: if the transaction operation is not supported - TransactionDoesNotExist: if the input of the transaction is not - found - TransactionNotInValidBlock: if the input of the transaction is not - in a valid block - TransactionOwnerError: if the new transaction is using an input it - doesn't own - DoubleSpend: if the transaction is a double spend - InvalidHash: if the hash of the transaction is wrong + ValidationError: If the block or any transaction in the block does + not validate """ self._validate_block(bigchain) @@ -232,13 +213,12 @@ class Block(object): object. Raises: - OperationError: If a non-federation node signed the Block. - InvalidSignature: If a Block's signature is invalid. + ValidationError: If there is a problem with the block """ # Check if the block was created by a federation node possible_voters = (bigchain.nodes_except_me + [bigchain.me]) if self.node_pubkey not in possible_voters: - raise OperationError('Only federation nodes can create blocks') + raise SybilError('Only federation nodes can create blocks') # Check that the signature is valid if not self.is_signature_valid(): @@ -251,16 +231,7 @@ class Block(object): bigchain (Bigchain): an instantiated bigchaindb.Bigchain object. Raises: - OperationError: if the transaction operation is not supported - TransactionDoesNotExist: if the input of the transaction is not - found - TransactionNotInValidBlock: if the input of the transaction is not - in a valid block - TransactionOwnerError: if the new transaction is using an input it - doesn't own - DoubleSpend: if the transaction is a double spend - InvalidHash: if the hash of the transaction is wrong - InvalidSignature: if the signature of the transaction is wrong + ValidationError: If an invalid transaction is found """ for tx in self.transactions: # If a transaction is not valid, `validate_transactions` will @@ -341,10 +312,10 @@ class Block(object): dict: The Block as a dict. Raises: - OperationError: If the Block doesn't contain any transactions. + ValueError: If the Block doesn't contain any transactions. """ if len(self.transactions) == 0: - raise OperationError('Empty block creation is not allowed') + raise ValueError('Empty block creation is not allowed') block = { 'timestamp': self.timestamp, diff --git a/bigchaindb/pipelines/block.py b/bigchaindb/pipelines/block.py index 1f2e9017..2a43686a 100644 --- a/bigchaindb/pipelines/block.py +++ b/bigchaindb/pipelines/block.py @@ -13,8 +13,7 @@ import bigchaindb from bigchaindb import backend from bigchaindb.backend.changefeed import ChangeFeed from bigchaindb.models import Transaction -from bigchaindb.common.exceptions import (SchemaValidationError, InvalidHash, - InvalidSignature, AmountError) +from bigchaindb.common.exceptions import ValidationError from bigchaindb import Bigchain @@ -63,8 +62,7 @@ class BlockPipeline: """ try: tx = Transaction.from_dict(tx) - except (SchemaValidationError, InvalidHash, InvalidSignature, - AmountError): + except ValidationError: return None # If transaction is in any VALID or UNDECIDED block we @@ -74,12 +72,14 @@ class BlockPipeline: return None # If transaction is not valid it should not be included - if not self.bigchain.is_valid_transaction(tx): + try: + tx.validate(self.bigchain) + return tx + except ValidationError as e: + # todo: log self.bigchain.delete_transaction(tx.id) return None - return tx - def create(self, tx, timeout=False): """Create a block. diff --git a/bigchaindb/pipelines/vote.py b/bigchaindb/pipelines/vote.py index da28cb30..088c4eac 100644 --- a/bigchaindb/pipelines/vote.py +++ b/bigchaindb/pipelines/vote.py @@ -61,7 +61,7 @@ class Vote: return block['id'], [self.invalid_dummy_tx] try: block._validate_block(self.bigchain) - except (exceptions.OperationError, exceptions.InvalidSignature): + except exceptions.ValidationError: # XXX: if a block is invalid we should skip the `validate_tx` # step, but since we are in a pipeline we cannot just jump to # another function. Hackish solution: generate an invalid @@ -105,7 +105,13 @@ class Vote: if not new: return False, block_id, num_tx - valid = bool(self.bigchain.is_valid_transaction(tx)) + try: + tx.validate(self.bigchain) + valid = True + except exceptions.ValidationError: + # TODO: log + valid = False + return valid, block_id, num_tx def vote(self, tx_validity, block_id, num_tx): diff --git a/bigchaindb/web/views/transactions.py b/bigchaindb/web/views/transactions.py index 7acaa279..925aed7a 100644 --- a/bigchaindb/web/views/transactions.py +++ b/bigchaindb/web/views/transactions.py @@ -9,20 +9,7 @@ import logging from flask import current_app, request from flask_restful import Resource, reqparse - -from bigchaindb.common.exceptions import ( - AmountError, - DoubleSpend, - InvalidHash, - InvalidSignature, - SchemaValidationError, - OperationError, - TransactionDoesNotExist, - TransactionOwnerError, - TransactionNotInValidBlock, - ValidationError, -) - +from bigchaindb.common.exceptions import SchemaValidationError, ValidationError from bigchaindb.models import Transaction from bigchaindb.web.views.base import make_error from bigchaindb.web.views import parameters @@ -84,7 +71,7 @@ class TransactionListApi(Resource): message='Invalid transaction schema: {}'.format( e.__cause__.message) ) - except (ValidationError, InvalidSignature) as e: + except ValidationError as e: return make_error( 400, 'Invalid transaction ({}): {}'.format(type(e).__name__, e) @@ -93,15 +80,7 @@ class TransactionListApi(Resource): with pool() as bigchain: try: bigchain.validate_transaction(tx_obj) - except (ValueError, - OperationError, - TransactionDoesNotExist, - TransactionOwnerError, - DoubleSpend, - InvalidHash, - InvalidSignature, - TransactionNotInValidBlock, - AmountError) as e: + except ValidationError as e: return make_error( 400, 'Invalid transaction ({}): {}'.format(type(e).__name__, e) diff --git a/tests/assets/test_digital_assets.py b/tests/assets/test_digital_assets.py index 1dc4764f..d44bc52c 100644 --- a/tests/assets/test_digital_assets.py +++ b/tests/assets/test_digital_assets.py @@ -1,3 +1,4 @@ +from bigchaindb.common.exceptions import ValidationError import pytest import random @@ -26,7 +27,7 @@ def test_validate_bad_asset_creation(b, user_pk): tx.asset['data'] = 'a' tx_signed = tx.sign([b.me_private]) - with pytest.raises(TypeError): + with pytest.raises(ValidationError): b.validate_transaction(tx_signed) @@ -108,4 +109,4 @@ def test_create_valid_divisible_asset(b, user_pk, user_sk): tx = Transaction.create([user_pk], [([user_pk], 2)]) tx_signed = tx.sign([user_sk]) - assert b.is_valid_transaction(tx_signed) + tx_signed.validate(b) diff --git a/tests/db/test_bigchain_api.py b/tests/db/test_bigchain_api.py index 96779e60..1f71862a 100644 --- a/tests/db/test_bigchain_api.py +++ b/tests/db/test_bigchain_api.py @@ -3,6 +3,8 @@ from time import sleep import pytest from unittest.mock import patch +from bigchaindb.common.exceptions import ValidationError + pytestmark = pytest.mark.bdb @@ -565,14 +567,14 @@ class TestTransactionValidation(object): # Manipulate input so that it has a `fulfills` defined even # though it shouldn't have one create_tx.inputs[0].fulfills = TransactionLink('abc', 0) - with pytest.raises(ValueError) as excinfo: + with pytest.raises(ValidationError) as excinfo: b.validate_transaction(create_tx) assert excinfo.value.args[0] == 'A CREATE operation has no inputs' def test_transfer_operation_no_inputs(self, b, user_pk, signed_transfer_tx): signed_transfer_tx.inputs[0].fulfills = None - with pytest.raises(ValueError) as excinfo: + with pytest.raises(ValidationError) as excinfo: b.validate_transaction(signed_transfer_tx) assert excinfo.value.args[0] == 'Only `CREATE` transactions can have null inputs' @@ -741,7 +743,7 @@ class TestBlockValidation(object): b.validate_block(block) def test_invalid_node_pubkey(self, b): - from bigchaindb.common.exceptions import OperationError + from bigchaindb.common.exceptions import SybilError from bigchaindb.common import crypto # blocks can only be created by a federation node @@ -758,8 +760,8 @@ class TestBlockValidation(object): # from a non federation node block = block.sign(tmp_sk) - # check that validate_block raises an OperationError - with pytest.raises(OperationError): + # check that validate_block raises an SybilError + with pytest.raises(SybilError): b.validate_block(block) @@ -778,7 +780,7 @@ class TestMultipleInputs(object): tx = tx.sign([user_sk]) # validate transaction - assert b.is_valid_transaction(tx) == tx + tx.validate(b) assert len(tx.inputs) == 1 assert len(tx.outputs) == 1 @@ -800,7 +802,7 @@ class TestMultipleInputs(object): asset_id=input_tx.id) tx = tx.sign([user_sk]) - assert b.is_valid_transaction(tx) == tx + tx.validate(b) assert len(tx.inputs) == 1 assert len(tx.outputs) == 1 @@ -832,7 +834,7 @@ class TestMultipleInputs(object): transfer_tx = transfer_tx.sign([user_sk, user2_sk]) # validate transaction - assert b.is_valid_transaction(transfer_tx) == transfer_tx + transfer_tx.validate(b) assert len(transfer_tx.inputs) == 1 assert len(transfer_tx.outputs) == 1 @@ -865,7 +867,7 @@ class TestMultipleInputs(object): asset_id=tx_input.id) tx = tx.sign([user_sk, user2_sk]) - assert b.is_valid_transaction(tx) == tx + tx.validate(b) assert len(tx.inputs) == 1 assert len(tx.outputs) == 1 @@ -1219,7 +1221,6 @@ def test_cant_spend_same_input_twice_in_tx(b, genesis_block): tx_transfer = Transaction.transfer(dup_inputs, [([b.me], 200)], asset_id=tx_create.id) tx_transfer_signed = tx_transfer.sign([b.me_private]) - assert b.is_valid_transaction(tx_transfer_signed) is False with pytest.raises(DoubleSpend): tx_transfer_signed.validate(b) diff --git a/tests/pipelines/test_block_creation.py b/tests/pipelines/test_block_creation.py index 2991f3cf..de829167 100644 --- a/tests/pipelines/test_block_creation.py +++ b/tests/pipelines/test_block_creation.py @@ -46,28 +46,19 @@ def test_validate_transaction_handles_exceptions(b, signed_create_tx): """ from bigchaindb.pipelines.block import BlockPipeline block_maker = BlockPipeline() + from bigchaindb.common.exceptions import ValidationError - # Test SchemaValidationError tx_dict = signed_create_tx.to_dict() - tx_dict['invalid_key'] = 'schema validation gonna getcha!' - assert block_maker.validate_tx(tx_dict) is None - # Test InvalidHash - tx_dict = signed_create_tx.to_dict() - tx_dict['id'] = 'a' * 64 - assert block_maker.validate_tx(tx_dict) is None + with patch('bigchaindb.models.Transaction.validate') as validate: + # Assert that validationerror gets caught + validate.side_effect = ValidationError() + assert block_maker.validate_tx(tx_dict) is None - # Test InvalidSignature when we pass a bad fulfillment - tx_dict = signed_create_tx.to_dict() - tx_dict['inputs'][0]['fulfillment'] = 'cf:0:aaaaaaaaaaaaaaaaaaaaaaaaa' - assert block_maker.validate_tx(tx_dict) is None - - # Test AmountError - signed_create_tx.outputs[0].amount = 0 - tx_dict = signed_create_tx.to_dict() - # set the correct value back so that we can continue using it - signed_create_tx.outputs[0].amount = 1 - assert block_maker.validate_tx(tx_dict) is None + # Assert that another error doesnt + validate.side_effect = IOError() + with pytest.raises(IOError): + block_maker.validate_tx(tx_dict) def test_create_block(b, user_pk): diff --git a/tests/pipelines/test_vote.py b/tests/pipelines/test_vote.py index 20beac1e..aaa184f0 100644 --- a/tests/pipelines/test_vote.py +++ b/tests/pipelines/test_vote.py @@ -135,10 +135,17 @@ def test_vote_validate_transaction(b): validation = vote_obj.validate_tx(tx, 123, 1) assert validation == (True, 123, 1) - # NOTE: Submit unsigned transaction to `validate_tx` yields `False`. - tx = Transaction.create([b.me], [([b.me], 1)]) - validation = vote_obj.validate_tx(tx, 456, 10) - assert validation == (False, 456, 10) + with patch('bigchaindb.models.Transaction.validate') as validate: + # Assert that validationerror gets caught + validate.side_effect = ValidationError() + validation = vote_obj.validate_tx(tx, 456, 10) + assert validation == (False, 456, 10) + + # Assert that another error doesnt + validate.side_effect = IOError() + with pytest.raises(IOError): + validation = vote_obj.validate_tx(tx, 456, 10) + @pytest.mark.genesis diff --git a/tests/test_models.py b/tests/test_models.py index 8de3a6c2..e3252f9b 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -1,4 +1,5 @@ from pytest import raises +from bigchaindb.common.exceptions import ValidationError class TestTransactionModel(object): @@ -8,12 +9,12 @@ class TestTransactionModel(object): tx = Transaction.create([b.me], [([b.me], 1)]) tx.operation = 'something invalid' - with raises(TypeError): + with raises(ValidationError): tx.validate(b) tx.operation = 'CREATE' tx.inputs = [] - with raises(ValueError): + with raises(ValidationError): tx.validate(b) @@ -61,11 +62,10 @@ class TestBlockModel(object): assert block.to_dict() == expected def test_block_invalid_serializaton(self): - from bigchaindb.common.exceptions import OperationError from bigchaindb.models import Block block = Block([]) - with raises(OperationError): + with raises(ValueError): block.to_dict() def test_block_deserialization(self, b): diff --git a/tests/web/test_transactions.py b/tests/web/test_transactions.py index 71f4f0e9..cf4105e9 100644 --- a/tests/web/test_transactions.py +++ b/tests/web/test_transactions.py @@ -1,4 +1,3 @@ -import builtins import json from unittest.mock import patch @@ -113,18 +112,15 @@ def test_post_create_transaction_with_invalid_schema(client, caplog): ('DoubleSpend', 'Nope! It is gone now!'), ('InvalidHash', 'Do not smoke that!'), ('InvalidSignature', 'Falsche Unterschrift!'), - ('OperationError', 'Create and transfer!'), + ('ValidationError', 'Create and transfer!'), ('TransactionDoesNotExist', 'Hallucinations?'), ('TransactionOwnerError', 'Not yours!'), ('TransactionNotInValidBlock', 'Wait, maybe?'), - ('ValueError', '?'), + ('ValidationError', '?'), )) def test_post_invalid_transaction(client, exc, msg, monkeypatch, caplog): from bigchaindb.common import exceptions - try: - exc_cls = getattr(exceptions, exc) - except AttributeError: - exc_cls = getattr(builtins, 'ValueError') + exc_cls = getattr(exceptions, exc) def mock_validation(self_, tx): raise exc_cls(msg) From 59e21bfa4dba107e189f5bfb9aab998fcccff06b Mon Sep 17 00:00:00 2001 From: Scott Sadler Date: Fri, 3 Mar 2017 11:52:12 +0100 Subject: [PATCH 2/7] fix test, log tx validation errors and document ValidationError --- bigchaindb/common/exceptions.py | 5 +++++ bigchaindb/pipelines/block.py | 2 +- bigchaindb/pipelines/vote.py | 4 ++-- tests/pipelines/test_vote.py | 3 +-- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/bigchaindb/common/exceptions.py b/bigchaindb/common/exceptions.py index 4b95f84b..76513010 100644 --- a/bigchaindb/common/exceptions.py +++ b/bigchaindb/common/exceptions.py @@ -42,6 +42,11 @@ class OperationError(BigchainDBError): ################################################################################ # Validation errors +# +# All validation errors (which are handleable errors, not faults) should +# subclass ValidationError. However, where possible they should also have their +# own distinct type to differentiate them from other validation errors, +# especially for the purposes of testing. class ValidationError(BigchainDBError): diff --git a/bigchaindb/pipelines/block.py b/bigchaindb/pipelines/block.py index 2a43686a..b1d6cdee 100644 --- a/bigchaindb/pipelines/block.py +++ b/bigchaindb/pipelines/block.py @@ -76,7 +76,7 @@ class BlockPipeline: tx.validate(self.bigchain) return tx except ValidationError as e: - # todo: log + logger.warning('Invalid tx: %s' % e) self.bigchain.delete_transaction(tx.id) return None diff --git a/bigchaindb/pipelines/vote.py b/bigchaindb/pipelines/vote.py index 088c4eac..0431e20b 100644 --- a/bigchaindb/pipelines/vote.py +++ b/bigchaindb/pipelines/vote.py @@ -108,8 +108,8 @@ class Vote: try: tx.validate(self.bigchain) valid = True - except exceptions.ValidationError: - # TODO: log + except exceptions.ValidationError as e: + logger.warning('Invalid tx: %s' % e) valid = False return valid, block_id, num_tx diff --git a/tests/pipelines/test_vote.py b/tests/pipelines/test_vote.py index aaa184f0..fa167d17 100644 --- a/tests/pipelines/test_vote.py +++ b/tests/pipelines/test_vote.py @@ -128,7 +128,7 @@ def test_validate_block_with_invalid_signature(b): @pytest.mark.genesis def test_vote_validate_transaction(b): from bigchaindb.pipelines import vote - from bigchaindb.models import Transaction + from bigchaindb.common.exceptions import ValidationError tx = dummy_tx(b) vote_obj = vote.Vote() @@ -147,7 +147,6 @@ def test_vote_validate_transaction(b): validation = vote_obj.validate_tx(tx, 456, 10) - @pytest.mark.genesis def test_vote_accumulates_transactions(b): from bigchaindb.pipelines import vote From 3346fcb47b993c6d6fb88a20792fdb8b0fa47202 Mon Sep 17 00:00:00 2001 From: Scott Sadler Date: Fri, 3 Mar 2017 13:48:52 +0100 Subject: [PATCH 3/7] break BigchainDBCritical into CriticalDoubleSpend and CriticalDoubleInclusion and add test --- bigchaindb/backend/exceptions.py | 8 ++++-- bigchaindb/core.py | 4 +-- tests/db/test_bigchain_api.py | 43 +++++++++++++++++++++++++++++--- 3 files changed, 47 insertions(+), 8 deletions(-) diff --git a/bigchaindb/backend/exceptions.py b/bigchaindb/backend/exceptions.py index 3b712b08..a5eff242 100644 --- a/bigchaindb/backend/exceptions.py +++ b/bigchaindb/backend/exceptions.py @@ -17,5 +17,9 @@ class DuplicateKeyError(OperationError): """Exception raised when an insert fails because the key is not unique""" -class BigchainDBCritical(Exception): - """Unhandleable error that requires attention""" +class CriticalDoubleSpend(BigchainDBError): + """Data integrity error that requires attention""" + + +class CriticalDoubleInclusion(BigchainDBError): + """Data integrity error that requires attention""" diff --git a/bigchaindb/core.py b/bigchaindb/core.py index 084df928..c0da6177 100644 --- a/bigchaindb/core.py +++ b/bigchaindb/core.py @@ -308,7 +308,7 @@ class Bigchain(object): if list(validity.values()).count(Bigchain.BLOCK_VALID) > 1: block_ids = str([block for block in validity if validity[block] == Bigchain.BLOCK_VALID]) - raise backend_exceptions.BigchainDBCritical( + raise backend_exceptions.CriticalDoubleInclusion( 'Transaction {tx} is present in ' 'multiple valid blocks: {block_ids}' .format(tx=txid, block_ids=block_ids)) @@ -361,7 +361,7 @@ class Bigchain(object): if self.get_transaction(transaction['id']): num_valid_transactions += 1 if num_valid_transactions > 1: - raise exceptions.BigchainDBCritical( + raise backend_exceptions.CriticalDoubleSpend( '`{}` was spent more than once. There is a problem' ' with the chain'.format(txid)) diff --git a/tests/db/test_bigchain_api.py b/tests/db/test_bigchain_api.py index 1f71862a..f56c9e5a 100644 --- a/tests/db/test_bigchain_api.py +++ b/tests/db/test_bigchain_api.py @@ -93,7 +93,7 @@ class TestBigchainApi(object): @pytest.mark.genesis def test_get_spent_with_double_inclusion_detected(self, b, monkeypatch): - from bigchaindb.backend.exceptions import BigchainDBCritical + from bigchaindb.backend.exceptions import CriticalDoubleInclusion from bigchaindb.models import Transaction tx = Transaction.create([b.me], [([b.me], 1)]) @@ -123,12 +123,47 @@ class TestBigchainApi(object): vote = b.vote(block3.id, b.get_last_voted_block().id, True) b.write_vote(vote) - with pytest.raises(BigchainDBCritical): + with pytest.raises(CriticalDoubleInclusion): + b.get_spent(tx.id, 0) + + @pytest.mark.genesis + def test_get_spent_with_double_spend_detected(self, b, monkeypatch): + from bigchaindb.backend.exceptions import CriticalDoubleSpend + from bigchaindb.models import Transaction + + tx = Transaction.create([b.me], [([b.me], 1)]) + tx = tx.sign([b.me_private]) + + monkeypatch.setattr('time.time', lambda: 1000000000) + block1 = b.create_block([tx]) + b.write_block(block1) + + monkeypatch.setattr('time.time', lambda: 1000000020) + transfer_tx = Transaction.transfer(tx.to_inputs(), [([b.me], 1)], + asset_id=tx.id) + transfer_tx = transfer_tx.sign([b.me_private]) + block2 = b.create_block([transfer_tx]) + b.write_block(block2) + + monkeypatch.setattr('time.time', lambda: 1000000030) + transfer_tx2 = Transaction.transfer(tx.to_inputs(), [([b.me], 2)], + asset_id=tx.id) + transfer_tx2 = transfer_tx2.sign([b.me_private]) + block3 = b.create_block([transfer_tx2]) + b.write_block(block3) + + # Vote both block2 and block3 valid + vote = b.vote(block2.id, b.get_last_voted_block().id, True) + b.write_vote(vote) + vote = b.vote(block3.id, b.get_last_voted_block().id, True) + b.write_vote(vote) + + with pytest.raises(CriticalDoubleSpend): b.get_spent(tx.id, 0) @pytest.mark.genesis def test_get_block_status_for_tx_with_double_inclusion(self, b, monkeypatch): - from bigchaindb.backend.exceptions import BigchainDBCritical + from bigchaindb.backend.exceptions import CriticalDoubleInclusion from bigchaindb.models import Transaction tx = Transaction.create([b.me], [([b.me], 1)]) @@ -148,7 +183,7 @@ class TestBigchainApi(object): vote = b.vote(block2.id, b.get_last_voted_block().id, True) b.write_vote(vote) - with pytest.raises(BigchainDBCritical): + with pytest.raises(CriticalDoubleInclusion): b.get_blocks_status_containing_tx(tx.id) @pytest.mark.genesis From 352627b83ab6549e5951ab6934440b59fe9721e4 Mon Sep 17 00:00:00 2001 From: Scott Sadler Date: Mon, 6 Mar 2017 12:12:04 +0100 Subject: [PATCH 4/7] add test that asset id is a string --- tests/db/test_bigchain_api.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/db/test_bigchain_api.py b/tests/db/test_bigchain_api.py index f56c9e5a..d2cc82eb 100644 --- a/tests/db/test_bigchain_api.py +++ b/tests/db/test_bigchain_api.py @@ -1313,3 +1313,10 @@ def test_is_new_transaction(b, genesis_block): # Tx is new because it's only found in an invalid block assert b.is_new_transaction(tx.id) assert b.is_new_transaction(tx.id, exclude_block_id=block.id) + + +def test_validate_asset_id_string(signed_transfer_tx): + from bigchaindb.common.exceptions import ValidationError + signed_transfer_tx.asset['id'] = 1 + with pytest.raises(ValidationError): + signed_transfer_tx.validate(None) From e5dd5c665ba707fc40e9369889f92cff59f8761b Mon Sep 17 00:00:00 2001 From: Scott Sadler Date: Tue, 7 Mar 2017 13:15:31 +0100 Subject: [PATCH 5/7] address vrde's comments, reshuffle some exceptions around --- bigchaindb/backend/exceptions.py | 8 -------- bigchaindb/common/exceptions.py | 4 ++-- bigchaindb/core.py | 6 +++--- bigchaindb/exceptions.py | 8 ++++++++ bigchaindb/models.py | 6 +++--- tests/db/test_bigchain_api.py | 6 +++--- 6 files changed, 19 insertions(+), 19 deletions(-) diff --git a/bigchaindb/backend/exceptions.py b/bigchaindb/backend/exceptions.py index a5eff242..017e19e4 100644 --- a/bigchaindb/backend/exceptions.py +++ b/bigchaindb/backend/exceptions.py @@ -15,11 +15,3 @@ class OperationError(BackendError): class DuplicateKeyError(OperationError): """Exception raised when an insert fails because the key is not unique""" - - -class CriticalDoubleSpend(BigchainDBError): - """Data integrity error that requires attention""" - - -class CriticalDoubleInclusion(BigchainDBError): - """Data integrity error that requires attention""" diff --git a/bigchaindb/common/exceptions.py b/bigchaindb/common/exceptions.py index 76513010..5cffda7c 100644 --- a/bigchaindb/common/exceptions.py +++ b/bigchaindb/common/exceptions.py @@ -92,8 +92,8 @@ class AmountError(ValidationError): """Raised when there is a problem with a transaction's output amounts""" -class TransactionDoesNotExist(ValidationError): - """Raised if the transaction is not in the database""" +class InputDoesNotExist(ValidationError): + """Raised if a transaction input does not exist""" class TransactionOwnerError(ValidationError): diff --git a/bigchaindb/core.py b/bigchaindb/core.py index c0da6177..283969e3 100644 --- a/bigchaindb/core.py +++ b/bigchaindb/core.py @@ -4,6 +4,7 @@ import collections from time import time from itertools import compress +from bigchaindb import exceptions as core_exceptions from bigchaindb.common import crypto, exceptions from bigchaindb.common.utils import gen_timestamp, serialize from bigchaindb.common.transaction import TransactionLink @@ -11,7 +12,6 @@ from bigchaindb.common.transaction import TransactionLink import bigchaindb from bigchaindb import backend, config_utils, utils -from bigchaindb.backend import exceptions as backend_exceptions from bigchaindb.consensus import BaseConsensusRules from bigchaindb.models import Block, Transaction @@ -308,7 +308,7 @@ class Bigchain(object): if list(validity.values()).count(Bigchain.BLOCK_VALID) > 1: block_ids = str([block for block in validity if validity[block] == Bigchain.BLOCK_VALID]) - raise backend_exceptions.CriticalDoubleInclusion( + raise core_exceptions.CriticalDoubleInclusion( 'Transaction {tx} is present in ' 'multiple valid blocks: {block_ids}' .format(tx=txid, block_ids=block_ids)) @@ -361,7 +361,7 @@ class Bigchain(object): if self.get_transaction(transaction['id']): num_valid_transactions += 1 if num_valid_transactions > 1: - raise backend_exceptions.CriticalDoubleSpend( + raise core_exceptions.CriticalDoubleSpend( '`{}` was spent more than once. There is a problem' ' with the chain'.format(txid)) diff --git a/bigchaindb/exceptions.py b/bigchaindb/exceptions.py index d8a4cd73..336ce231 100644 --- a/bigchaindb/exceptions.py +++ b/bigchaindb/exceptions.py @@ -1,2 +1,10 @@ class BigchainDBError(Exception): """Base class for BigchainDB exceptions.""" + + +class CriticalDoubleSpend(BigchainDBError): + """Data integrity error that requires attention""" + + +class CriticalDoubleInclusion(BigchainDBError): + """Data integrity error that requires attention""" diff --git a/bigchaindb/models.py b/bigchaindb/models.py index fd71f98d..7c390967 100644 --- a/bigchaindb/models.py +++ b/bigchaindb/models.py @@ -1,6 +1,6 @@ from bigchaindb.common.crypto import hash_data, PublicKey, PrivateKey from bigchaindb.common.exceptions import (InvalidHash, InvalidSignature, - DoubleSpend, TransactionDoesNotExist, + DoubleSpend, InputDoesNotExist, TransactionNotInValidBlock, AssetIdMismatch, AmountError, SybilError, ValidationError) @@ -60,8 +60,8 @@ class Transaction(Transaction): get_transaction(input_txid, include_status=True) if input_tx is None: - raise TransactionDoesNotExist("input `{}` doesn't exist" - .format(input_txid)) + raise InputDoesNotExist("input `{}` doesn't exist" + .format(input_txid)) if status != bigchain.TX_VALID: raise TransactionNotInValidBlock( diff --git a/tests/db/test_bigchain_api.py b/tests/db/test_bigchain_api.py index d2cc82eb..b1f94b73 100644 --- a/tests/db/test_bigchain_api.py +++ b/tests/db/test_bigchain_api.py @@ -93,7 +93,7 @@ class TestBigchainApi(object): @pytest.mark.genesis def test_get_spent_with_double_inclusion_detected(self, b, monkeypatch): - from bigchaindb.backend.exceptions import CriticalDoubleInclusion + from bigchaindb.exceptions import CriticalDoubleInclusion from bigchaindb.models import Transaction tx = Transaction.create([b.me], [([b.me], 1)]) @@ -128,7 +128,7 @@ class TestBigchainApi(object): @pytest.mark.genesis def test_get_spent_with_double_spend_detected(self, b, monkeypatch): - from bigchaindb.backend.exceptions import CriticalDoubleSpend + from bigchaindb.exceptions import CriticalDoubleSpend from bigchaindb.models import Transaction tx = Transaction.create([b.me], [([b.me], 1)]) @@ -163,7 +163,7 @@ class TestBigchainApi(object): @pytest.mark.genesis def test_get_block_status_for_tx_with_double_inclusion(self, b, monkeypatch): - from bigchaindb.backend.exceptions import CriticalDoubleInclusion + from bigchaindb.exceptions import CriticalDoubleInclusion from bigchaindb.models import Transaction tx = Transaction.create([b.me], [([b.me], 1)]) From 1db8d59a880df7170c1e4cc3e889c22cad5072b8 Mon Sep 17 00:00:00 2001 From: Scott Sadler Date: Tue, 7 Mar 2017 13:24:20 +0100 Subject: [PATCH 6/7] use comma for arguments in logging calls instead of format operator --- bigchaindb/pipelines/block.py | 2 +- bigchaindb/pipelines/vote.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bigchaindb/pipelines/block.py b/bigchaindb/pipelines/block.py index b1d6cdee..fd503867 100644 --- a/bigchaindb/pipelines/block.py +++ b/bigchaindb/pipelines/block.py @@ -76,7 +76,7 @@ class BlockPipeline: tx.validate(self.bigchain) return tx except ValidationError as e: - logger.warning('Invalid tx: %s' % e) + logger.warning('Invalid tx: %s', e) self.bigchain.delete_transaction(tx.id) return None diff --git a/bigchaindb/pipelines/vote.py b/bigchaindb/pipelines/vote.py index 0431e20b..e4273470 100644 --- a/bigchaindb/pipelines/vote.py +++ b/bigchaindb/pipelines/vote.py @@ -109,7 +109,7 @@ class Vote: tx.validate(self.bigchain) valid = True except exceptions.ValidationError as e: - logger.warning('Invalid tx: %s' % e) + logger.warning('Invalid tx: %s', e) valid = False return valid, block_id, num_tx From a3fccbc599c47457abd11f954bec9a0384f4e21b Mon Sep 17 00:00:00 2001 From: Scott Sadler Date: Wed, 8 Mar 2017 13:01:52 +0100 Subject: [PATCH 7/7] change TransactionDoesNotExist to InputDoesNotExist in tests --- tests/db/test_bigchain_api.py | 8 ++++---- tests/web/test_transactions.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/db/test_bigchain_api.py b/tests/db/test_bigchain_api.py index b1f94b73..c39a104f 100644 --- a/tests/db/test_bigchain_api.py +++ b/tests/db/test_bigchain_api.py @@ -567,7 +567,7 @@ class TestBigchainApi(object): @pytest.mark.usefixtures('inputs') def test_non_create_input_not_found(self, b, user_pk): from cryptoconditions import Ed25519Fulfillment - from bigchaindb.common.exceptions import TransactionDoesNotExist + from bigchaindb.common.exceptions import InputDoesNotExist from bigchaindb.common.transaction import Input, TransactionLink from bigchaindb.models import Transaction from bigchaindb import Bigchain @@ -579,7 +579,7 @@ class TestBigchainApi(object): tx = Transaction.transfer([input], [([user_pk], 1)], asset_id='mock_asset_link') - with pytest.raises(TransactionDoesNotExist): + with pytest.raises(InputDoesNotExist): tx.validate(Bigchain()) def test_count_backlog(self, b, user_pk): @@ -615,11 +615,11 @@ class TestTransactionValidation(object): assert excinfo.value.args[0] == 'Only `CREATE` transactions can have null inputs' def test_non_create_input_not_found(self, b, user_pk, signed_transfer_tx): - from bigchaindb.common.exceptions import TransactionDoesNotExist + from bigchaindb.common.exceptions import InputDoesNotExist from bigchaindb.common.transaction import TransactionLink signed_transfer_tx.inputs[0].fulfills = TransactionLink('c', 0) - with pytest.raises(TransactionDoesNotExist): + with pytest.raises(InputDoesNotExist): b.validate_transaction(signed_transfer_tx) @pytest.mark.usefixtures('inputs') diff --git a/tests/web/test_transactions.py b/tests/web/test_transactions.py index cf4105e9..5533dbd0 100644 --- a/tests/web/test_transactions.py +++ b/tests/web/test_transactions.py @@ -113,7 +113,7 @@ def test_post_create_transaction_with_invalid_schema(client, caplog): ('InvalidHash', 'Do not smoke that!'), ('InvalidSignature', 'Falsche Unterschrift!'), ('ValidationError', 'Create and transfer!'), - ('TransactionDoesNotExist', 'Hallucinations?'), + ('InputDoesNotExist', 'Hallucinations?'), ('TransactionOwnerError', 'Not yours!'), ('TransactionNotInValidBlock', 'Wait, maybe?'), ('ValidationError', '?'),