[reconfiguration] Notify checkpoint service of last checkpoint #6585
Conversation
| @@ -2294,7 +2293,20 @@ impl AuthorityState { | |||
| // | |||
| // Only after CheckpointService::notify_checkpoint stores checkpoint in it's store we update checkpoint boundary | |||
| if let Some((index, roots)) = self.database.last_checkpoint(round)? { | |||
| checkpoint_service.notify_checkpoint(index, roots, false)?; | |||
| let final_checkpoint_round = self.database.final_epoch_checkpoint()?; | |||
There was a problem hiding this comment.
So there will be some period of gap between when we receive 2f+1 EndOfPublish and here. What happens in that period?
There was a problem hiding this comment.
We might have more narwhal transactions in the commit, and we will process them. Remaining certificates will be rejected, other messages like checkpoint signatures will be processed as usual
|
|
||
| /// Records narwhal consensus output index of the final checkpoint in epoch | ||
| /// This is a single entry table with key FINAL_EPOCH_CHECKPOINT_INDEX | ||
| final_epoch_checkpoint: DBMap<u64, u64>, |
There was a problem hiding this comment.
Do you need final_epoch_checkpoint? Could you derive it from the current checkpoint-to-be-built under the condition that all certs are closed?
There was a problem hiding this comment.
I do need this, since we continue to process narwhal commits even after receiving 2f+1 EndOfPublish
There was a problem hiding this comment.
Thinking more, I think this table could be removed in the future, after some of the other refactoring is done, namely when we merge #6305 and move all of narwhal commit processing into single batch(if it is even possible). With what we have now however, I think we have to live with it
There was a problem hiding this comment.
worth adding some comments here
#5763