-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: distribute public keys #123
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I would wait for @DavidM-D's approval that this is what he had in mind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
It's a bit strange that we are setting PK set separately from other info about nodes. But it will work for now. Maybe this logic will be executed exactly one time :/
So... how it will work in dev env? Is it automated? |
@volovyks what do you mean? Signer nodes still need to know their key pairs statically (i.e. they are generated outside of the application). Leader node makes a bunch of requests on startup to collect all public keys and then distributes them across the signer nodes. |
@itegulov ok, cool then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from one comment, LGTM
mpc-recovery/src/sign_node/mod.rs
Outdated
|
||
let mut public_keys = state.node_info.nodes_public_keys.write().await; | ||
tracing::debug!("Setting node public keys => {:?}", request.public_keys); | ||
public_keys.replace(request.public_keys); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these keys should only be set in a situation where there are no public keys on the node currently. I don't think we want nodes changing signing groups with payloads in flight.
Also what happens if a sign node restart, do they lose their public keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, let me store them in Datastore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Resolves #115.
Opened up to test for now since local dev isn't working on macos right nowCreates two new endpoints
/public_key_node
and/accept_pk_set
in sign nodes.public_key_node
: fetches the public key for the sign node.accept_pk_set
: sign node accepts a list of public keys to be set.Currently leader node will aggregate all public keys from sign nodes, then send them to each sign node separately.
/accept_pk_set
can also be used to set the keys manually too if needed.