[checkpoint v2] Submit checkpoint signatures to consensus #6021
Conversation
| @@ -436,6 +436,12 @@ impl CheckpointProposalContents { | |||
| } | |||
| } | |||
|
|
|||
| /// This is a message validators publish to consensus in order to sign checkpoint | |||
| #[derive(Clone, Debug, Serialize, Deserialize, Hash)] | |||
| pub struct SignedCheckpointData { | |||
There was a problem hiding this comment.
I don't quite understand why you need a wrapper of SignedCheckpointSummary?
There was a problem hiding this comment.
Mostly because I am planning to add more data there, more specifically consensus integrity hash, so that we can have some alerting when it diverges (we wanted this for a while now)
There was a problem hiding this comment.
In that case, wouldn't you need to also sign this data structure? Otherwise anyone could just replace the rest of the fields in this data structure and send it to consensus.
There was a problem hiding this comment.
The plan is to make sure others can't submit checkpoint messages on your behalf.
Next PR has specific TODO for it: https://github.com/MystenLabs/sui/pull/6032/files#diff-ed86b8167f6985d5e9921bba3b99e5d28ba89a2168f5c071b968ee695d93d974R366 (see checkpoints2/mod.rs:366)
There was a problem hiding this comment.
I still don't get it: to verify that you can just compare the checkpoint summary signature with cert author. There is no need for any other data.
crates/sui-types/src/messages.rs
Outdated
| @@ -2026,6 +2028,16 @@ impl ConsensusTransaction { | |||
| } | |||
| } | |||
|
|
|||
| pub fn new_checkpoint_signature_message(data: SignedCheckpointData) -> Self { | |||
| let mut hasher = DefaultHasher::new(); | |||
| data.hash(&mut hasher); | |||
There was a problem hiding this comment.
It's probably not necessary to hash the entire thing. I think you just need to hash the signature since it's a also commit to the hash?
andll
force-pushed
the
andrey-58
branch
4 times, most recently
from
cf35747 to
d7387ce
Compare
| data.verify(&self.committee.load()).map_err(|err|{ | ||
| warn!( | ||
| "Ignoring malformed checkpoint signature (failed to verify) from {}, sequence {}: {:?}", | ||
| transaction.consensus_output.certificate.header.author, data.summary.summary.sequence_number, err |
There was a problem hiding this comment.
i don't know how author will be formatted here, but validator names in sui are insanely long so we added a .concise() method to them for concise logs. Up to you if you want to change this, just fyi.
There was a problem hiding this comment.
It seems that at least on this particular author field there is no concise method :(
2330 | transaction.consensus_output.certificate.header.author.concise(), data.summary.summary.sequence_number, err
| ^^^^^^^ method not found in `fastcrypto::bls12381::min_sig::BLS12381PublicKey`
| @@ -436,6 +436,12 @@ impl CheckpointProposalContents { | |||
| } | |||
| } | |||
|
|
|||
| /// This is a message validators publish to consensus in order to sign checkpoint | |||
| #[derive(Clone, Debug, Serialize, Deserialize, Hash)] | |||
| pub struct SignedCheckpointData { | |||
There was a problem hiding this comment.
In that case, wouldn't you need to also sign this data structure? Otherwise anyone could just replace the rest of the fields in this data structure and send it to consensus.
|
Removed |
andll
force-pushed
the
andrey-58
branch
2 times, most recently
from
829dbf1 to
cc5610f
Compare
This PR signs and submit checkpoint to consensus so it can be later aggregated into a certified checkpoint. #5763
This PR signs and submit checkpoint to consensus so it can be later aggregated into a certified checkpoint.
#5763