Add pczt frost-sign#137
Conversation
|
In starting to review this, I'm surprised that FROST signing is the first thing that was implemented, because in order to perform signing we need to have keys in the wallet database, and in order to have those keys we need the DKG process implemented. Is the DKG on your agenda for implementation in the devtool? |
As it stands, the keys need to be generated with The goal of this PR is mostly to do a proof of concept and gather feedback on the overall idea and the API. I agree that if we go forward it would be better to move to the DKG first. If you also have any feedback on the key data storage, that would be helpful. |
The overall approach that we will want is: In the wallet backend (in the Then, the signing process should be integrated with the transaction proposal -> PCZT -> transaction finalization workflow. |
|
I've opened #150 which supersedes this PR with a complete FROST implementation, integration tests, and documentation. Implementation (behind the
Tests (17 tests in
Documentation (
This addresses the DKG gap noted in the review -- all functionality from |
nullcopy
left a comment
There was a problem hiding this comment.
Hi @conradoplg I've weighed in my thoughts w.r.t. the API. Overall, I think this would be useful and valuable. I left a few minor questions/suggestions below
but if we want to go ahead it might make sense to add a new command for participants (pczt frost-sign does the coordinator logic) so that using frost-client is not required.
I agree with this
|
|
||
| let signature: [u8; 64] = signature.serialize()?.try_into().unwrap(); | ||
| let signature = redpallas::Signature::<SpendAuth>::from(signature); | ||
| signatures.push((alphas[0].0, signature)); |
There was a problem hiding this comment.
Curious why you only push alphas[0]. Don't we need one for each non-dummy output?
| /// The server URL to use. If not specified, it will use the server URL | ||
| /// for the specified group, if any. | ||
| #[arg(short, long)] | ||
| pub server_url: Option<String>, |
There was a problem hiding this comment.
I'm new to the frost signing flow, but is it possible to coordinate signatures from offline signers? Or is a connection to a coordinator server strictly necessary?
Pardon my ignorance 😅
|
|
||
| let mut comms = frost_client::coordinator::comms::http::HTTPComms::new(&pargs, &args) | ||
| .map_err(|e| anyhow!(e.to_string()))?; | ||
| let signature = frost_client::coordinator::cli::coordinator(&mut comms, pargs) |
There was a problem hiding this comment.
I'm curious how timeouts/retries work here. If one signer becomes unavailable or fails to contribute in time, is it possible to resume a signing round or do all parties need to start over?
| }) | ||
| .collect::<Result<Vec<_>, _>>()?, | ||
| }; | ||
| let args = frost_client::coordinator::comms::http::Args { |
There was a problem hiding this comment.
There's lots of manual struct assembling in this PoC. I would prefer to have constructors where possible
This is just a proof of concept to gather feedback. The code is ugly; I'm looking for feedback on (1) is the API something acceptable? and (2) is this something that could be possibly merged after cleaning up?
I documented how to run it here: https://frost.zfnd.org/zcash/devtool-demo.html
Regarding the API, currently it is supposed to work along the
frost-clientCLI for the participants (and thefrost-serverin the same repo to handle comms), but if we want to go ahead it might make sense to add a new command for participants (pczt frost-signdoes the coordinator logic) so that usingfrost-clientis not required.Regarding what is currently missing:
frost-clientconfig file which stores everything in cleartext)