Playback via ledger entries #721
Conversation
|
playback_via_ae@4481 aka 20200203.3 vs master ewma over 30 builds from 4059 to 4477 |
Codecov Report
@@ Coverage Diff @@
## master #721 +/- ##
==========================================
- Coverage 79.17% 72.79% -6.38%
==========================================
Files 153 153
Lines 11638 11638
==========================================
- Hits 9214 8471 -743
- Misses 2424 3167 +743
|
| @@ -337,8 +337,17 @@ namespace ccf | |||
| std::shared_ptr<enclave::RpcContext> ctx, | |||
There was a problem hiding this comment.
Minor: it seems that the comment above is now out-of-date?
| } | ||
| case kv::DeserialiseSuccess::PASS_SIGNATURE: | ||
| { | ||
| throw std::logic_error( |
There was a problem hiding this comment.
I wonder if the kv should return a similar status when it has deserialised a Pre_Prepare transaction (in my mental model, PBFT's Pre-prepares are roughly Raft's signatures). Then you can remove playback_transaction() from Replica.cpp and call playback_request() and playback_pre_prepare() directly from here, which I think is neater?
There was a problem hiding this comment.
That would entail having the kv look for ccf.signature table and ccf.pbft.preprepares table and return PASS_SIGNATURE status. I am weary of semantically mixing up pre-prepares with signatures. A pre-prepare is not necessarily signed and therefore it will not necessarily globally commit. The code will work, but the semantics will be off.
There was a problem hiding this comment.
We can return PASS_PRE_PREPARE from deserialise to keep the two enum different.
There was a problem hiding this comment.
Sure if we can agree on that I can add the PASS_PRE_PREPARE to DeserialiseSuccess and there will be no need for the playback_transaction as you said
There was a problem hiding this comment.
Happy with this once the KV reserved stuff is fixed. As discussed, could you also create a ticket or write a TODO in the code so that we can think of making the late_joiners.py test part of the existing end-to-end suite as we have all the building blocks for that already?
…t, typo and comment addition
…back implementation
Resolves #694 and #693
Status messages are used to detect if a replica is so far behind that it can not catch up by message re transmission. In that case the Replica will send an
Append_entries(new type) message. This will be intercepted byPbftand will send entries from the ledger.Pbftkeeps the ledger index every time something is written to the ledger via replicate. This is needed since version, seqno, and ledger index's do not match in any way. Also it keeps track of the ledger index when a seqno is marked as stable. This is needed because when a replica joins very late, we want to send ledger entries up until our last seqno that was marked as stable. From there we can send messages that are stored in the replica (who is helping the late joiner catch up) state.When a ledger entry is received it will be deserialised and then passed to the Replica for execution. Entries should come in the sequence of: request(s) and then pre-prepare. Playbacks will also take the transaction that was populated when
Store::deserialisewas called, and it will be used to execute the request or write the pre-prepare to the ledger.As replicas might be playing-back and receiving out of order pre-prepares/prepares/commits, we need to reject those pre-prepares/prepares/commits if they are old.
Opening as a draft since I need to look at things a bit more closely and possibly cleanup-refactor some bits here and there
Before opening as a normal PR I need to add the batching method for the append entries size (same as raft)