Implement Verifier's Deposit Finalize handler #423

mmtftr · 2025-01-17T08:17:10Z

Description

Created a new deposit_signatures table with primary key of (outpt, op_idx)
Implemented deposit finalize handler:

Verifies all incoming signatures,
Saves to DB
Signs Move_TX and returns partial_sig

Linked Issues

Closes Implement Verifier Deposit Finalize #354

Testing

Currently no tests since I don't have a complete understanding of the business logic here.

Warning

Need to add tests before merge

Created a new deposit_signatures table with primary key of (outpt, op_idx) Implemented deposit finalize handler: - Verifies all incoming signatures, - Saves to DB - Signs Move_TX and returns partial_sig

core/src/rpc/verifier.rs

Some code cleaning and documentation improvements: - Macro for impl TryFrom<Vec> for byte array-backed structs - Rustdoc on new_deposit and inner comments - Naming improvements Features: - Split verified sigs into per operator arrays and save with operator_idx - Collect +1 nonce for movetx but only sign at the final stage Fixes: - Some panics were replaced with errors

…t signing

mmtftr · 2025-01-20T12:58:24Z

There are some issues with the backend causing tests to fail. Other than that, I think it's ready for review.

ozankaymak · 2025-01-20T13:15:12Z

Please resolve the conflicts before review.

…lize

mmtftr

Questions to the open

mmtftr · 2025-01-20T13:26:32Z

core/src/rpc/aggregator.rs

 async fn nonce_aggregator(
    mut nonce_streams: Vec<
        impl Stream<Item = Result<MusigPubNonce, BridgeError>> + Unpin + Send + 'static,
    >,
    mut sighash_stream: impl Stream<Item = Result<TapSighash, BridgeError>> + Unpin + Send + 'static,
    agg_nonce_sender: Sender<AggNonceQueueItem>,
-) -> Result<(), BridgeError> {
-    while let Ok(sighash) = sighash_stream.next().await.transpose() {


I believe previous behavior was incorrect. Ok(None) means stream end here, but it would've caused a panic due to ok_or below. Also, an error in the stream would've just broken out of the loop, which is incorrect behavior.

I fixed this along with adding better logging.

mmtftr · 2025-01-20T13:27:46Z

core/src/rpc/aggregator.rs

        let nonce_dist_handle = tokio::spawn(nonce_distributor(
            agg_nonce_receiver,
            partial_sig_streams,
            partial_sig_sender,
        ));
+
+        // Start the signature aggregation pipe.
        let sig_agg_handle = thread::spawn(|| {


thread::spawn and tokio::spawn are mixed here, was there a reason why? I think we can just use tokio::spawn everywhere, as it's a multi-threaded Runtime anyway.

It's because aggregation tasks are CPU dependent while distributor tasks are IO dependent. IO dependent tasks are opted for a Tokio "thread" to reduce memory usage and improve startup time. Tried using Tokio's spawn and spawn_blockings but they failed to run 2 aggregation tasks in parallel, as far as I remember.

Also, we need all these tasks to run at the "same time" to achieve pipeline. That is the reason we spawn other Tokio runtimes inside this async function.

I'm not sure if that's related, there really shouldn't be such a big difference unless there is a bug in Tokio itself. We can write a test for this to make sure it works properly

On a related note, I saw some problems with the mutex lock being held for a long time (i.e. for most of the duration of the function) in some functions. I believe I fixed one of these in the PR but we may benefit from testing whether locks are being held for too long to the point they'd reduce parallelism.

In this function's case, holding the sessions lock would block any other aggregator from running for any other session.

mmtftr · 2025-01-20T13:28:18Z

core/src/rpc/aggregator.rs

        ));

-        nonce_agg_handle.join().unwrap().unwrap();
+        // Wait for all pipeline tasks to complete
        try_join_all(vec![nonce_dist_handle]).await.unwrap();
        sig_agg_handle.join().unwrap().unwrap();
        try_join_all(vec![sig_dist_handle]).await.unwrap();


I don't think these try_join_all are necessary. We can simply await, everything i spawned anyways.

Yes, it looks like try_join_all calls are unnecessary. Can be replaced with smthng like nonce_dist_handle.await.unwrap().unwrap().

mmtftr · 2025-01-20T13:31:28Z

core/src/rpc/verifier.rs

+        u32,
+    ),
+    Status,
+> {


maybe even TryFrom<(DepositSignSession, usize)> for (...result tuple...)

But usize wouldn't be clear that it's actually a verifier_idx.

I was thinking of doing some impls on Prost-generated types for ease of use and code quality, I think we can attempt to do it after the gRPC API stabilizes further

Sure, maybe we can create an issue for that to not forget?

mmtftr · 2025-01-20T14:32:35Z

Please resolve the conflicts before review.

Conflicts have been resolved, waiting for review

ekrembal · 2025-01-20T15:34:07Z

core/src/rpc/clementine.proto

@@ -142,12 +144,11 @@ message NonceGenRequest {
 }

 message NonceGenFirstResponse {
+  reserved 3; // 3 = sig, previously used for signing new keypairs
  // Nonce ID
  uint32 id = 1;
  // New public key for the deposit
  bytes public_key = 2;


We don't need public key here

ekrembal · 2025-01-20T15:36:18Z

core/src/rpc/clementine.proto

@@ -142,12 +144,11 @@ message NonceGenRequest {
 }

 message NonceGenFirstResponse {
+  reserved 3; // 3 = sig, previously used for signing new keypairs


We don't need to be backward compatible now

ekrembal · 2025-01-20T15:36:51Z

core/src/rpc/verifier.rs

+        u32,
+    ),
+    Status,
+> {


Sure, maybe we can create an issue for that to not forget?

ekrembal · 2025-01-20T15:37:36Z

core/src/rpc/verifier.rs

@@ -393,6 +398,7 @@ impl ClementineVerifier for Verifier {
        &self,
        req: Request<Streaming<VerifierDepositFinalizeParams>>,
    ) -> Result<Response<PartialSig>, Status> {
+        use clementine::verifier_deposit_finalize_params::Params;


Why use here?

easier to read and parse for the eyes below. These params are only relevant for this function, so we can put them in scope to make it easier to read below.

Implement Verifier's Deposit Finalize handler

8356cd9

Created a new deposit_signatures table with primary key of (outpt, op_idx) Implemented deposit finalize handler: - Verifies all incoming signatures, - Saves to DB - Signs Move_TX and returns partial_sig

mmtftr requested review from ekrembal and ozankaymak January 17, 2025 08:18

mmtftr self-assigned this Jan 17, 2025

mmtftr added this to the Deposit Signature Collection milestone Jan 17, 2025

Refactor deposit parameter parsing in verifier

bad5441

ekrembal requested changes Jan 17, 2025

View reviewed changes

core/src/rpc/verifier.rs Show resolved Hide resolved

core/src/rpc/verifier.rs Outdated Show resolved Hide resolved

core/src/rpc/verifier.rs Outdated Show resolved Hide resolved

mmtftr added 2 commits January 20, 2025 14:01

Move back to central keypair instead of per-session keypair in deposi…

5fe1d1b

…t signing

mmtftr marked this pull request as ready for review January 20, 2025 12:58

Merge remote-tracking branch 'origin/dev' into feat/verifier-dep-fina…

1bfab09

…lize

mmtftr mentioned this pull request Jan 20, 2025

Failed test due to DatabaseError (rpc::aggregator::tests::aggregator_setup_and_deposit) #437

Open

mmtftr requested a review from ekrembal January 20, 2025 13:32

mmtftr commented Jan 20, 2025

View reviewed changes

ekrembal approved these changes Jan 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Verifier's Deposit Finalize handler #423

Implement Verifier's Deposit Finalize handler #423

mmtftr commented Jan 17, 2025 •

edited

Loading

mmtftr commented Jan 20, 2025

ozankaymak commented Jan 20, 2025

mmtftr left a comment

mmtftr Jan 20, 2025

mmtftr Jan 20, 2025

ceyhunsen Jan 20, 2025

mmtftr Jan 20, 2025

mmtftr Jan 20, 2025

ceyhunsen Jan 20, 2025

mmtftr Jan 20, 2025

ekrembal Jan 20, 2025

mmtftr commented Jan 20, 2025

ekrembal Jan 20, 2025

ekrembal Jan 20, 2025

ekrembal Jan 20, 2025

ekrembal Jan 20, 2025

mmtftr Jan 20, 2025

Implement Verifier's Deposit Finalize handler #423

Are you sure you want to change the base?

Implement Verifier's Deposit Finalize handler #423

Conversation

mmtftr commented Jan 17, 2025 • edited Loading

Description

Linked Issues

Testing

mmtftr commented Jan 20, 2025

ozankaymak commented Jan 20, 2025

mmtftr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmtftr commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmtftr commented Jan 17, 2025 •

edited

Loading