fix voting bug with prev_block

bigchaindb/voting.py

@@ -1,5 +1,5 @@
 import collections
-from bigchaindb.backend.exceptions import BigchainDBCritical
+
 from bigchaindb.common.schema import SchemaValidationError, validate_vote_schema
 from bigchaindb.common.utils import serialize
 from bigchaindb.common.crypto import PublicKey
@@ -56,31 +56,30 @@ class Voting:
                 except ValueError:
                     pass
             ineligible.append(vote)
-
         return eligible, ineligible
 
     @classmethod
     def count_votes(cls, eligible_votes):
         """
         Given a list of eligible votes, (votes from known nodes that are listed
-        as voters), count the votes to produce three quantities:
+        as voters), produce the number that say valid and the number that say
+        invalid.
 
-            Number of votes that say valid
+        * Detect if there are multiple votes from a single node and return them
-            Number of votes that say invalid
-            Highest agreement on previous block ID
-
-        Also, detect if there are multiple votes from a single node and return them
           in a separate "cheat" dictionary.
+        * Votes must agree on previous block, otherwise they become invalid.
         """
-        by_voter = collections.defaultdict(list)
-        for vote in eligible_votes:
-            by_voter[vote['node_pubkey']].append(vote)
-
         n_valid = 0
         n_invalid = 0
         prev_blocks = collections.Counter()
         cheat = []
         malformed = []
+        prev_block = None
+
+        # Group by pubkey to detect duplicate voting
+        by_voter = collections.defaultdict(list)
+        for vote in eligible_votes:
+            by_voter[vote['node_pubkey']].append(vote)
 
         for pubkey, votes in by_voter.items():
             if len(votes) > 1:
@@ -101,20 +100,28 @@ class Voting:
             else:
                 n_invalid += 1
 
-        n_prev = prev_blocks.most_common()[0][1] if prev_blocks else 0
+        # Neutralise difference between valid block and previous block,
+        # so that nodes must agree on previous block
+        if n_valid:
+            prev_block, n_prev = prev_blocks.most_common()[0]
+            del prev_blocks[prev_block]
+            diff = n_valid - n_prev
+            n_valid -= diff
+            n_invalid += diff
 
         return {
             'counts': {
                 'n_valid': n_valid,
                 'n_invalid': n_invalid,
-                'n_agree_prev_block': n_prev,
             },
             'cheat': cheat,
             'malformed': malformed,
+            'previous_block': prev_block,
+            'other_previous_block': dict(prev_blocks),
         }
 
     @classmethod
-    def decide_votes(cls, n_voters, n_valid, n_invalid, n_agree_prev_block):
+    def decide_votes(cls, n_voters, n_valid, n_invalid):
         """
         Decide on votes.
 
@@ -124,22 +131,10 @@ class Voting:
         A tie on an even number of votes counts as INVALID so the >= operator is
         used.
         """
-
-        # Check insane cases. This is basic, not exhaustive.
-        if n_valid + n_invalid > n_voters or n_agree_prev_block > n_voters:
-            raise BigchainDBCritical('Arguments not sane: %s' % {
-                'n_voters': n_voters,
-                'n_valid': n_valid,
-                'n_invalid': n_invalid,
-                'n_agree_prev_block': n_agree_prev_block,
-            })
-
         if n_invalid * 2 >= n_voters:
             return INVALID
         if n_valid * 2 > n_voters:
-            if n_agree_prev_block * 2 > n_voters:
             return VALID
-            return INVALID
         return UNDECIDED
 
     @classmethod

tests/test_voting.py

@@ -1,6 +1,6 @@
 import pytest
+from collections import Counter
 
-from bigchaindb.backend.exceptions import BigchainDBCritical
 from bigchaindb.core import Bigchain
 from bigchaindb.voting import Voting, INVALID, VALID, UNDECIDED
 
@@ -37,21 +37,45 @@ def test_count_votes():
         def verify_vote_schema(cls, vote):
             return vote['node_pubkey'] != 'malformed'
 
-    voters = ['cheat', 'cheat', 'says invalid', 'malformed']
+    voters = (['cheat', 'cheat', 'says invalid', 'malformed'] +
-    voters += ['kosher' + str(i) for i in range(10)]
+              ['kosher' + str(i) for i in range(10)])
 
     votes = [Bigchain(v).vote('block', 'a', True) for v in voters]
     votes[2]['vote']['is_block_valid'] = False
+    # Incorrect previous block subtracts from n_valid and adds to n_invalid
     votes[-1]['vote']['previous_block'] = 'z'
 
     assert TestVoting.count_votes(votes) == {
         'counts': {
-            'n_valid': 10,
+            'n_valid': 9,    # 9 kosher votes
-            'n_invalid': 3,
+            'n_invalid': 4,  # 1 cheat, 1 invalid, 1 malformed, 1 rogue prev block
-            'n_agree_prev_block': 9
         },
         'cheat': [votes[:2]],
         'malformed': [votes[3]],
+        'previous_block': 'a',
+        'other_previous_block': {'z': 1},
+    }
+
+
+def test_must_agree_prev_block():
+    class TestVoting(Voting):
+        @classmethod
+        def verify_vote_schema(cls, vote):
+            return True
+
+    voters = 'abcd'
+    votes = [Bigchain(v).vote('block', 'a', True) for v in voters]
+    votes[0]['vote']['previous_block'] = 'b'
+    votes[1]['vote']['previous_block'] = 'c'
+    assert TestVoting.count_votes(votes) == {
+        'counts': {
+            'n_valid': 2,
+            'n_invalid': 2,
+        },
+        'previous_block': 'a',
+        'other_previous_block': {'b': 1, 'c': 1},
+        'malformed': [],
+        'cheat': [],
     }
 
 
@@ -60,16 +84,16 @@ def test_count_votes():
 
 
 DECISION_TESTS = [dict(
-    zip(['n_voters', 'n_valid', 'n_invalid', 'n_agree_prev_block'], t))
+    zip(['n_voters', 'n_valid', 'n_invalid'], t))
     for t in [
-         (1,          1,         1,           1),
+         (1,          1,         1),
-         (2,          2,         1,           2),
+         (2,          2,         1),
-         (3,          2,         2,           2),
+         (3,          2,         2),
-         (4,          3,         2,           3),
+         (4,          3,         2),
-         (5,          3,         3,           3),
+         (5,          3,         3),
-         (6,          4,         3,           4),
+         (6,          4,         3),
-         (7,          4,         4,           4),
+         (7,          4,         4),
-         (8,          5,         4,           5),
+         (8,          5,         4),
     ]
 ]
 
@@ -79,8 +103,6 @@ def test_decide_votes_valid(kwargs):
     kwargs = kwargs.copy()
     kwargs['n_invalid'] = 0
     assert Voting.decide_votes(**kwargs) == VALID
-    kwargs['n_agree_prev_block'] -= 1
-    assert Voting.decide_votes(**kwargs) == INVALID
     kwargs['n_valid'] -= 1
     assert Voting.decide_votes(**kwargs) == UNDECIDED
 
@@ -94,16 +116,67 @@ def test_decide_votes_invalid(kwargs):
     assert Voting.decide_votes(**kwargs) == UNDECIDED
 
 
-def test_decide_votes_checks_arguments():
+################################################################################
-    with pytest.raises(BigchainDBCritical):
+# Actions - test state transitions
-        Voting.decide_votes(n_voters=1, n_valid=2, n_invalid=0,
+
-                            n_agree_prev_block=0)
+
-    with pytest.raises(BigchainDBCritical):
+@pytest.mark.parametrize('n_voters', range(8))
-        Voting.decide_votes(n_voters=1, n_valid=0, n_invalid=2,
+def test_vote_actions(n_voters):
-                            n_agree_prev_block=0)
+    """
-    with pytest.raises(BigchainDBCritical):
+    * Legal transitions are UNDECIDED -> [VALID|INVALID] only
-        Voting.decide_votes(n_voters=1, n_valid=0, n_invalid=0,
+    * Block is never left UNDECIDED after voting
-                            n_agree_prev_block=2)
+    * Accomodates rogues on previous block / invalid schema
+    """
+    class TestVoting(Voting):
+        @classmethod
+        def verify_vote_schema(cls, vote):
+            return type(vote['vote']['is_block_valid']) == bool
+
+        @classmethod
+        def verify_vote_signature(cls, vote):
+            return True
+
+    keyring = 'abcdefghijklmnopqrstuvwxyz'[:n_voters]
+    block = {'id': 'block', 'block': {'voters': keyring}}
+    state = UNDECIDED
+    todo = [(state, [], [])]
+
+    def branch(p, r):
+        todo.append((state, votes, votes + [{
+            'node_pubkey': keyring[len(votes)],
+            'vote': {'previous_block': p, 'is_block_valid': r}
+        }]))
+
+    while todo:
+        prev_state, prev_votes, votes = todo.pop(0)
+        results = Counter(v['vote']['is_block_valid'] for v in votes)
+        prev_blocks = Counter(v['vote']['previous_block'] for v in votes)
+        majority = n_voters // 2 + 1
+        honest = (len(votes) == majority and len(prev_blocks) == 1 and
+                  not results['lol'] and len(results) == 1)
+        closed = len(votes) == n_voters
+
+        # Test legal transition
+        if votes:
+            state = TestVoting.block_election(block, votes, keyring)['status']
+            assert prev_state in [state, UNDECIDED]
+
+        # Test that decision has been reached
+        if honest or closed:
+            assert state != UNDECIDED or n_voters == 0
+
+        if closed:
+            continue
+
+        # Can accomodate more votes, add them to the todo list.
+        # This vote is the good case
+        branch('A', True)
+        # This vote disagrees on previous block
+        branch('B', True)
+        # This vote says the block is invalid
+        branch('A', False)
+        # This vote is invalid
+        branch('A', 'lol')
 
 
 ################################################################################
@@ -113,10 +186,6 @@ def test_decide_votes_checks_arguments():
 def test_verify_vote_signature_passes(b):
     vote = b.vote('block', 'a', True)
     assert Voting.verify_vote_signature(vote)
-
-
-def test_verify_vote_signature_fails(b):
-    vote = b.vote('block', 'a', True)
     vote['signature'] = ''
     assert not Voting.verify_vote_signature(vote)
 
@@ -128,37 +197,7 @@ def test_verify_vote_signature_fails(b):
 def test_verify_vote_schema(b):
     vote = b.vote('b' * 64, 'a' * 64, True)
     assert Voting.verify_vote_schema(vote)
-    vote = b.vote('b', 'a', True)
+    vote = b.vote('b' * 64, 'a', True)
+    assert not Voting.verify_vote_schema(vote)
+    vote = b.vote('b', 'a' * 64, True)
     assert not Voting.verify_vote_schema(vote)
-
-
-################################################################################
-# block_election tests
-# A more thorough test will follow  as part of #1217
-
-
-def test_block_election(b):
-
-    class TestVoting(Voting):
-        @classmethod
-        def verify_vote_signature(cls, vote):
-            return True
-
-        @classmethod
-        def verify_vote_schema(cls, vote):
-            return True
-
-    keyring = 'abc'
-    block = {'block': {'voters': 'ab'}}
-    votes = [{
-        'node_pubkey': c,
-        'vote': {'is_block_valid': True, 'previous_block': 'a'}
-    } for c in 'abc']
-
-    assert TestVoting.block_election(block, votes, keyring) == {
-        'status': VALID,
-        'counts': {'n_agree_prev_block': 2, 'n_valid': 2, 'n_invalid': 0},
-        'ineligible': [votes[-1]],
-        'cheat': [],
-        'malformed': [],
-    }