Approximates code from algorithmic description#1235
Conversation
Co-authored-by: Sergio Mena <sergio@informal.systems>
Co-authored-by: Sergio Mena <sergio@informal.systems>
sergio-mena
left a comment
There was a problem hiding this comment.
Thanks for this @lasarojc, looking great!
cason
left a comment
There was a problem hiding this comment.
I think the changes are legit, but I have two main comments:
- We are changing how the protocol works. In the case we locked on a block (so we have that block) in a previous round, and receive a Proposal from the current round for the locked block ID, plus the associated 2/3+ Prevotes, we are not issuing a Precommit for that (locked) block if we didn't receive the full proposed block again (the same block, but with updated round).
- I wonder why the above mentioned situation is not covered by a test unit. The test unit would also serve as a way to document the protocol operation change we are introducing here. I still think that the ideal procedure should be to first create a test that fails, then update the code so that the test succeeds.
I am aware that creating and updating test units for consensus is a nightmare. But I think that, if we don't have that test, we should at least clearly describe in the PR the change in the scenario mentioned above (and others, if it is the case).
| logger.Error("failed publishing event lock", "err", err) | ||
| } | ||
| } else { | ||
| if err := cs.eventBus.PublishEventRelock(cs.RoundStateEvent()); err != nil { |
There was a problem hiding this comment.
We are not re-locking in this case, right? We are unlocking and then locking a different value. I wonder whether the event we are producing should be the same. Actually, changing a lock is something unusual, I kind of information that should probably be highlighted on the logs and parsed in a different way.
There was a problem hiding this comment.
Indeed we are changing the locked value, which I considered to be a relocking.
We can add another event type to handle this case or send two events in sequence, unlock and lock.
|
There is a test which is failining: |
|
As a suggestion for test units to be considered/updated: |
|
The scenario that I mentioned in previous comments is tested in PR #1257. |
|
We are closing this PR without merging, as it would revert an optimization that is in place, even though the optimization is for the non-happy path and, as stated in the PR, causes the code to slightly diverge from the pseudo-code. |
lines 36 to 43 of the algorithm are broken into two separate conditions to detect if a value is being relocked.
Relocking is a subclass of locking and restructuring the case as such brings the code closer to the algorithm, while keeping the distinction in the logging.
PR checklist
.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments