[DONT MERGE] CI for hotfix #1178

ekrembal · 2025-10-06T17:21:29Z

No description provided.

Copilot

Pull Request Overview

Improve streaming error propagation and MuSig2 signing pipeline robustness; add optional partial-signature verification and enhanced monitoring. Key updates:

Change streaming channels to carry Result types and wire monitor_standalone_task to forward task errors to clients.
Carry public nonces alongside aggregated nonces and partial signatures; update aggregation to accept (PartialSignature, PublicNonce) and optionally verify partial signatures.
Strengthen aggregator pipeline with tighter counting, error handling, and task join reporting; various error message improvements.
Note: a safety check in header_chain_prover for genesis state hash is commented out.

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
core/src/verifier.rs	Stream partial sigs as Result, forward task errors via monitor, improved premature end message
core/src/utils.rs	monitor_standalone_task now forwards task errors to a provided channel
core/src/test/musig2.rs	Adjust tests to pass (PartialSignature, PublicNonce) tuples
core/src/rpc/verifier.rs	Propagate monitor errors, clarify error contexts, handle Result-wrapped partial sigs
core/src/rpc/parser/verifier.rs	Clearer parse error messages
core/src/rpc/parser/mod.rs	Optional-message macro returns Option directly
core/src/rpc/operator.rs	Monitor error forwarding; operator deposit_sign sends Result-wrapped sigs
core/src/rpc/aggregator.rs	Carry/route public nonces; stricter counts; improved error handling and logging; API adjustments
core/src/musig2.rs	aggregate_partial_signatures now accepts (PartialSignature, PublicNonce); optional per-partial verification
core/src/header_chain_prover.rs	Commented out genesis chain state hash validation (risk)
core/src/errors.rs	New ErrorExt methods; changed BridgeError -> Status mapping to always internal

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-06T17:27:25Z

core/src/rpc/aggregator.rs

+        while let Some((queue_item, pub_nonces)) = queue_rx.recv().await {
+            let pub_nonces_ref = pub_nonces.as_slice();
            let partial_sigs = try_join_all(partial_sig_rx.iter_mut().enumerate().map(
                |(idx, stream)| async move {


pub_nonces_ref[idx] has type &PublicNonce, but the tuple expects a PublicNonce, causing a type mismatch. Use a value (copy/clone or deref) instead of a reference.

Copilot · 2025-10-06T17:27:25Z

core/src/rpc/aggregator.rs

                    )
+                    .wrap_err("Failed to parse partial signature")?;
+
+                    Ok::<_, BridgeError>((partial_sig, pub_nonces_ref[idx]))


pub_nonces_ref[idx] has type &PublicNonce, but the tuple expects a PublicNonce, causing a type mismatch. Use a value (copy/clone or deref) instead of a reference.

Suggested change

Ok::<_, BridgeError>((partial_sig, pub_nonces_ref[idx]))

Ok::<_, BridgeError>((partial_sig, pub_nonces_ref[idx].clone()))

Copilot · 2025-10-06T17:27:25Z

core/src/rpc/aggregator.rs

+    if total_sigs != needed_nofn_sigs {
+        let err_msg = format!(
+            "Expected {} nofn signatures, got {} from sighash stream",
+            needed_nofn_sigs, total_sigs
+        );
+        tracing::error!(err_msg);
+        return Err(eyre::eyre!(err_msg).into());
    }


tracing::error!(err_msg) does not format the message as intended. Use tracing::error!(%err_msg) or tracing::error!("{}", err_msg) to emit the string. The same pattern appears at lines 290–291 and 366–367 below; please update those too.

Copilot · 2025-10-06T17:27:26Z

core/src/musig2.rs

+                "MuSig2 Error: partial signature verification failed for verifiers: {}",
+                partial_sig_verification_errors.join(", ")
+            );
+            tracing::error!(error_msg);


tracing::error!(error_msg) won’t render the message; use tracing::error!(%error_msg) or tracing::error!("{}", error_msg) to log the string.

Suggested change

tracing::error!(error_msg);

tracing::error!("{}", error_msg);

Copilot · 2025-10-06T17:27:26Z

core/src/header_chain_prover.rs

+            // let genesis_chain_state_hash = genesis_chain_state.to_hash();
+            // if genesis_chain_state_hash != config.protocol_paramset().genesis_chain_state_hash {
+            //     return Err(eyre::eyre!(
+            //         "Genesis chain state hash mismatch: {} != {}",
+            //         hex::encode(genesis_chain_state_hash),
+            //         hex::encode(config.protocol_paramset().genesis_chain_state_hash)
+            //     )
+            //     .into());
+            // }


Genesis chain state hash validation is commented out, removing an important correctness/safety check. Please restore the validation to ensure the prover is anchored to the expected genesis parameters.

Suggested change

// let genesis_chain_state_hash = genesis_chain_state.to_hash();

// if genesis_chain_state_hash != config.protocol_paramset().genesis_chain_state_hash {

// return Err(eyre::eyre!(

// "Genesis chain state hash mismatch: {} != {}",

// hex::encode(genesis_chain_state_hash),

// hex::encode(config.protocol_paramset().genesis_chain_state_hash)

// )

// .into());

// }

let genesis_chain_state_hash = genesis_chain_state.to_hash();

if genesis_chain_state_hash != config.protocol_paramset().genesis_chain_state_hash {

return Err(eyre::eyre!(

"Genesis chain state hash mismatch: {} != {}",

hex::encode(genesis_chain_state_hash),

hex::encode(config.protocol_paramset().genesis_chain_state_hash)

)

.into());

}

Copilot · 2025-10-06T17:27:27Z

core/src/rpc/verifier.rs

+            )
+            .await
+            .wrap_err_with(|| {
+                format!("While waiting for {nonce_idx} + 1th sig out of {num_required_nofn_sigs} ")


The message has a stray space and reads '+ 1th'. Consider: format!("While waiting for signature {} of {}", nonce_idx + 1, num_required_nofn_sigs) for a clearer error.

Suggested change

format!("While waiting for {nonce_idx} + 1th sig out of {num_required_nofn_sigs} ")

format!(

"While waiting for signature {} of {}",

nonce_idx + 1,

num_required_nofn_sigs

)

Copilot · 2025-10-06T17:27:27Z

core/src/rpc/aggregator.rs

 }

-/// For each expected sighash, we collect a batch of public nonces from all verifiers. We aggregate and send to the agg_nonce_sender. Then repeat for the next sighash.
+/// For each expected sighash, we collect a batch of public nonces from all verifiers. We aggregate and send aggregatod nonce and all public nonces (needed for partial signature verification) to the agg_nonce_sender. Then repeat for the next sighash.


Corrected spelling of 'aggregatod' to 'aggregated'.

Suggested change

/// For each expected sighash, we collect a batch of public nonces from all verifiers. We aggregate and send aggregatod nonce and all public nonces (needed for partial signature verification) to the agg_nonce_sender. Then repeat for the next sighash.

/// For each expected sighash, we collect a batch of public nonces from all verifiers. We aggregate and send aggregated nonce and all public nonces (needed for partial signature verification) to the agg_nonce_sender. Then repeat for the next sighash.

Copilot · 2025-10-06T17:27:27Z

core/src/rpc/aggregator.rs

 }

-/// Reroutes aggregated nonces to the signature aggregator.
+/// Reroutes aggregated nonces and public nonces for each aggregatedd nonce to the signature aggregator.


Corrected spelling of 'aggregatedd' to 'aggregated'.

Suggested change

/// Reroutes aggregated nonces and public nonces for each aggregatedd nonce to the signature aggregator.

/// Reroutes aggregated nonces and public nonces for each aggregated nonce to the signature aggregator.

Copilot · 2025-10-06T17:27:28Z

core/src/rpc/aggregator.rs

+        if nonce_count != needed_nofn_sigs {
+            let err_msg = format!("Expected {needed_nofn_sigs} aggregated nonces in nonce_distributor, got {nonce_count}",);
+            tracing::error!(err_msg);
+            return Err(eyre::eyre!(err_msg).into());
        }


As with earlier occurrences, use tracing::error!(%err_msg) or tracing::error!("{}", err_msg) so the error message is rendered.

Copilot · 2025-10-06T17:27:28Z

core/src/rpc/aggregator.rs

+            let err_msg = format!(
+                "Expected {needed_nofn_sigs} partial signatures in nonce_distributor, got {sig_count}",
+            );
+            tracing::error!(err_msg);


Use tracing::error!(%err_msg) or tracing::error!("{}", err_msg) to log the formatted string; otherwise it won’t be printed as intended.

Suggested change

tracing::error!(err_msg);

tracing::error!("{}", err_msg);

This reverts commit 431b3de.

Copilot

Pull Request Overview

Copilot reviewed 17 out of 26 changed files in this pull request and generated 4 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-06T19:05:48Z

core/src/musig2.rs

+    let partial_sigs_and_nonces: Vec<&(PartialSignature, PublicNonce)> =
+        partial_sigs.iter().collect();
+    let partial_sigs: Vec<&PartialSignature> =
+        partial_sigs_and_nonces.iter().map(|(sig, _)| sig).collect();
+    // enable partial signature verification with an environment variable to see which verifier is giving bad partial signatures


This double-iter pattern yields &&(PartialSignature, PublicNonce) and destructuring with |(sig, _)| won’t match; it will fail to compile or infer types incorrectly. Build the references directly from the function parameter: let partial_sigs: Vec<&PartialSignature> = partial_sigs.iter().map(|(sig, _)| sig).collect();

Copilot · 2025-10-06T19:05:48Z

core/src/rpc/aggregator.rs

                    )
+                    .wrap_err("Failed to parse partial signature")?;
+
+                    Ok::<_, BridgeError>((partial_sig, pub_nonces_ref[idx]))


You return (PartialSignature, PublicNonce) but pass pub_nonces_ref[idx], which is &PublicNonce. This mismatches the expected owned type. Use clone() (or deref if Copy) to produce a PublicNonce: Ok::<_, BridgeError>((partial_sig, pub_nonces_ref[idx].clone())).

Suggested change

Ok::<_, BridgeError>((partial_sig, pub_nonces_ref[idx]))

Ok::<_, BridgeError>((partial_sig, pub_nonces_ref[idx].clone()))

Copilot · 2025-10-06T19:05:48Z

core/src/utils.rs

    }
 }

 /// Monitors a [`tokio::task::JoinHandle`] in the background and logs it's end


Use the possessive 'its' instead of 'it's'.

Suggested change

/// Monitors a [`tokio::task::JoinHandle`] in the background and logs it's end

/// Monitors a [`tokio::task::JoinHandle`] in the background and logs its end

Copilot · 2025-10-06T19:05:49Z

circuits-lib/src/bridge_circuit/lc_proof.rs

+    env::verify(LC_IMAGE_ID, &light_client_proof.lc_journal).unwrap();

-    let light_client_circuit_output = deserialize_circuit_output(&light_client_proof.lc_journal);
+    let light_client_circuit_output: LightClientCircuitOutput =
+        borsh::from_slice(light_client_proof.lc_journal.as_slice())
+            .expect("Failed to deserialize light client circuit output");


Avoid unwrap() here; use expect with a clear message (as you already do for deserialization) so failures include context, or consider returning a Result from this function instead of panicking.

atacann and others added 17 commits October 2, 2025 12:33

do not give metrics error if fetching stopped tasks fails

cf52a55

init better logging

ca4847d

do not return error from optional message fetcher

2870736

fmt

a67ef17

change log level

9fd73bd

check number sent messages in aggregator rpc streams, log errors

c18b2b4

change comment

5218bf6

catch more errors

af8cb5f

fix wrong num needed sigs issue

301dcf6

better errors on nonce aggregation

bfb3d8e

add partial signature verification

eb5843f

verifty partial sigs only when env var is true

1a4edb9

change var naming

0f14130

update docs

a77117d

add logging

f360533

show all partial verify errors, not only the first

5984323

ignore genesis chain state hash

91c689d

Copilot AI review requested due to automatic review settings October 6, 2025 17:21

Copilot AI reviewed Oct 6, 2025

View reviewed changes

Revert "Add Regtest Check for Lcp Verification (#1032)"

c7d28eb

This reverts commit 431b3de.

github-actions bot added the F-deposit-replace-needed Flag: Deposit replacements are needed. This will change Txids. label Oct 6, 2025

Fix incorrect regtest check

918efe1

Copilot AI review requested due to automatic review settings October 6, 2025 19:00

Copilot AI reviewed Oct 6, 2025

View reviewed changes

	Ok::<_, BridgeError>((partial_sig, pub_nonces_ref[idx]))
	Ok::<_, BridgeError>((partial_sig, pub_nonces_ref[idx].clone()))

	tracing::error!(error_msg);
	tracing::error!("{}", error_msg);

	/// For each expected sighash, we collect a batch of public nonces from all verifiers. We aggregate and send aggregatod nonce and all public nonces (needed for partial signature verification) to the agg_nonce_sender. Then repeat for the next sighash.
	/// For each expected sighash, we collect a batch of public nonces from all verifiers. We aggregate and send aggregated nonce and all public nonces (needed for partial signature verification) to the agg_nonce_sender. Then repeat for the next sighash.

	/// Reroutes aggregated nonces and public nonces for each aggregatedd nonce to the signature aggregator.
	/// Reroutes aggregated nonces and public nonces for each aggregated nonce to the signature aggregator.

	/// Monitors a [`tokio::task::JoinHandle`] in the background and logs it's end
	/// Monitors a [`tokio::task::JoinHandle`] in the background and logs its end

[DONT MERGE] CI for hotfix #1178

Are you sure you want to change the base?

[DONT MERGE] CI for hotfix #1178

Uh oh!

Conversation

ekrembal commented Oct 6, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants