Merge pull request #1208 from bigchaindb/is_new_transaction

Bigchaindb.is_new_transaction, fix double inclusion in Vote pipeline

bigchaindb/backend/mongodb/query.py

@@ -212,13 +212,6 @@ def get_block(conn, block_id):
                   projection={'_id': False}))
 
 
-@register_query(MongoDBConnection)
-def has_transaction(conn, transaction_id):
-    return bool(conn.run(
-        conn.collection('bigchain')
-        .find_one({'block.transactions.id': transaction_id})))
-
-
 @register_query(MongoDBConnection)
 def count_blocks(conn):
     return conn.run(

bigchaindb/backend/query.py

@@ -211,20 +211,6 @@ def get_block(connection, block_id):
     raise NotImplementedError
 
 
-@singledispatch
-def has_transaction(connection, transaction_id):
-    """Check if a transaction exists in the bigchain table.
-
-    Args:
-        transaction_id (str): the id of the transaction to check.
-
-    Returns:
-        ``True`` if the transaction exists, ``False`` otherwise.
-    """
-
-    raise NotImplementedError
-
-
 @singledispatch
 def count_blocks(connection):
     """Count the number of blocks in the bigchain table.

bigchaindb/backend/rethinkdb/query.py

@@ -158,13 +158,6 @@ def get_block(connection, block_id):
     return connection.run(r.table('bigchain').get(block_id))
 
 
-@register_query(RethinkDBConnection)
-def has_transaction(connection, transaction_id):
-    return bool(connection.run(
-            r.table('bigchain', read_mode=READ_MODE)
-            .get_all(transaction_id, index='transaction_id').count()))
-
-
 @register_query(RethinkDBConnection)
 def count_blocks(connection):
     return connection.run(

bigchaindb/core.py

@@ -187,6 +187,22 @@ class Bigchain(object):
                 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
+        VALID or UNDECIDED block. Return False otherwise.
+
+        Args:
+            txid (str): Transaction ID
+            exclude_block_id (str): Exclude block from search
+        """
+        block_statuses = self.get_blocks_status_containing_tx(txid)
+        block_statuses.pop(exclude_block_id, None)
+        for status in block_statuses.values():
+            if status != self.BLOCK_INVALID:
+                return False
+        return True
+
     def get_block(self, block_id, include_status=False):
         """Get the block with the specified `block_id` (and optionally its status)
 
@@ -534,9 +550,6 @@ class Bigchain(object):
 
         return backend.query.write_block(self.connection, block)
 
-    def transaction_exists(self, transaction_id):
-        return backend.query.has_transaction(self.connection, transaction_id)
-
     def prepare_genesis_block(self):
         """Prepare a genesis block."""
 

bigchaindb/pipelines/block.py

@@ -67,28 +67,19 @@ class BlockPipeline:
                 AmountError):
             return None
 
-        if self.bigchain.transaction_exists(tx.id):
-            # if the transaction already exists, we must check whether
-            # it's in a valid or undecided block
-            tx, status = self.bigchain.get_transaction(tx.id,
-                                                       include_status=True)
-            if status == self.bigchain.TX_VALID \
-               or status == self.bigchain.TX_UNDECIDED:
-                # if the tx is already in a valid or undecided block,
-                # then it no longer should be in the backlog, or added
-                # to a new block. We can delete and drop it.
-                self.bigchain.delete_transaction(tx.id)
-                return None
-
-        tx_validated = self.bigchain.is_valid_transaction(tx)
-        if tx_validated:
-            return tx
-        else:
-            # if the transaction is not valid, remove it from the
-            # backlog
+        # If transaction is in any VALID or UNDECIDED block we
+        # should not include it again
+        if not self.bigchain.is_new_transaction(tx.id):
             self.bigchain.delete_transaction(tx.id)
             return None
 
+        # If transaction is not valid it should not be included
+        if not self.bigchain.is_valid_transaction(tx):
+            self.bigchain.delete_transaction(tx.id)
+            return None
+
+        return tx
+
     def create(self, tx, timeout=False):
         """Create a block.
 

bigchaindb/pipelines/vote.py

@@ -98,7 +98,8 @@ class Vote:
             yield tx, block_id, num_tx
 
     def validate_tx(self, tx, block_id, num_tx):
-        """Validate a transaction.
+        """Validate a transaction. Transaction must also not be in any VALID
+           block.
 
         Args:
             tx (dict): the transaction to validate
@@ -109,7 +110,12 @@ class Vote:
             Three values are returned, the validity of the transaction,
             ``block_id``, ``num_tx``.
         """
-        return bool(self.bigchain.is_valid_transaction(tx)), block_id, num_tx
+        new = self.bigchain.is_new_transaction(tx.id, exclude_block_id=block_id)
+        if not new:
+            return False, block_id, num_tx
+
+        valid = bool(self.bigchain.is_valid_transaction(tx))
+        return valid, block_id, num_tx
 
     def vote(self, tx_validity, block_id, num_tx):
         """Collect the validity of transactions and cast a vote when ready.

tests/backend/mongodb/test_queries.py

@@ -248,19 +248,6 @@ def test_get_block(signed_create_tx):
     assert block_db == block.to_dict()
 
 
-def test_has_transaction(signed_create_tx):
-    from bigchaindb.backend import connect, query
-    from bigchaindb.models import Block
-    conn = connect()
-
-    # create and insert block
-    block = Block(transactions=[signed_create_tx])
-    conn.db.bigchain.insert_one(block.to_dict())
-
-    assert query.has_transaction(conn, signed_create_tx.id)
-    assert query.has_transaction(conn, 'aaa') is False
-
-
 def test_count_blocks(signed_create_tx):
     from bigchaindb.backend import connect, query
     from bigchaindb.models import Block

tests/backend/test_generics.py

@@ -29,7 +29,6 @@ def test_schema(schema_func_name, args_qty):
     ('get_votes_by_block_id', 1),
     ('write_block', 1),
     ('get_block', 1),
-    ('has_transaction', 1),
     ('write_vote', 1),
     ('get_last_voted_block', 1),
     ('get_unvoted_blocks', 1),

tests/db/test_bigchain_api.py

@@ -1240,3 +1240,40 @@ def test_transaction_unicode(b):
     assert b.get_block(block.id) == block.to_dict()
     assert block.validate(b) == block
     assert beer_json in serialize(block.to_dict())
+
+
+@pytest.mark.bdb
+def test_is_new_transaction(b, genesis_block):
+    from bigchaindb.models import Transaction
+
+    def write_tx(n):
+        tx = Transaction.create([b.me], [([b.me], n)])
+        tx = tx.sign([b.me_private])
+        # Tx is new because it's not in any block
+        assert b.is_new_transaction(tx.id)
+
+        block = b.create_block([tx])
+        b.write_block(block)
+        return tx, block
+
+    # test VALID case
+    tx, block = write_tx(1)
+    # Tx is now in undecided block
+    assert not b.is_new_transaction(tx.id)
+    assert b.is_new_transaction(tx.id, exclude_block_id=block.id)
+    # After voting valid, should not be new
+    vote = b.vote(block.id, genesis_block.id, True)
+    b.write_vote(vote)
+    assert not b.is_new_transaction(tx.id)
+    assert b.is_new_transaction(tx.id, exclude_block_id=block.id)
+
+    # test INVALID case
+    tx, block = write_tx(2)
+    # Tx is now in undecided block
+    assert not b.is_new_transaction(tx.id)
+    assert b.is_new_transaction(tx.id, exclude_block_id=block.id)
+    vote = b.vote(block.id, genesis_block.id, False)
+    b.write_vote(vote)
+    # 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)

tests/pipelines/test_vote.py

@@ -629,3 +629,17 @@ def test_start(mock_start, b):
     from bigchaindb.pipelines import vote
     vote.start()
     mock_start.assert_called_with()
+
+
+@pytest.mark.genesis
+def test_vote_no_double_inclusion(b):
+    from bigchaindb.pipelines import vote
+
+    tx = dummy_tx(b)
+    block = b.create_block([tx])
+    r = vote.Vote().validate_tx(tx, block.id, 1)
+    assert r == (True, block.id, 1)
+
+    b.write_block(block)
+    r = vote.Vote().validate_tx(tx, 'other_block_id', 1)
+    assert r == (False, 'other_block_id', 1)

tests/test_core.py

@@ -88,12 +88,3 @@ def test_has_previous_vote(monkeypatch):
     block = {'votes': ({'node_pubkey': 'pubkey'},)}
     with pytest.raises(Exception):
         bigchain.has_previous_vote(block)
-
-
-@pytest.mark.parametrize('exists', (True, False))
-def test_transaction_exists(monkeypatch, exists):
-    from bigchaindb.core import Bigchain
-    monkeypatch.setattr(
-        'bigchaindb.backend.query.has_transaction', lambda x, y: exists)
-    bigchain = Bigchain(public_key='pubkey', private_key='privkey')
-    assert bigchain.transaction_exists('txid') is exists