From f858fc8f8d006176e39c264717021609f7d4079d Mon Sep 17 00:00:00 2001 From: Scott Sadler Date: Sat, 25 Feb 2017 10:21:40 +0100 Subject: [PATCH] fix voting bug with prev_block --- bigchaindb/voting.py | 53 +++++++------- tests/test_voting.py | 167 ++++++++++++++++++++++++++----------------- 2 files changed, 127 insertions(+), 93 deletions(-) diff --git a/bigchaindb/voting.py b/bigchaindb/voting.py index f8511644..67741bb5 100644 --- a/bigchaindb/voting.py +++ b/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 - 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. + * 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 VALID return UNDECIDED @classmethod diff --git a/tests/test_voting.py b/tests/test_voting.py index ea61fa6a..404f4c93 100644 --- a/tests/test_voting.py +++ b/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 += ['kosher' + str(i) for i in range(10)] + voters = (['cheat', 'cheat', 'says invalid', 'malformed'] + + ['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_invalid': 3, - 'n_agree_prev_block': 9 + 'n_valid': 9, # 9 kosher votes + 'n_invalid': 4, # 1 cheat, 1 invalid, 1 malformed, 1 rogue prev block }, '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), - (2, 2, 1, 2), - (3, 2, 2, 2), - (4, 3, 2, 3), - (5, 3, 3, 3), - (6, 4, 3, 4), - (7, 4, 4, 4), - (8, 5, 4, 5), + (1, 1, 1), + (2, 2, 1), + (3, 2, 2), + (4, 3, 2), + (5, 3, 3), + (6, 4, 3), + (7, 4, 4), + (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): - Voting.decide_votes(n_voters=1, n_valid=2, n_invalid=0, - n_agree_prev_block=0) - with pytest.raises(BigchainDBCritical): - Voting.decide_votes(n_voters=1, n_valid=0, n_invalid=2, - n_agree_prev_block=0) - with pytest.raises(BigchainDBCritical): - Voting.decide_votes(n_voters=1, n_valid=0, n_invalid=0, - n_agree_prev_block=2) +################################################################################ +# Actions - test state transitions + + +@pytest.mark.parametrize('n_voters', range(8)) +def test_vote_actions(n_voters): + """ + * Legal transitions are UNDECIDED -> [VALID|INVALID] only + * Block is never left UNDECIDED after voting + * 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': [], - }