Key reshare addition #950
Key reshare addition #950
Conversation
JesseAbram
requested review from
ameba23 and
HCastano
and removed request for
ameba23
JesseAbram
added
the
Bump `spec_version`
| Channels(broadcast_out, rx_from_others) | ||
| }; | ||
|
|
||
| let session = make_key_resharing_session( |
There was a problem hiding this comment.
This breaks the convention we have with the other protocols of calling this function in ./crates/protocol/src/execute_protocol.rs - so i would maybe move it there. That said i am thinking to do a refactor of entropy-protocol soon so could leave it till then.
There was a problem hiding this comment.
yes tbh there needs to be a refactor a little because it can mainly fit the same code as proactive refresh for individual dkg keys but that assumes all parties have the keys unlike this one, so a refactor should remove all or most this code, happy to wait
| @@ -160,7 +160,7 @@ pub enum SessionId { | |||
| /// A distributed key generation protocol session for registering | |||
| Dkg { user: AccountId32, block_number: u32 }, | |||
| /// A proactive refresh session | |||
| ProactiveRefresh { verifying_key: Vec<u8>, block_number: u32 }, | |||
| Reshare { verifying_key: Vec<u8>, block_number: u32 }, | |||
There was a problem hiding this comment.
It seems strange to remove the term 'proactive refresh' here but keep it elsewhere.
My proposal would be to call it reshare but include an Option with the new_signer field from OcwMessageReshare. Including that in the session Id ensures everyone is in agreement, and we can do a reshare with or without a change in the signing committee. (Whether we choose to do refreshes without a change is another question but i would like to keep the option open).
There was a problem hiding this comment.
yes but I kinda wanna push on and handle this in the refactor
| @@ -201,7 +208,53 @@ pub mod pallet { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| /// Submits a request to perform a proactive refresh to the threshold servers. | |||
There was a problem hiding this comment.
If keeping the fn below why not also keep the doccomment
| let mut user_names_and_verifying_keys = | ||
| vec![("eve", hex::encode(EVE_VERIFYING_KEY)), ("dave", hex::encode(DAVE_VERIFYING_KEY))]; | ||
| if add_parent_key { | ||
| user_names_and_verifying_keys.push(("eve", hex::encode(NETWORK_PARENT_KEY))) |
There was a problem hiding this comment.
For clarity, we use a second copy of eve's keyshares as the parent key.
I guess at some point we will remove per-user keyshares and this will be the only pre-configured keyshare.
There was a problem hiding this comment.
yup pretty much, why at a new key, If no objections Im happy with keeping less code here
| .map_err(|_| ValidatorErr::ProtocolError("Error executing protocol".to_string()))? | ||
| .0 | ||
| .ok_or(ValidatorErr::NoOutputFromReshareProtocol)?; | ||
| let _serialized_key_share = key_serialize(&new_key_share) |
There was a problem hiding this comment.
Just for clarity - new_key_share is of type Option<ThresholdKeyShare<...>>, so we serialize it here even if it is None, and we don't yet store it. I guess if it is none we will here remove our network parent keyshare if we have one as that would mean we are no longer in the signing committee.
There was a problem hiding this comment.
if it is none, don't we hit the error on line 200?
| @@ -61,14 +61,14 @@ async fn get_hkdf(kv: &KvManager) -> Result<Hkdf<Sha256>, UserErr> { | |||
| } | |||
|
|
|||
| /// Given a mnemonic, setup hkdf | |||
| fn get_hkdf_from_mnemonic(mnemonic: &str) -> Result<Hkdf<Sha256>, UserErr> { | |||
| pub fn get_hkdf_from_mnemonic(mnemonic: &str) -> Result<Hkdf<Sha256>, UserErr> { | |||
There was a problem hiding this comment.
I wouldn't make these public if you only need them in tests, rather use get_signer_and_x25519_secret_from_mnemonic below.
| ) | ||
| .await; | ||
| for response_result in response_results { | ||
| assert_eq!(response_result.unwrap().text().await.unwrap(), ""); |
There was a problem hiding this comment.
Just for clarity, this checks that the protocol runs successfully, but does not (yet) check for a change in the signing committee since the updated keyshares don't yet get stored.
There was a problem hiding this comment.
yup, next PR, have ideas of how this should be done.....but ya next PR
| // important: the header->Content-Type must be added and match that of the receiving | ||
| // party!! | ||
| let pending = http::Request::post(url, vec![req_body.encode()]) | ||
| .deadline(deadline) |
There was a problem hiding this comment.
I know this is exactly the same as the proactive refresh deadline, and i think we've discussed this before.
IIRC the missing the deadline doesn't really make any difference as if this fn returns an error nothing bad happens. But a deadline of two seconds to run the reshare protocol seems quite short.
There was a problem hiding this comment.
yup will just log an error on the chain logs, no big deal
Co-authored-by: peg <[email protected]>
Related #941