feat(bridge-withdrawer): support FROST threshold signing #1948
Conversation
noot
force-pushed
the
noot/bridge-withdrawer
branch
from
8fd627b to
cb67d38
Compare
noot
requested review from
SuperFluffy,
Fraser999,
a team and
joroshiba
as code owners
There was a problem hiding this comment.
Let's do 1-1-1 here https://protobuf.dev/best-practices/1-1-1/ (going to try doing this for the entire mono repo moving forward).
Essentially this means a single file per item.
| bytes participant_identifier = 2; | ||
| } | ||
|
|
||
| message Part1Request {} |
There was a problem hiding this comment.
Should we call this Part1Request or Round1Request? I am not sure what the standard frost nomenclature is, but I see that for example frost_ed25519 uses round*: https://docs.rs/frost-ed25519/latest/frost_ed25519/round1/index.html
(Unrelated bikeshed: I have a preference for writing out numbers as in PartOneRequest because Part1 and Part2 only have a levenshtein distance of 1 whereas PartOne and PartTwo have a distance of 3....; it's easier to see the difference and also grep for it when going through the code).
|
|
||
| message Part1Request {} | ||
|
|
||
| message Part1Response { |
There was a problem hiding this comment.
Similar thoughts as for Part1Request above.
| uint32 request_identifier = 2; | ||
| } | ||
|
|
||
| message Part2Request { |
There was a problem hiding this comment.
Similar thoughts as for Part1Request above.
| uint32 request_identifier = 3; | ||
| } | ||
|
|
||
| message Part2Response { |
There was a problem hiding this comment.
Similar thoughts as for Part1Request above.
| async fn frost_part_1( | ||
| &self, | ||
| ) -> ( | ||
| Vec<(Identifier, axum::body::Bytes, u32)>, |
There was a problem hiding this comment.
Can you give this a structured type as the return value? Both for the elements inside the Vec as well as the (Vec, BTreeMap) overall.
| Vec<(Identifier, axum::body::Bytes, u32)>, | ||
| BTreeMap<Identifier, round1::SigningCommitments>, | ||
| ) { | ||
| let stream = futures::stream::FuturesUnordered::new(); |
There was a problem hiding this comment.
I am noticing that id is the element by which you are tracking failure as well as the return value. Maybe a tokio JoinMap is more straight forward here?
| @@ -71,13 +76,20 @@ impl BridgeWithdrawer { | |||
| /// # Errors | |||
| /// | |||
| /// - If the provided `api_addr` string cannot be parsed as a socket address. | |||
| pub fn new(cfg: Config, metrics: &'static Metrics) -> eyre::Result<(Self, ShutdownHandle)> { | |||
| pub async fn new( | |||
There was a problem hiding this comment.
I have a strong preference for this remaining sync (not async).
BridgeWithdrawer already has a setup phase during which async steps are run.
| sequencer_key_path: String, | ||
| sequencer_address_prefix: String, | ||
| ) -> eyre::Result<Arc<dyn Signer>> { | ||
| let signer: Arc<dyn Signer> = if frost_threshold_signing_enabled { |
There was a problem hiding this comment.
Noted elsewhere - I think making this an enum (static dispatch) rather than a trait (dynamic dispatch) might end up being cleaner and easier to understand.
| .wrap_err("failed to deserialize public key package")?; | ||
|
|
||
| let participant_clients = | ||
| initialize_frost_participant_clients(frost_participant_endpoints, &public_key_package) |
There was a problem hiding this comment.
This should be done in the init/startup phase of bridge withdrawer.
Summary
support FROST ed25519 threshold signing in the bridge withdrawer.
Background
improves security of the withdrawer over having one private key used; instead, we can employ an m-of-n threshold signature scheme to sign withdrawals.
Changes
FrostParticipantServicegRPC proto definition, which is to be implemented by a binary which contains a FROST secret key partial that participates in the threshold signing processSignertraitFrostSignerwhich has gRPC clients for the signing participantsFrostSigner.sign()performs the signing process by calling each participant for their commitment (part 1) and signature share (part 2) and finally aggregates the shares to create a valid signature.FrostSigner.sign()will fail if it does not receive responses from at least the minimum number of signers required. the min number of signers is determined during key generation, which is done completely separately.Testing
blackbox tests with mock signer servers have been added.
also, this was tested with a 2/2 threshold signer node setup. withdrawal transactions are successfully signed, submitted, and included on the sequencer. see https://www.notion.so/astria-org/bridge-threshold-withdrawer-testing-1936bd31a90c803ab33ccc1221f545a1?pvs=4
Changelogs
Changelogs updated.
Related Issues
closes #1937