Skip to content

test: Add ParSigEx end-to-end signature exchange test#468

Open
therustmonk wants to merge 1 commit into
mainfrom
kd-jun2-F1
Open

test: Add ParSigEx end-to-end signature exchange test#468
therustmonk wants to merge 1 commit into
mainfrom
kd-jun2-F1

Conversation

@therustmonk
Copy link
Copy Markdown
Collaborator

Adds the first automated end-to-end test for partial signature exchange: four nodes connect over real loopback-TCP libp2p, each signs a shared message with its own threshold-BLS share, and broadcasts the partial through the ParSigEx protocol.

One node then aggregates the threshold of partials it received over the network and verifies the result against the group public key, proving the exchange-and-aggregation path works end to end.

The test is self-contained (no relay, no external network) and reuses the existing ParSigEx behaviour as-is. Also adds pluto-crypto, pluto-testutil, and k256 as dev-dependencies.

Adds the first automated end-to-end test for partial signature exchange: four nodes connect over real loopback-TCP libp2p, each
signs a shared message with its own threshold-BLS share, and broadcasts the partial through the `ParSigEx` protocol. One node then
aggregates the threshold of partials it received over the network and verifies the result against the group public key, proving
the exchange-and-aggregation path works end to end.

The test is self-contained (no relay, no external network) and reuses the existing `ParSigEx` behaviour as-is. Also adds
`pluto-crypto`, `pluto-testutil`, and `k256` as `dev-dependencies`.
Copy link
Copy Markdown
Collaborator

@emlautarom1 emlautarom1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few things could be improved. Main issue right now is that the test swallows certain errors, and the timeout is extremely generous.

Comment on lines +309 to +314
async fn with_timeout<F, T>(future: F, msg: &'static str) -> Result<T>
where
F: Future<Output = Result<T>>,
{
timeout(TEST_TIMEOUT, future).await.context(msg)?
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I don't think this helper provides much value.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, many operations use this timeout which is quite large (30s), meaning that the full test could run for multiple minutes. Prefer a per-case timeout

Comment on lines +476 to +482
async fn broadcast_partial(
handle: Handle,
duty: Duty,
data_set: ParSignedDataSet,
) -> parsigex::Result<u64> {
handle.broadcast_and_wait(duty, data_set).await
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single usage, prefer to inline it

/// Each node signs [`MSG`] with its share and broadcasts it via ParSigEx.
fn broadcast_all(&self) -> Result<Vec<JoinHandle<parsigex::Result<u64>>>> {
let duty = Duty::new(SlotNumber::new(SLOT), DutyType::Randao);
let mut tasks = Vec::with_capacity(self.running.len());
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a JoinSet

use pluto_parsigex::{self as parsigex, DutyGater, Event, Handle, Verifier};
use pluto_testutil::random::{generate_insecure_k1_key, generate_test_bls_key};
use tokio::{
spawn,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer to not import standalone functions, instead use them with their module (ex. tokio::spawn instead of importing spawn)


/// Spawns each node's swarm loop, forwarding listen/connection/receive events.
fn spawn_nodes(bundles: Vec<NodeBundle>, sinks: &EventSinks) -> Vec<RunningNode> {
let mut running = Vec::with_capacity(bundles.len());
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use JoinSet

Comment on lines +254 to +261
/// Collects partials addressed to `receiver` until the threshold is met.
async fn collect_partials(&mut self, receiver: usize) -> Result<HashMap<u64, Signature>> {
with_timeout(
self.recv_threshold_partials(receiver),
"timed out collecting partial signatures",
)
.await
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline this with the comment, it's a single call used only once

let received = harness.collect_partials(0).await?;
ensure!(
received.len() == THRESHOLD,
"node 0 should receive exactly {THRESHOLD} partials, got {}",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it is, this ensure! is not useful:

  • We receive one partial signature at the time, until it receives THRESHOLD
  • Early return happens on panic, internal error or timeout (.await?)

Instead, the timeout should be handled inside recv_threshold_partials so that on timeout we can actually print the number of received shares (on error/panic we propagate which is fine already)

);

harness.aggregate_and_verify(&received)?;
await_broadcasts(broadcasts).await?;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is handled better by JoinSet

index: usize,
sinks: EventSinks,
mut dial_rx: mpsc::UnboundedReceiver<Vec<Multiaddr>>,
mut stop_rx: oneshot::Receiver<()>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use a CancellationToken here for consistency, and then await the join handle at the end.

Note that this is not needed, everything will be dropped at the end of the scope

Comment on lines +460 to +462
if let Ok(signature) = data.signed_data.signature() {
let _ = sinks.recv_tx.send((index, data.share_idx, signature));
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're ignoring potential errors on .signature(). We should probably propagate them at the caller, or panic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants