Repository navigation
bft: pass the WAL truncation flag explicitly - #740
Conversation
HagarMeir
left a comment
HagarMeir
left a comment
There was a problem hiding this comment.
The refactor looks behaviour-preserving to me: every call site passes the same flag the old GetProposedRecord() != nil check produced. The comments below are about tests and comments, not correctness.
Written by Claude (AI assistant) on behalf of Hagar Meir.
| vc.Stop() | ||
| reqTimer.AssertCalled(t, "RestartTimers") | ||
| state.AssertCalled(t, "Save", mock.Anything) | ||
| state.AssertCalled(t, "Save", mock.Anything, mock.Anything) |
There was a problem hiding this comment.
All the mocks now accept mock.Anything for the flag, so no test checks the value a caller passes. For example, flipping view.go:414 to false, or the commit save to true, would still pass the view/viewchanger/controller tests. Could some of these assert the actual bool?
Written by Claude on behalf of Hagar Meir.
| } | ||
|
|
||
| func (sr *StateRecorder) Save(message *protos.SavedMessage) error { | ||
| func (sr *StateRecorder) Save(message *protos.SavedMessage, _ bool) error { |
There was a problem hiding this comment.
Maybe record the flag here (e.g. a TruncateTo []bool next to SavedMessages). Then the existing view and viewchanger tests that check SavedMessages could also check where the truncation points fall.
Written by Claude on behalf of Hagar Meir.
| truncateTo: []bool{true, false}, | ||
| }, | ||
| { | ||
| description: "a new view with nothing in flight is a truncation point", |
There was a problem hiding this comment.
These two cases say a new view with nothing in flight truncates and one with a proposal in flight doesn't, but Save ignores proposalInFlight. It just passes through the flag the case gives it, and every new-view save in production passes false. As written, the test describes behaviour from a later change, and it would still pass if that change got it wrong.
Written by Claude on behalf of Hagar Meir.
| newProposal := msgToSave.GetProposedRecord() != nil | ||
| // TODO: handle view message here as well, and add "|| finalizedView" to truncate flag | ||
| return ps.WAL.Append(b, newProposal) | ||
| // The caller decides whether this record is a truncation point. It knows |
There was a problem hiding this comment.
This says the caller decides based on whether a proposal is still held and whether the view has stopped. In this PR, every caller passes a fixed literal that depends only on the message type. Maybe word it for what the code does now?
Written by Claude on behalf of Hagar Meir.
| // the view that could append after this record has stopped. Computing the | ||
| // flag here from the shared in-flight state would make it depend on which | ||
| // goroutine happens to change that state first. | ||
| return ps.WAL.Append(b, truncateTo) |
There was a problem hiding this comment.
The removed comment explained when truncating is safe (after a pre-prepare, or after a new view with 2f+1 attestations) and had the new-view TODO. Now that the decision lives at the call sites, it would help to keep that reasoning in the code, e.g. on the State interface.
Written by Claude on behalf of Hagar Meir.
| // Save saves a message. truncateTo tells the WAL whether everything written | ||
| // before the message can be dropped. The caller decides it, because only the | ||
| // caller knows whether the node still has a proposal to recover. | ||
| Save(message *protos.SavedMessage, truncateTo bool) error |
There was a problem hiding this comment.
Same point as in state.go: none of the callers checks whether there is a proposal to recover yet, so this describes a contract nothing implements today.
Written by Claude on behalf of Hagar Meir.
| ProposedRecord: &protos.ProposedRecord{ | ||
| PrePrepare: &protos.PrePrepare{ | ||
| Proposal: &protos.Proposal{ | ||
| Header: []byte{1}, |
There was a problem hiding this comment.
Nit: these fixtures nearly copy proposedRecord/preparedProof in TestStateRestore, and the commit record is written again in the next test. Shared helpers next to newViewRecord would avoid that.
Written by Claude on behalf of Hagar Meir.
16de864 to
46a5e56
Compare
fixed |
PersistedState.Save used to derive the truncation flag from the saved message type. The caller knows the recovery state, so pass the flag explicitly. This commit only moves the decision to the caller and keeps the existing semantics: a pre-prepare truncates, a commit never does, and view records do not truncate. Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>
46a5e56 to
9590cf9
Compare
PersistedState.Save used to derive the truncation flag from the saved message type. The caller knows the recovery state, so pass the flag explicitly. This commit only moves the decision to the caller and keeps the existing semantics: a pre-prepare truncates, a commit never does, and view records do not truncate.
After that, I plan two more pull requests.
#736