Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
249 changes: 145 additions & 104 deletions Cargo.lock

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
[workspace]
members = [
"crates/app",
"crates/parsigex",
"crates/build-proto",
"crates/cli",
"crates/cluster",
Expand Down Expand Up @@ -99,6 +100,7 @@ wiremock = "0.6"

# Crates in the workspace
pluto-app = { path = "crates/app" }
pluto-parsigex = { path = "crates/parsigex" }
pluto-build-proto = { path = "crates/build-proto" }
pluto-cli = { path = "crates/cli" }
pluto-cluster = { path = "crates/cluster" }
Expand Down
8 changes: 8 additions & 0 deletions crates/core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ cancellation.workspace = true
chrono.workspace = true
crossbeam.workspace = true
futures.workspace = true
futures-timer.workspace = true
dyn-clone.workspace = true
dyn-eq.workspace = true
hex.workspace = true
Expand All @@ -31,18 +32,25 @@ tokio-util.workspace = true
tracing.workspace = true
pluto-eth2util.workspace = true
tree_hash.workspace = true
unsigned-varint.workspace = true

[dev-dependencies]
anyhow.workspace = true
alloy.workspace = true
clap.workspace = true
rand.workspace = true
libp2p.workspace = true
k256.workspace = true
prost.workspace = true
prost-types.workspace = true
hex.workspace = true
chrono.workspace = true
test-case.workspace = true
pluto-eth2util.workspace = true
pluto-cluster.workspace = true
pluto-p2p.workspace = true
pluto-testutil.workspace = true
pluto-tracing.workspace = true
Comment on lines +38 to +53
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.

are these newly added dependencies in use?

tokio = { workspace = true, features = ["test-util"] }
wiremock.workspace = true
pluto-ssz.workspace = true
Expand Down
4 changes: 4 additions & 0 deletions crates/core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ pub mod deadline;
/// parsigdb
pub mod parsigdb;

mod parsigex_codec;

pub use parsigex_codec::ParSigExCodecError;

/// Test utilities.
#[cfg(test)]
pub mod testutils;
118 changes: 118 additions & 0 deletions crates/core/src/parsigex_codec.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
//! Partial signature exchange codec helpers used by core types.

use std::any::Any;

use crate::{
signeddata::{
Attestation, BeaconCommitteeSelection, SignedAggregateAndProof, SignedRandao,
SignedSyncContributionAndProof, SignedSyncMessage, SignedVoluntaryExit,
SyncCommitteeSelection, VersionedAttestation, VersionedSignedAggregateAndProof,
VersionedSignedProposal, VersionedSignedValidatorRegistration,
},
types::{DutyType, Signature, SignedData},
};

/// Error type for partial signature exchange codec operations.
#[derive(Debug, thiserror::Error)]
pub enum ParSigExCodecError {
/// Missing duty or data set fields.
#[error("invalid parsigex msg fields")]
InvalidMessageFields,

/// Invalid partial signed data set proto.
#[error("invalid partial signed data set proto fields")]
InvalidParSignedDataSetFields,

/// Invalid partial signed proto.
#[error("invalid partial signed proto")]
InvalidParSignedProto,
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.

no usage


/// Invalid duty type.
#[error("invalid duty")]
InvalidDuty,

/// Unsupported duty type.
#[error("unsupported duty type")]
UnsupportedDutyType,

/// Deprecated builder proposer duty.
#[error("deprecated duty builder proposer")]
DeprecatedBuilderProposer,

/// Failed to parse a public key.
#[error("invalid public key: {0}")]
InvalidPubKey(String),

/// Invalid share index.
#[error("invalid share index")]
InvalidShareIndex,

/// Serialization failed.
#[error("marshal signed data: {0}")]
Serialize(#[from] serde_json::Error),
}

pub(crate) fn serialize_signed_data(data: &dyn SignedData) -> Result<Vec<u8>, ParSigExCodecError> {
Comment thread
varex83agent marked this conversation as resolved.
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.

Bug: JSON-only codec — Go uses SSZ-first serialization since v0.17, causing a hard interoperability failure

serialize_signed_data and deserialize_signed_data use serde_json exclusively. Go's marshal() (charon/core/proto.go:266) tries ssz.Marshaler first, falling back to JSON. Go's unmarshal() tries SSZ then JSON. Since v0.17, sszMarshallingEnabled = true by default.

Affected types that Go writes as SSZ: VersionedSignedProposal, Attestation, VersionedAttestation, SignedAggregateAndProof, VersionedSignedAggregateAndProof, SignedSyncMessage, SignedSyncContributionAndProof.

A Go node will write SSZ; the Rust node will fail to decode it, returning InvalidPayload and silently dropping the partial signature. This is a complete interoperability failure for those duty types.

Fix: implement SSZ-first serialization/deserialization for all SignedData types that implement SSZ in Go, matching the marshal/unmarshal logic in charon/core/proto.go:264-304.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is addressed in #322

let any = data as &dyn Any;

macro_rules! serialize_as {
($ty:ty) => {
if let Some(value) = any.downcast_ref::<$ty>() {
return Ok(serde_json::to_vec(value)?);
}
};
}

serialize_as!(Attestation);
serialize_as!(VersionedAttestation);
serialize_as!(VersionedSignedProposal);
serialize_as!(VersionedSignedValidatorRegistration);
serialize_as!(SignedVoluntaryExit);
serialize_as!(SignedRandao);
serialize_as!(Signature);
serialize_as!(BeaconCommitteeSelection);
serialize_as!(SignedAggregateAndProof);
serialize_as!(VersionedSignedAggregateAndProof);
serialize_as!(SignedSyncMessage);
serialize_as!(SyncCommitteeSelection);
serialize_as!(SignedSyncContributionAndProof);

Err(ParSigExCodecError::UnsupportedDutyType)
}

pub(crate) fn deserialize_signed_data(
duty_type: &DutyType,
bytes: &[u8],
) -> Result<Box<dyn SignedData>, ParSigExCodecError> {
macro_rules! deserialize_json {
($ty:ty) => {
serde_json::from_slice::<$ty>(bytes)
.map(|value| Box::new(value) as Box<dyn SignedData>)
.map_err(ParSigExCodecError::from)
};
}

match duty_type {
// Match Go order: old Attestation format first, then VersionedAttestation.
DutyType::Attester => deserialize_json!(Attestation)
.or_else(|_| deserialize_json!(VersionedAttestation))
.map_err(|_| ParSigExCodecError::UnsupportedDutyType),
DutyType::Proposer => deserialize_json!(VersionedSignedProposal),
DutyType::BuilderProposer => Err(ParSigExCodecError::DeprecatedBuilderProposer),
DutyType::BuilderRegistration => deserialize_json!(VersionedSignedValidatorRegistration),
DutyType::Exit => deserialize_json!(SignedVoluntaryExit),
DutyType::Randao => deserialize_json!(SignedRandao),
DutyType::Signature => deserialize_json!(Signature),
DutyType::PrepareAggregator => deserialize_json!(BeaconCommitteeSelection),
// Match Go order: old SignedAggregateAndProof format first, then versioned.
DutyType::Aggregator => deserialize_json!(SignedAggregateAndProof)
.or_else(|_| deserialize_json!(VersionedSignedAggregateAndProof))
.map_err(|_| ParSigExCodecError::UnsupportedDutyType),
DutyType::SyncMessage => deserialize_json!(SignedSyncMessage),
DutyType::PrepareSyncContribution => deserialize_json!(SyncCommitteeSelection),
DutyType::SyncContribution => deserialize_json!(SignedSyncContributionAndProof),
DutyType::Unknown | DutyType::InfoSync | DutyType::DutySentinel(_) => {
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: DutyType::InfoSync rejection is correct (matches Go's default branch in ParSignedDataFromProto) but has no comment explaining the intent. A future maintainer might assume this is a missing case. Add:

// InfoSync is not used in parsigex in Go (handled via a separate channel);
// Unknown and DutySentinel are sentinel/invalid values that are never transmitted.
DutyType::Unknown | DutyType::InfoSync | DutyType::DutySentinel(_) => {
    Err(ParSigExCodecError::UnsupportedDutyType)
}

Err(ParSigExCodecError::UnsupportedDutyType)
}
}
}
3 changes: 3 additions & 0 deletions crates/core/src/signeddata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ pub enum SignedDataError {
/// Invalid attestation wrapper JSON.
#[error("unmarshal attestation")]
AttestationJson,
/// Custom error.
#[error("{0}")]
Custom(Box<dyn std::error::Error>),
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.

Major: SignedDataError::Custom wraps Box<dyn Error> without Send + Sync — breaks async usage

Box<dyn std::error::Error> does not require Send or Sync. SignedDataError is returned by SignedData::signature(), which is called inside the async parsigex pipeline (handler.rs) and in TryFrom impls used across .await points in spawned tasks.

Holding a Custom variant across an .await point in a tokio::spawn task will either fail to compile or silently constrain concrete error types to Send + Sync.

Fix:

Custom(Box<dyn std::error::Error + Send + Sync>),

}

fn hash_root<T: TreeHash>(value: &T) -> [u8; 32] {
Expand Down
145 changes: 142 additions & 3 deletions crates/core/src/types.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
//! Types for the Charon core.

use std::{collections::HashMap, fmt::Display, iter};
use std::{any::Any, collections::HashMap, fmt::Display, iter};

use chrono::{DateTime, Duration, Utc};
use dyn_clone::DynClone;
use dyn_eq::DynEq;
use serde::{Deserialize, Serialize};
use std::fmt::Debug as StdDebug;

use crate::signeddata::SignedDataError;
use crate::{
ParSigExCodecError,
corepb::v1::core as pbcore,
parsigex_codec::{deserialize_signed_data, serialize_signed_data},
signeddata::SignedDataError,
};

/// The type of duty.
#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]
Expand Down Expand Up @@ -66,6 +71,52 @@ impl DutyType {
}
}

impl From<&DutyType> for i32 {
fn from(duty_type: &DutyType) -> Self {
match duty_type {
DutyType::Unknown => 0,
DutyType::Proposer => 1,
DutyType::Attester => 2,
DutyType::Signature => 3,
DutyType::Exit => 4,
DutyType::BuilderProposer => 5,
DutyType::BuilderRegistration => 6,
DutyType::Randao => 7,
DutyType::PrepareAggregator => 8,
DutyType::Aggregator => 9,
DutyType::SyncMessage => 10,
DutyType::PrepareSyncContribution => 11,
DutyType::SyncContribution => 12,
DutyType::InfoSync => 13,
DutyType::DutySentinel(_) => 14,
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.

Bug: DutySentinel serializes as 14 but TryFrom<i32> has no arm for 14 — round-trip is broken

From<&DutyType> for i32 maps DutySentinel(_) => 14 (here), but TryFrom<i32> for DutyType falls through to _ => Err(ParSigExCodecError::InvalidDuty) at line 115 — 14 is never matched.

In Go, dutySentinel = 14 is an unexported constant used only as a loop bound and is never transmitted on the wire. The Rust DutySentinel variant should not be serializable to the wire format at all.

Fix: guard against encoding DutySentinel before it reaches From<&DutyType> for i32 (return an error), and change TryFrom<i32> to return Err(UnsupportedDutyType) (not InvalidDuty) for value 14 to distinguish "recognised but unsupported" from "not a valid duty type".

}
}
}

impl TryFrom<i32> for DutyType {
type Error = ParSigExCodecError;

fn try_from(value: i32) -> Result<Self, Self::Error> {
match value {
0 => Ok(DutyType::Unknown),
1 => Ok(DutyType::Proposer),
2 => Ok(DutyType::Attester),
3 => Ok(DutyType::Signature),
4 => Ok(DutyType::Exit),
5 => Ok(DutyType::BuilderProposer),
6 => Ok(DutyType::BuilderRegistration),
7 => Ok(DutyType::Randao),
8 => Ok(DutyType::PrepareAggregator),
9 => Ok(DutyType::Aggregator),
10 => Ok(DutyType::SyncMessage),
11 => Ok(DutyType::PrepareSyncContribution),
12 => Ok(DutyType::SyncContribution),
13 => Ok(DutyType::InfoSync),
_ => Err(ParSigExCodecError::InvalidDuty),
}
}
}

/// SlotNumber struct
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)]
pub struct SlotNumber(u64);
Expand Down Expand Up @@ -192,6 +243,28 @@ impl Duty {
}
}

impl From<&Duty> for pbcore::Duty {
fn from(duty: &Duty) -> Self {
Self {
slot: duty.slot.inner(),
r#type: i32::from(&duty.duty_type),
}
}
}

impl TryFrom<&pbcore::Duty> for Duty {
type Error = ParSigExCodecError;

fn try_from(duty: &pbcore::Duty) -> Result<Self, Self::Error> {
let duty_type = DutyType::try_from(duty.r#type)?;
if !duty_type.is_valid() {
return Err(ParSigExCodecError::InvalidDuty);
}

Ok(Self::new(duty.slot.into(), duty_type))
}
}

/// The type of proposal.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)]
#[serde(rename_all = "snake_case")]
Expand Down Expand Up @@ -452,7 +525,7 @@ impl AsRef<[u8; SIG_LEN]> for Signature {
}

/// Signed data type
pub trait SignedData: DynClone + DynEq + StdDebug + Send + Sync {
pub trait SignedData: Any + DynClone + DynEq + StdDebug + Send + Sync {
/// signature returns the signed duty data's signature.
fn signature(&self) -> Result<Signature, SignedDataError>;

Expand Down Expand Up @@ -517,6 +590,39 @@ impl ParSignedData {
}
}

impl TryFrom<&ParSignedData> for pbcore::ParSignedData {
type Error = ParSigExCodecError;

fn try_from(data: &ParSignedData) -> Result<Self, Self::Error> {
let encoded = serialize_signed_data(data.signed_data.as_ref())?;
let share_idx =
i32::try_from(data.share_idx).map_err(|_| ParSigExCodecError::InvalidShareIndex)?;
let signature = data.signed_data.signature().map_err(|err| {
ParSigExCodecError::Serialize(serde_json::Error::io(std::io::Error::other(
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.

Major: roundabout error conversion through serde_json::Error::io for a non-I/O failure

ParSigExCodecError::Serialize(serde_json::Error::io(std::io::Error::other(err.to_string())))

A SignedDataError from signature() is being tunnelled through: string → io::Errorserde_json::ErrorParSigExCodecError::Serialize. This is semantically wrong — a signature extraction failure is not a serialization error.

Fix: add a dedicated variant:

// in parsigex_codec.rs ParSigExCodecError:
#[error("extract signature: {0}")]
ExtractSignature(String),

And map to it directly: .map_err(|e| ParSigExCodecError::ExtractSignature(e.to_string())).

err.to_string(),
)))
})?;

Ok(Self {
data: encoded.into(),
signature: signature.as_ref().to_vec().into(),
share_idx,
})
}
}

impl TryFrom<(&DutyType, &pbcore::ParSignedData)> for ParSignedData {
type Error = ParSigExCodecError;

fn try_from(value: (&DutyType, &pbcore::ParSignedData)) -> Result<Self, Self::Error> {
let (duty_type, data) = value;
let share_idx =
u64::try_from(data.share_idx).map_err(|_| ParSigExCodecError::InvalidShareIndex)?;
let signed_data = deserialize_signed_data(duty_type, &data.data)?;
Ok(Self::new_boxed(signed_data, share_idx))
}
}

/// ParSignedDataSet is a set of partially signed duty data only signed by a
/// single threshold BLS share.
#[derive(Debug, Clone, PartialEq, Eq, Default)]
Expand Down Expand Up @@ -554,6 +660,39 @@ impl ParSignedDataSet {
}
}

impl TryFrom<&ParSignedDataSet> for pbcore::ParSignedDataSet {
type Error = ParSigExCodecError;

fn try_from(set: &ParSignedDataSet) -> Result<Self, Self::Error> {
let mut out = std::collections::BTreeMap::new();
for (pub_key, value) in set.inner() {
out.insert(pub_key.to_string(), pbcore::ParSignedData::try_from(value)?);
}

Ok(Self { set: out })
}
}

impl TryFrom<(&DutyType, &pbcore::ParSignedDataSet)> for ParSignedDataSet {
type Error = ParSigExCodecError;

fn try_from(value: (&DutyType, &pbcore::ParSignedDataSet)) -> Result<Self, Self::Error> {
let (duty_type, set) = value;
if set.set.is_empty() {
return Err(ParSigExCodecError::InvalidParSignedDataSetFields);
}

let mut out = Self::new();
for (pub_key, value) in &set.set {
let pub_key = PubKey::try_from(pub_key.as_str())
.map_err(|_| ParSigExCodecError::InvalidPubKey(pub_key.clone()))?;
out.insert(pub_key, ParSignedData::try_from((duty_type, value))?);
}

Ok(out)
}
}

/// SignedDataSet is a set of signed duty data.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct SignedDataSet<T: SignedData>(HashMap<PubKey, T>);
Expand Down
Loading
Loading