Fix cft election

[achamayou]

Fixes #589

In a nutshell, the change amounts to:

1. Publishing a last committable index on election, and the term of that committable index
2. Resolving the election on that basis, rather than using the last seen commit index and associated term
3. Only rolling back uncommittable transactions, rather than transactions not yet uncommitted

[ghost]

fix_cft_election@13369 aka 20200930.8 vs master ewma over 50 builds from 12767 to 13349

[jumaffre]

As recovery.py mentions #589, could you remove the check there as well?

CCF/tests/recovery.py

Lines 59 to 63 in 3ca2946

	# In theory, check_commit should be sufficient to guarantee that the new primary
	# will know about all the recovery shares submitted so far. However, because of
	# https://github.com/microsoft/CCF/issues/589, we have to wait for all nodes
	# to have committed all transactions.
	recovered_network.wait_for_all_nodes_to_catch_up(primary)

[jumaffre]

The solution in this PR seems simpler than what is described in #589 (comment). I think we should record the "why" of the changes in this PR (either in the original ticket or in this PR) as well as the "how" as this is an important change.

[achamayou]

@eddyashton pointed out that this probably isn't right, and that the election should strictly be based on committable_idx/term(committable_idx). I don't have time just now, but I'll update the PR and describe that case later.

[achamayou]

PR is up to date, please review. There are a few more tests I want to add before it's ready.

[achamayou]

Looking at the test failure, the election change invalidates our current view history reconstruction logic. In particular, we can no longer assume that commit index is in the term being replayed, and we cannot issue signature transactions until we have "caught up" with previous terms.

I am making necessary changes to address this and will change the PR status again once it is ready.

[achamayou]

raft_test is still failing because we replicate transactions marked as committable but that do not deserialise as signatures, so committable indices aren't correct on followers, and now that our term history depends on committable rather than committed indices, it's also inaccurate. Will fix tomorrow.

[achamayou]

Axed in favour of the end to end committable.py for now. The existing raft test scaffolding doesn't have a way to accurately replicate signatures at the moment.

[eddyashton]

What's the difference between suspend() and stop() here? (We suspend 2 and stop 1?)

[achamayou]

Suspend allows resuming, stop is killing the node. We do not need 3 anymore, since we want only three nodes to force a unanimous election, and it's unnecessary to operate after that. We only ever start backup 4 because we want f=2.

[eddyashton]

This kind of sleep is something we do in several places. It now potentially doesn't do what was intended - if we're in the fancy election state and Raft is not returning a signable index, we're simply skipping signatures and not producing them at regular intervals.

This is an argument in favour of continuing to produce signatures (making a max-lengthed block of transactions committable) without any global commit claim in them, but that's uglier for ledger parsing. I'm happy to keep the current behaviour (signatures are sometimes skipped) for this PR and discuss alternative approaches offline/in future.

[achamayou]

Yes that's true although that will not cause the test to silently pass.

The issue here is that we do not have a way to observe what's committable, the real condition would be, the last of those tx is committable on backup[1]. Even if we shortened the sigx-tx-interval, we wouldn't know that a signature was produced and replicated to backup[1].

[achamayou]

Reconfiguration suite failure in nightly

45: 20:22:39.960 | INFO     | reconfiguration:node_configs:17 - 200 {"0":{"address":"127.65.38.252:45699"},"1":{"address":"127.114.112.214:42871"},"2":{"address":"127.114.247.179:39983"},"3":{"add + 69 chars
45: 20:22:39.960 | ERROR    | __main__:run:88 - Test reconfiguration.test_retire_primary failed
45: Traceback (most recent call last):
45: 
45:   File "/mnt/build/2/s/tests/e2e_suite.py", line 163, in <module>
45:     run(args)
45:     │   └ Namespace(app_script=None, binary_dir='.', consensus='cft', debug_nodes=[], domain=None, enclave_type='release', enforce_reqs...
45:     └ <function run at 0x7fe830d0d1f0>
45: 
45: > File "/mnt/build/2/s/tests/e2e_suite.py", line 78, in run
45:     new_network = test(network, args)
45:                   │    │        └ Namespace(app_script=None, binary_dir='.', consensus='cft', debug_nodes=[], domain=None, enclave_type='release', enforce_reqs...
45:                   │    └ <infra.network.Network object at 0x7fe8292eb3d0>
45:                   └ <function test_retire_primary at 0x7fe8293598b0>
45: 
45:   File "/mnt/build/2/s/tests/suite/test_requirements.py", line 20, in wrapper
45:     return func(*args, **kwargs)
45:            │     │       └ {}
45:            │     └ (<infra.network.Network object at 0x7fe8292eb3d0>, Namespace(app_script=None, binary_dir='.', consensus='cft', debug_nodes=[]...
45:            └ <function test_retire_primary at 0x7fe8293599d0>
45: 
45:   File "/mnt/build/2/s/tests/suite/test_requirements.py", line 43, in wrapper
45:     return func(network, args, *nargs, **kwargs)
45:            │    │        │      │        └ {}
45:            │    │        │      └ ()
45:            │    │        └ Namespace(app_script=None, binary_dir='.', consensus='cft', debug_nodes=[], domain=None, enclave_type='release', enforce_reqs...
45:            │    └ <infra.network.Network object at 0x7fe8292eb3d0>
45:            └ <function test_retire_primary at 0x7fe829359940>
45: 
45:   File "/mnt/build/2/s/tests/reconfiguration.py", line 125, in test_retire_primary
45:     pre_count = count_nodes(node_configs(network))
45:                 │           │            └ <infra.network.Network object at 0x7fe8292eb3d0>
45:                 │           └ <function node_configs at 0x7fe8293569d0>
45:                 └ <function count_nodes at 0x7fe829356a60>
45: