Retain all signatures during a soft rollback #5749
Conversation
|
Excellent find!
I think of "soft rollback" as "rollback_uncommittable_suffix()" and "hard rollback" as "rollback_conflicting_suffix()", which can roll back some committable (but obviously not committed) entries as well. |
|
soft_rollback_retains_signatures@77781 aka 20231025.14 vs main ewma over 20 builds from 77052 to 77722 Click to see tablemain
soft_rollback_retains_signatures
|
I found 4 other uses:
Summing that up, I think it's viable to remove |
This was unsafe, and a small extension (87495bf) to the new scenario makes this clear. All of those new asserts failed when the vote functions above used I've made the further changes described in the comment above, and removed |
|
One of the changes above was too aggressive, and several tests are now failing. Will investigate that shortly. Right now I'm trying to get the new scenario to Trace Validate correctly. First it fails at an I think this should be removed - its not a safety requirement, and we can produce valid scenarios/traces consisting purely of adjacent signatures. However, I thought this could also be trivially worked around by adding a I think the problem is this: Attempting to use |
@eddyashton Need/want help with this one? |
|
After discussion with @achamayou, we've gone for a simpler fix for this case. We've removed the Its feasible to track the "committable because they're in my term" entries by construction, but there are several edge cases to consider including a potential liveness-of-commit-knowledge gap. Rather than trying to unpick these, and coming up with appropriate naming to distinguish the cases, its simpler and cleaner to make a small modification to the commit logic to rediscover the value there. In short, remembering all signatures means we definitely have the information we need, and any clearance of this structure is tricky to confirm as safe. |
…ton/CCF into soft_rollback_retains_signatures
Following #5674 (comment).
Includes a scenario that reproduces the failure (and will act as a regression test going forward), and a modification to
raft.hto resolve the failure.This issue was introduced by my change in #3971. Prior to that,
committable_indiceshad meant "all signature transactions", and solast_committable_index()was "last signature". To avoid committing entries from prior terms, we changed the meaning ofcommittable_indicesto mean only "signatures from the current term". Uses oflast_committable_index()which required the last signature (such as in this "soft rollback") were now incorrect."Soft rollback" is not a term we use in the code, but I think is a helpful concept here. We call
rollback()in 2 scenarios: when we become aware of a new term, and when we receive an AppendEntries which conflicts with our ledger. The latter is a "hard rollback", in that it can remove any entry up to our commit index - we have a signal from a primary that this is safe to discard, and discarding may be necessary to resync with the primary. The former is a much softer signal, and all we intend to do here is drop unsigned state so that we reason purely about signed state in future.Opening as a draft to gauge test coverage, and while I triage other uses of
last_committable_index().