Use checkpoint sigs from batches without waiting for a commit #12880
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 6 Ignored Deployments
|
| // All checkpoint sigs have been verified, forward them to the checkpoint service | ||
| for ckpt in ckpt_messages { | ||
| self.checkpoint_service | ||
| .notify_checkpoint_signature(&self.epoch_store, &ckpt)?; |
There was a problem hiding this comment.
Unlike in consensus_handler, we have not verified that the sender matches the name in the checkpoint sig. Is this important? If so, I need some advice on how to find the originator of the batch here.
There was a problem hiding this comment.
To be honest it don't seem that important to me - after all it's a valid signature, sure it's suspicious/odd that a node is distributing someone else's signature but I don't see a security issue or something. The batch it self doesn't contain the originator, so we'll need to inject that , off the top of my head, via two places:
- when it gets broadcasted by the source
- when is requested from the source
but it might not worth the effort
There was a problem hiding this comment.
Yeah, I think since we checked signature validity, it does not matter who sent it
| @@ -1791,6 +1791,9 @@ impl AuthorityPerEpochStore { | |||
| kind: ConsensusTransactionKind::CheckpointSignature(info), | |||
| .. | |||
| }) => { | |||
| // We usually call notify_checkpoint_signature in SuiTxValidator, but that step can | |||
| // be skipped when a batch is already part of a certificate, so we must also | |||
There was a problem hiding this comment.
nit: since this file is part of Sui, maybe specify these are Narwhal batch and Narwhal certificate to avoid confusion with Sui certificate.
mystenmark
force-pushed
the
mlogan-checkpoint-sig-latency
branch
from
fbbacb8 to
fbf9898
Compare
On a local cluster this appears to reduce checkpoint_summary_age_ms p50 from ~2000ms to ~600ms
On a local cluster this appears to reduce checkpoint_summary_age_ms p50 from ~2000ms to ~600ms