Rebase/feat/586/integrate tx model #641

sbellem · 2016-09-26T16:33:21Z

No description provided.

codecov-io · 2016-09-26T16:37:21Z

Current coverage is 95.84% (diff: 98.23%)

Merging #641 into master will decrease coverage by 0.02%

@@             master       #641   diff @@
==========================================
  Files            23         21     -2   
  Lines          1308       1155   -153   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           1254       1107   -147   
+ Misses           54         48     -6   
  Partials          0          0

Powered by Codecov. Last update e74b4ee...df86def

sbellem · 2016-09-26T16:39:11Z

Note to @TimDaub @rhsimplex @vrde and @diminator The rebase was quite messy, so it would be important that each one of you double-checks that nothing was lost or changed incorrectly along the way.

Some key things to look for (there may be others):

refactor of rethinkdb connection, pipeline -- @vrde
voting/election mechanisms, handling of stale txs -- @rhsimplex
server api -- @diminator
all new tx model integration changes -- @TimDaub

TimDaub · 2016-09-27T08:38:26Z

.travis.yml

@@ -12,7 +12,7 @@ before_install:

 install:
  - sudo apt-get install rethinkdb
-  - pip install -e .[test]
+  - pip install -e .[test] --process-dependency-links


Before merging into master:

Push latest BDB-common to pip

Remove this label

Update version in setup.py

TimDaub · 2016-09-27T08:38:45Z

Dockerfile-dev

@@ -9,4 +9,4 @@ RUN pip install --upgrade pip

 COPY . /usr/src/app/

-RUN pip install --no-cache-dir -e .[dev]
+RUN pip install --no-cache-dir --process-dependency-links -e .[dev]


Before merging into master:

Push latest BDB-common to pip

Remove this label

Update version in setup.py

TimDaub · 2016-09-27T08:42:50Z

bigchaindb/core.py

@@ -268,7 +271,7 @@ def search_block_election_on_index(self, value, index):
            index (str): name of a secondary index, e.g. 'transaction_id'

        Returns:
-            A list of blocks with with only election information
+            A list of blocks (list(dict))with with only election information


Missing space

will take care of it

TimDaub · 2016-09-27T08:48:11Z

bigchaindb/models.py

+                    raise DoubleSpend('input `{}` was already spent'
+                                      .format(input_txid))
+        else:
+            raise TypeError('`operation` must be either `TRANSFER`, `CREATE` '


We could take Transaction.ALLOWED_OPERATIONS here to create the error message.

Might also be nice to tell the user what their given operation was too.

TimDaub · 2016-09-27T08:57:19Z

bigchaindb/models.py

+        verifying_key = VerifyingKey(block['node_pubkey'])
+        try:
+            return verifying_key.verify(block_serialized, self.signature)
+        # TODO FOR CC: Shouldn't throw `AttributeError` or `ValueError` but


I've created a ticket for this in the cryptoconditions repo: bigchaindb/cryptoconditions#27

So, should we remove the TODO note?

TimDaub · 2016-09-27T09:00:14Z

bigchaindb/pipelines/block.py

@@ -1,6 +1,6 @@
 """This module takes care of all the logic related to block creation.


I renamed this class to now be called BlockPipeline, which is hopefully alright (@vrde and @rhsimplex?).
Shall we rename the file as well?

I think it is redundant. The directory/sub-package pipelines acts as a namespace, so I do not understand the need for re-specifying "pipeline", whether for classes, modules, files, etc.

It was renamed to BlockPipeline as I introduced another class called Block that is the actual block.

Since we're handling Blocks in BlockPipeline, I thought a separation of the two would be healthy for readability.

ah ok, I see

Agree with @TimDaub this was confusing to begin with (when I worked on it) and now that blocks are objects, the distinction is necessary

TimDaub · 2016-09-27T09:02:16Z

bigchaindb/pipelines/block.py

        """
-        if self.bigchain.transaction_exists(tx['id']):
+        tx = Transaction.from_dict(tx)
+        if self.bigchain.transaction_exists(tx.id):


@rhsimplex: This is new logic that was added in this commit: c33d123

Would it make sense to put it in: https://github.com/bigchaindb/bigchaindb/pull/641/files#diff-9f1ab5b5daf1b3072e2123b6939bfb07R13 ?

I'm not sure. I think the established validation returns False if the transaction is in an undecided or invalid block, where the block process here treats undecided and valid blocks as the same.

TimDaub · 2016-09-27T09:04:29Z

bigchaindb/pipelines/block.py


        Returns:
            The block.
        """
        r.table('backlog')\
-         .get_all(*[tx['id'] for tx in block['block']['transactions']])\
+         .get_all(*[tx['id'] for tx in block.to_dict()['block']['transactions']])\


Instead of passing the whole block here, we should just pass the transaction ids to delete.

def delete_tx(self, transaction_ids): ...

Q: Do we really need the whole block to be returned? @vrde @rhsimplex

I didn't notice that this function was part of pipelines/block.py.
We'll have to return the block. I fixed it by not deserializing the block.

Done: 0e10a87

TimDaub · 2016-09-27T09:07:04Z

bigchaindb/processes.py

@@ -1,6 +1,7 @@
 import logging
 import multiprocessing as mp

+


Remove additional line.

TimDaub · 2016-09-27T09:09:16Z

bigchaindb/util.py

-    new_tx = create_tx(b.me, transaction['fulfillments'][0]['owners_before'], None, 'CREATE', payload=payload)
-    return new_tx
-
-
 def is_genesis_block(block):


Not very proud of this method.
It handles both a dict-block as well as a Block-block.

Ideally, it should only handle Block-block.

why are both needed?

See: #641 (comment)

TimDaub · 2016-09-27T09:10:23Z

setup.py

+        'bigchaindb-common>=0.0.2',
+    ],
+    dependency_links=[
+        'git+https://github.com/bigchaindb/bigchaindb-common.git@integrate-transaction-model#egg=bigchaindb_common-0.0.2',


Before merging into master, remove this line and push BDB-common to pip.

will take care of it

TimDaub · 2016-09-27T09:10:56Z

tests/conftest.py

+
+
+@pytest.fixture
+def transfer_tx(signed_create_tx, user_vk, user_sk):


I should call this signed_transfer_tx.

TimDaub · 2016-09-27T09:19:58Z

tests/test_commands.py

+@patch('argparse.ArgumentParser.parse_args')
+@patch('bigchaindb.commands.utils.base_parser')
+@patch('bigchaindb.commands.utils.start')
+def test_calling_main(start_mock, base_parser_mock, parse_args_mock,


I think @vrde proposed a better way to test this. Maybe he can comment on this and tell us what to do.

sohkai · 2016-09-27T10:23:35Z

bigchaindb/exceptions.py

@@ -1,65 +0,0 @@
-"""Custom exceptions used in the `bigchaindb` package.


Any reason to not leave these exceptions in bigchaindb? Does bigchaindb_common use these exceptions?

Valid point, lets handle (as you proposed) in this ticket: bigchaindb/bigchaindb-common#23

sohkai · 2016-09-27T10:32:30Z

bigchaindb/consensus.py

-
-
-class BaseConsensusRules(AbstractConsensusRules):
+class BaseConsensusRules():


Can probably rename this to just ConsensusRules now? From my limited viewing, this class also doesn't seem very useful anymore. At best, it feels like these should all be module functions rather than statics.

From my limited viewing, this class also doesn't seem very useful anymore. At best, it feels like these should all be module functions rather than statics.

There are functions to be added to the class: #501

I kind of agree with @sohkai in the sense that functions would be sufficient. The module acts as a namespace.

In any case I would prefer that we treat that in a separate issue, as the current solution works, and this PR is already big enough, and has been time-consuming.

sohkai · 2016-09-27T10:48:40Z

bigchaindb/core.py

@@ -684,9 +621,10 @@ def get_last_voted_block(self):

        except r.ReqlNonExistenceError:
            # return last vote if last vote exists else return Genesis block
-            return list(self.connection.run(
+            block = list(self.connection.run(


I think this is really misleading; it'd be nicer to split these out into two statements (assuming we really have to cast the rdb result to a list first):

res = self.connection.run( r.table('bigchain', read_mode=self.read_mode) .filter(util.is_genesis_block)) block = list(res)[0]

block = list(...) and then Block.from_dict(block) really makes you think how from_dict works with a list, until you see the trailing [0] at the end.

sohkai · 2016-09-27T10:51:30Z

bigchaindb/core.py

+        """Return all the blocks that have not been voted on by this node.
+
+        Returns:
+            block (list(dict)): a list of unvoted blocks


Should probably be :obj:listof:obj:dict?

In any case, why does this return a list of dicts, when it could be returning a list of Blocks?

In any case, why does this return a list of dicts, when it could be returning a list of Blocks?

This method is also the reason why https://github.com/bigchaindb/bigchaindb/pull/641/files/76c796429055d180dcf3caaf2a95e1591e0d743c#r80649416 (is_genesis_block) can handle both dict-blocks as well as Block-blocks.

To understand why, you have to see where its used. Fun fact: It's only used once in the entire code base:

https://github.com/bigchaindb/bigchaindb/blob/master/bigchaindb/pipelines/vote.py#L140

initialize runs only once when you freshly start bigchaindb. It basically simulates the changefeed and hence all blocks the newly initialized node has to work for. The changefeed is in json, so is this method's return value.

My proposal (this should maybe handled outside of this PR) is to put this function into vote.py.

I'll make that change:

Should probably be :obj:list of :obj:dict?

My proposal (this should maybe handled outside of this PR) is to put this function into vote.py.

Sounds like a plan!

sohkai · 2016-09-27T11:16:00Z

bigchaindb/models.py

+
+        input_conditions = []
+        inputs_defined = reduce(and_, [bool(ffill.tx_input) for ffill
+                                       in self.fulfillments])


A better way to do this is inputs_defined = all([ffill.tx_input for ffill in self.fulfillments)]

sohkai · 2016-09-27T11:21:31Z

bigchaindb/models.py

+                    input_conditions.append(input_tx.conditions[input_cid])
+
+                spent = bigchain.get_spent(input_txid, ffill.tx_input.cid)
+                if spent and spent.id != self.id:


Should put this before input_conditions.append(); if we raise here, the append is wasted. Could also use input_cid instead of ffill.tx_input.cid.

sohkai · 2016-09-27T11:25:23Z

bigchaindb/models.py

+        elif transactions is None:
+            self.transactions = []
+        else:
+            self.transactions = transactions


I wonder if self.transactions = transactions || [] is a typical python pattern?

self.transactions = transactions or []

or

self.transactions = transactions if transactions else []

work

sohkai · 2016-09-27T11:28:45Z

bigchaindb/models.py

+            other = other.to_dict()
+        except AttributeError:
+            return False
+        return self.to_dict() == other


to_dict can sometimes throw... which certainly makes for an unpleasant experience if you're trying to block1 == block2. Maybe catch OperationErrors here too?

Puhh, that's a tough one. It could be that other and self both throw OperationError but are the same.
Not sure how to go about this. @vrde @sbellem to halp!

I would handle this in a separate issue, and review the current OperationError mechanism. For instance, it is not clear to me why an empty block cannot have a dictionary representation. It could be an empty dict {}, perhaps, or something like:

{ 'id': None, 'block': { 'timestamp': self.timestamp, 'transactions': [tx.to_dict() for tx in self.transactions], 'node_pubkey': self.node_pubkey, 'voters': self.voters, }, 'signature': self.signature, }

In any case, the point I am trying to make here is that the class initialization method (__init__) allows for no transaction, meanwhile the to_dict method does not, and moreover, the __eq__ comparison method relies on the to_dict method. Which means, if understand correctly, that the one can only compare two Block instances if they have transactions.

I think we could review the class design a bit, and for this reason, it makes more sense to tackle this in a separate issue.

For instance, it is not clear to me why an empty block cannot have a dictionary representation. It could be an empty dict {}, perhaps, or something like:

That's absolutely right. Usually we validate the block right after serialization anyways. So serializing with an empty list of transactions is OK IMO.

Ok, should we create a separate ticket for this? I think it would be simpler.

sohkai · 2016-09-27T11:40:21Z

bigchaindb/models.py

+
+    def sign(self, private_key):
+        block_body = self.to_dict()
+        block_serialized = serialize(block_body['block'])


Don't really like how to_dict() serializes the block (for creating the id), throws it away, and then this serializes it again. Maybe it's worth adding a method that just creates the block dict?

Maybe it's worth adding a method that just creates the block dict?

You mean this part? https://github.com/bigchaindb/bigchaindb/pull/641/files/76c796429055d180dcf3caaf2a95e1591e0d743c#diff-9f1ab5b5daf1b3072e2123b6939bfb07R193 ?

sohkai · 2016-09-27T11:47:09Z

bigchaindb/models.py

+            'id': block_id,
+            'block': block,
+            'signature': self.signature,
+        }


In the future, it might be nice to cache this since this is potentially quite expensive (1000 tx.to_dicts()) to be used each time for random look ups, like .id.

@sohkai Would you mind creating a ticket for that?

handled in bigchaindb/cryptoconditions#27; see bigchaindb/cryptoconditions#27

locally for development purposes

* Adjust imports to bigchaindb_common * Adjust get_spent function signature * Adjust block serialization * Fix BigchainApi Test * Fix TestTransactionValidation tests * Fix TestBlockValidation tests * WIP: TestMultipleInputs * Adjust tests to tx-model interface changes - Fix old tests - Fix tests in TestMultipleInputs class * Remove fulfillment message tests * Fix TransactionMalleability tests * Remove Cryptoconditions tests * Remove create_transaction * Remove signing logic * Remove consensus plugin * Fix block_creation pipeline * Fix election pipeline * Replace some util functions with bdb_common ones - timestamp ==> gen_timestamp - serialize. * Implement Block model * Simplify function signatures for vote functions Change parameter interface for the following functions: - has_previous_vote - verify_vote_signature - block_election_status so that they take a block's id and voters instead of a fake block. * Integrate Block and Transaction model * Fix leftover tests and cleanup conftest * Add bigchaindb-common to install_requires * Delete transactions after block is written (bigchaindb#609) * delete transactions after block is written * cleanup transaction_exists * check for duplicate transactions * delete invalid tx from backlog * test duplicate transaction * Remove dead code * Test processes.py * Test invalid tx in on server * Fix tests for core.py * Fix models tests * Test commands main fn * Add final coverage to vote pipeline * Add more tests to voting pipeline * Remove consensus plugin docs and misc * Post rebase fixes * Fix rebase mess * Remove extra blank line * Improve docstring * Remove comment handled in bigchaindb/cryptoconditions#27; see bigchaindb/cryptoconditions#27 * Fix block serialization in block creation * Add signed_ prefix to transfer_tx * Improve docs * Add library documentation page on pipelines * PR feedback for models.py * Impr. readability of get_last_voted_block * Use dict comprehension * Add docker-compose file to build and serve docs locally for development purposes * Change private_key for signing_key * Improve docstrings * Remove consensus docs * Document new consensus module * Create different transactions for the block * Cleanup variable names in block.py * Create different transactions for the block * Cleanup variable names in block.py

sbellem assigned sbellem, TimDaub, rhsimplex, diminator and vrde Sep 26, 2016

TimDaub reviewed Sep 27, 2016

View reviewed changes

sohkai reviewed Sep 27, 2016

View reviewed changes

sbellem and others added 15 commits September 28, 2016 16:48

Remove extra blank line

445646b

Improve docstring

a21c7e5

Remove comment

28d6e65

handled in bigchaindb/cryptoconditions#27; see bigchaindb/cryptoconditions#27

Fix block serialization in block creation

6fe69e1

Add signed_ prefix to transfer_tx

ff39e1b

Improve docs

d50204e

Add library documentation page on pipelines

604c072

Use dict comprehension

958379b

Add docker-compose file to build and serve docs

dc33288

locally for development purposes

PR feedback for models.py

560a68a

Impr. readability of get_last_voted_block

2d69caa

Change private_key for signing_key

e2e1cce

Improve docstrings

3ab6e21

Remove consensus docs

74b0114

Document new consensus module

eef814c

sbellem force-pushed the rebase/feat/586/integrate-tx-model branch from 38443d8 to eef814c Compare September 28, 2016 14:48

sbellem and others added 4 commits September 28, 2016 17:05

Create different transactions for the block

af0f4c7

Cleanup variable names in block.py

e18db11

Create different transactions for the block

f3e6eaf

Cleanup variable names in block.py

d3081e2

sbellem force-pushed the rebase/feat/586/integrate-tx-model branch from e18db11 to d3081e2 Compare September 28, 2016 15:20

Fix failing tests

df86def

sbellem merged commit 50b0b3c into bigchaindb:master Sep 29, 2016

sohkai mentioned this pull request Sep 29, 2016

Handle Block.to_dict() throwing, and representation of empty Blocks. #649

Closed

TimDaub mentioned this pull request Sep 30, 2016

Wrapup DACS integration bigchaindb/org#22

Closed

27 tasks

sohkai mentioned this pull request Sep 30, 2016

Remove plugin architecture #500

Closed

TimDaub mentioned this pull request Oct 5, 2016

IPLD support in BigchainDB #485

Closed

r-marques mentioned this pull request Oct 20, 2016

Review, and possibly implement class hierarchy for exceptions #721

Closed

pyup-vuln-bot mentioned this pull request Oct 28, 2016

Changelog bigchaindb version 0.7.0 pyupio/safety-db#1686

Closed

		@@ -1,6 +1,6 @@
		"""This module takes care of all the logic related to block creation.



		@pytest.fixture
		def transfer_tx(signed_create_tx, user_vk, user_sk):

		@@ -1,65 +0,0 @@
		"""Custom exceptions used in the `bigchaindb` package.



		class BaseConsensusRules(AbstractConsensusRules):
		class BaseConsensusRules():

Rebase/feat/586/integrate tx model #641

Rebase/feat/586/integrate tx model #641

Conversation

sbellem commented Sep 26, 2016

codecov-io commented Sep 26, 2016 • edited Loading

Current coverage is 95.84% (diff: 98.23%)

sbellem commented Sep 26, 2016

TimDaub Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimDaub Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

sbellem Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimDaub Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimDaub Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohkai Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbellem Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimDaub Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbellem Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

TimDaub Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 26, 2016 •

edited

Loading

TimDaub Sep 27, 2016 •

edited

Loading

TimDaub Sep 27, 2016 •

edited

Loading

sbellem Sep 27, 2016 •

edited

Loading

TimDaub Sep 27, 2016 •

edited

Loading

TimDaub Sep 27, 2016 •

edited

Loading

sohkai Sep 27, 2016 •

edited

Loading

sbellem Sep 27, 2016 •

edited

Loading

TimDaub Sep 27, 2016 •

edited

Loading

sbellem Sep 28, 2016 •

edited

Loading

TimDaub Sep 28, 2016 •

edited

Loading