diff --git a/crates/consensus/src/qbft/component.rs b/crates/consensus/src/qbft/component.rs index f3638186..99fddd94 100644 --- a/crates/consensus/src/qbft/component.rs +++ b/crates/consensus/src/qbft/component.rs @@ -976,42 +976,59 @@ pub(crate) mod tests { assert_eq!(err.to_string(), "value hash not found in values"); } + // Parity with charon core/consensus/qbft/msg.go newMsg @ v1.7.1: an + // absent, all-zero, or non-32-byte value hash is admitted and collapses to + // the nil hash. Whether a nil value is acceptable for a given message type + // is decided by the generic core's justification rules, not at receive + // time. #[test_case(vec![] ; "empty")] #[test_case(vec![0; 32] ; "zero")] #[test_case(vec![1; 31] ; "short")] #[test_case(vec![1; 33] ; "long")] #[tokio::test] - async fn handle_rejects_invalid_value_hash(hash: Vec) { + async fn handle_admits_malformed_value_hash_as_nil(hash: Vec) { + let consensus = consensus(0, true); let mut msg = unsigned_msg(0); msg.value_hash = hash.into(); let msg = sign_for_peer(msg, 0); + let inst = consensus.get_instance_io(duty()); - let err = consensus(0, true) + consensus .handle(consensus_msg(msg), &CancellationToken::new()) .await - .unwrap_err(); + .unwrap(); - assert_eq!(err.to_string(), "invalid value hash"); + let mut recv_rx = inst.take_recv_rx().unwrap(); + assert_eq!(recv_rx.try_recv().unwrap().value(), [0u8; 32]); } + // Parity with charon newMsg @ v1.7.1: the prepared hash is admitted on the + // same rule as value_hash and is independent of prepared_round, so even a + // ROUND-CHANGE claiming `prepared_round > 0` with a malformed prepared + // hash is received with the nil prepared value. #[test_case(vec![] ; "empty")] #[test_case(vec![0; 32] ; "zero")] #[test_case(vec![1; 31] ; "short")] #[test_case(vec![1; 33] ; "long")] #[tokio::test] - async fn handle_rejects_invalid_prepared_round_change_hash(hash: Vec) { + async fn handle_admits_malformed_prepared_round_change_hash_as_nil(hash: Vec) { + let consensus = consensus(0, true); let mut msg = unsigned_msg(0); msg.r#type = i64::from(qbft::MSG_ROUND_CHANGE); msg.prepared_round = 1; msg.prepared_value_hash = hash.into(); let msg = sign_for_peer(msg, 0); + let inst = consensus.get_instance_io(duty()); - let err = consensus(0, true) + consensus .handle(consensus_msg(msg), &CancellationToken::new()) .await - .unwrap_err(); + .unwrap(); - assert_eq!(err.to_string(), "invalid prepared value hash"); + let mut recv_rx = inst.take_recv_rx().unwrap(); + let received = recv_rx.try_recv().unwrap(); + assert_eq!(received.prepared_round(), 1); + assert_eq!(received.prepared_value(), [0u8; 32]); } #[tokio::test] diff --git a/crates/consensus/src/qbft/definition.rs b/crates/consensus/src/qbft/definition.rs index 315b628b..1836e533 100644 --- a/crates/consensus/src/qbft/definition.rs +++ b/crates/consensus/src/qbft/definition.rs @@ -212,8 +212,16 @@ fn compare_attester( fn local_compare_value( request: &qbft::CompareRequest<'_, ConsensusQbftTypes>, ) -> std::result::Result { - // The generic QBFT core uses `T::Compare::default()` as the "not cached" - // sentinel. For this adapter that is `Any::default()`. + // Sentinel parity: charon core/consensus/qbft/qbft.go:120 @ v1.7.1 uses + // `inputValueSource == nil`. Rust's `T::Compare` is `Default`-able, not + // `Option`, and the generic core seeds it with `Any::default()`, so + // `Any::default()` IS this adapter's `nil`. + // + // Invariant: a genuinely cached compare value is always a marshalled + // `UnsignedDataSet` and therefore carries a non-empty `type_url`, so it can + // never equal `Any::default()` (empty type_url + empty value). Hence + // comparing against the default is a sound "not cached" test. See the + // sentinel invariant test in this module. if request.input_value_source != &Any::default() { return Ok(request.input_value_source.clone()); } @@ -798,6 +806,25 @@ mod tests { assert!(matches!(result, Ok(()))); } + /// Pins the `local_compare_value` sentinel invariant: a genuinely cached + /// compare value is a marshalled `UnsignedDataSet` carrying a non-empty + /// `type_url`, so it can never equal `Any::default()` — the adapter's + /// equivalent of Go's `inputValueSource == nil` "not cached" sentinel. + #[test] + fn any_default_is_distinct_from_marshalled_value() { + let set = unsigned_attestation_set(&pubkey(1), attestation_data()); + let cached = any_unsigned(&set); + + assert!(!cached.type_url.is_empty()); + assert_ne!(cached, Any::default()); + + // An empty UnsignedDataSet still marshals with a type_url, so even a + // degenerate cached value cannot collide with the sentinel. + let empty = any_unsigned(&pbcore::UnsignedDataSet::default()); + assert!(!empty.type_url.is_empty()); + assert_ne!(empty, Any::default()); + } + #[test] fn compare_attester_returns_error_when_cancelled_waiting_for_local_value() { let leader = unsigned_attestation_set(&pubkey(1), attestation_data()); diff --git a/crates/consensus/src/qbft/msg.rs b/crates/consensus/src/qbft/msg.rs index ecefc21a..08bc912b 100644 --- a/crates/consensus/src/qbft/msg.rs +++ b/crates/consensus/src/qbft/msg.rs @@ -66,19 +66,10 @@ pub enum Error { #[error("value hash not found in values")] ValueHashNotFound, - /// Value hash was absent, zero, or not exactly 32 bytes when required. - #[error("invalid value hash")] - InvalidValueHash, - /// Prepared value hash did not exist in the values map. #[error("prepared value hash not found in values")] PreparedValueHashNotFound, - /// Prepared value hash was absent, zero, or not exactly 32 bytes when - /// required. - #[error("invalid prepared value hash")] - InvalidPreparedValueHash, - /// Value did not exist in the values map. #[error("value not found")] ValueNotFound, @@ -145,13 +136,15 @@ impl fmt::Debug for Msg { impl Msg { /// Wraps a raw QBFT protobuf message for the generic core. /// - /// Value-bearing messages must include a non-zero 32-byte `value_hash` - /// present in `values`. This is deliberately stricter than Charon's - /// current wrapper behavior, which collapses absent or malformed hashes to - /// nil; admitting that shape can let core progress on a value that cannot - /// be decoded at decision time. - /// - /// `prepared_value_hash` is optional only while `prepared_round` is zero. + /// Admission mirrors Charon's `newMsg`: a `value_hash` / + /// `prepared_value_hash` that is absent, all-zero, or not exactly 32 bytes + /// collapses to the nil hash `[0u8; 32]` with no error. A well-formed, + /// non-zero 32-byte hash must be present in `values`, otherwise this + /// returns [`Error::ValueHashNotFound`] / + /// [`Error::PreparedValueHashNotFound`]. There is no message-type or + /// prepared-round requirement here; round consistency and value presence + /// at decision time are enforced by the generic QBFT core's justification + /// rules. /// /// Justifications are raw protobuf messages from the same consensus /// envelope. They are recursively wrapped with the same shared value map. @@ -345,25 +338,11 @@ fn to_hash32(value: &[u8]) -> Option { } fn value_hash(msg: &pbconsensus::QbftMsg, values: &ValueMap) -> Result { - let required = value_hash_required(MessageType::from_wire(msg.r#type)); - if msg.value_hash.is_empty() { - return if required { - Err(Error::InvalidValueHash) - } else { - Ok([0u8; 32]) - }; - } - - if msg.value_hash.len() != 32 { - return Err(Error::InvalidValueHash); - } - + // Mirror Charon newMsg: an absent / zero / non-32-byte value_hash collapses + // to the nil hash with no error; a well-formed non-zero 32-byte hash must + // be present in `values`. let Some(hash) = to_hash32(&msg.value_hash) else { - return if required { - Err(Error::InvalidValueHash) - } else { - Ok([0u8; 32]) - }; + return Ok([0u8; 32]); }; if values.contains_key(&hash) { @@ -373,32 +352,11 @@ fn value_hash(msg: &pbconsensus::QbftMsg, values: &ValueMap) -> Result Err(Error::ValueHashNotFound) } -fn value_hash_required(type_: MessageType) -> bool { - type_ == qbft::MSG_PRE_PREPARE - || type_ == qbft::MSG_PREPARE - || type_ == qbft::MSG_COMMIT - || type_ == qbft::MSG_DECIDED -} - fn prepared_value_hash(msg: &pbconsensus::QbftMsg, values: &ValueMap) -> Result { - if msg.prepared_value_hash.is_empty() { - return if msg.prepared_round > 0 { - Err(Error::InvalidPreparedValueHash) - } else { - Ok([0u8; 32]) - }; - } - - if msg.prepared_value_hash.len() != 32 { - return Err(Error::InvalidPreparedValueHash); - } - + // Mirror Charon newMsg: the prepared hash is admitted on the same rule as + // value_hash and is independent of prepared_round. let Some(hash) = to_hash32(&msg.prepared_value_hash) else { - return if msg.prepared_round > 0 { - Err(Error::InvalidPreparedValueHash) - } else { - Ok([0u8; 32]) - }; + return Ok([0u8; 32]); }; if values.contains_key(&hash) { @@ -578,12 +536,16 @@ mod tests { assert_eq!(msg.values().len(), 2); } + // Parity with charon core/consensus/qbft/msg.go newMsg @ v1.7.1: an + // absent, all-zero, or non-32-byte value hash is admitted and collapses to + // the nil hash for every message type — there is no type-gating and no + // malformed-length rejection in the reference wrapper. #[test_case(MSG_PRE_PREPARE, vec![] ; "pre_prepare_empty")] #[test_case(MSG_PREPARE, vec![0; 32] ; "prepare_zero")] #[test_case(MSG_COMMIT, vec![1; 31] ; "commit_short")] #[test_case(MSG_DECIDED, vec![1; 33] ; "decided_long")] - fn new_rejects_required_invalid_value_hash(type_: MessageType, hash: Vec) { - let err = Msg::new( + fn new_admits_malformed_value_hash_as_nil(type_: MessageType, hash: Vec) { + let msg = Msg::new( pbconsensus::QbftMsg { r#type: i64::from(type_), value_hash: hash.into(), @@ -592,9 +554,9 @@ mod tests { vec![], sync::Arc::default(), ) - .unwrap_err(); + .unwrap(); - assert_eq!(err.to_string(), "invalid value hash"); + assert_eq!(msg.value(), [0u8; 32]); } #[test_case(vec![] ; "empty")] @@ -614,58 +576,67 @@ mod tests { assert_eq!(msg.value(), [0u8; 32]); } - #[test_case(vec![1; 31] ; "short")] - #[test_case(vec![1; 33] ; "long")] - fn new_rejects_malformed_optional_value_hash(hash: Vec) { - let err = Msg::new( + // Parity with charon newMsg @ v1.7.1: the prepared hash is admitted on the + // same rule as value_hash and is independent of prepared_round — even a + // ROUND-CHANGE claiming `prepared_round > 0` with an absent/zero/malformed + // prepared hash constructs with the nil hash. Round/prepared-round + // consistency is enforced by the generic core's justification rules. + #[test_case(0, vec![] ; "unprepared_empty")] + #[test_case(0, vec![0; 32] ; "unprepared_zero")] + #[test_case(0, vec![1; 31] ; "unprepared_short")] + #[test_case(0, vec![1; 33] ; "unprepared_long")] + #[test_case(1, vec![] ; "prepared_empty")] + #[test_case(1, vec![0; 32] ; "prepared_zero")] + #[test_case(1, vec![1; 31] ; "prepared_short")] + #[test_case(1, vec![1; 33] ; "prepared_long")] + fn new_admits_malformed_prepared_value_hash_as_nil(prepared_round: i64, hash: Vec) { + let msg = Msg::new( pbconsensus::QbftMsg { - r#type: i64::from(MSG_ROUND_CHANGE), - value_hash: hash.into(), + prepared_round, + prepared_value_hash: hash.into(), ..Default::default() }, vec![], sync::Arc::default(), ) - .unwrap_err(); + .unwrap(); - assert_eq!(err.to_string(), "invalid value hash"); + assert_eq!(msg.prepared_value(), [0u8; 32]); } - #[test_case(vec![] ; "empty")] - #[test_case(vec![0; 32] ; "zero_hash")] - fn new_allows_nil_prepared_value_hash_when_unprepared(hash: Vec) { + /// The two hash helpers are independent: a malformed `value_hash` collapses + /// to nil while a valid present `prepared_value_hash` still maps, and vice + /// versa. + #[test] + fn new_maps_valid_hash_beside_malformed_other_hash() { + let valid_hash = hash_proto(×tamp(1)).unwrap(); + let values = sync::Arc::new(value_map(vec![(valid_hash, any_timestamp(1))])); + let msg = Msg::new( pbconsensus::QbftMsg { - prepared_value_hash: hash.into(), + value_hash: vec![1; 31].into(), + prepared_value_hash: valid_hash.to_vec().into(), ..Default::default() }, vec![], - sync::Arc::default(), + values.clone(), ) .unwrap(); + assert_eq!(msg.value(), [0u8; 32]); + assert_eq!(msg.prepared_value(), valid_hash); - assert_eq!(msg.prepared_value(), [0u8; 32]); - } - - #[test_case(0, vec![1; 31] ; "unprepared_short")] - #[test_case(0, vec![1; 33] ; "unprepared_long")] - #[test_case(1, vec![] ; "prepared_empty")] - #[test_case(1, vec![0; 32] ; "prepared_zero")] - #[test_case(1, vec![1; 31] ; "prepared_short")] - #[test_case(1, vec![1; 33] ; "prepared_long")] - fn new_rejects_invalid_prepared_value_hash(prepared_round: i64, hash: Vec) { - let err = Msg::new( + let msg = Msg::new( pbconsensus::QbftMsg { - prepared_round, - prepared_value_hash: hash.into(), + value_hash: valid_hash.to_vec().into(), + prepared_value_hash: vec![1; 33].into(), ..Default::default() }, vec![], - sync::Arc::default(), + values, ) - .unwrap_err(); - - assert_eq!(err.to_string(), "invalid prepared value hash"); + .unwrap(); + assert_eq!(msg.value(), valid_hash); + assert_eq!(msg.prepared_value(), [0u8; 32]); } #[test] diff --git a/crates/consensus/src/timer.rs b/crates/consensus/src/timer.rs index 78219e3b..b410ed05 100644 --- a/crates/consensus/src/timer.rs +++ b/crates/consensus/src/timer.rs @@ -366,24 +366,33 @@ fn linear_round_timeout(round: i64) -> Result { } /// Returns the reduced timeout used after linear round one. +/// +/// Matches Charon v1.7.1 `core/consensus/timer/roundtimer.go:243`: +/// `time.Duration(200*(round-1) + 200)`. The Go literal has no +/// `* time.Millisecond`, and `time.Duration` is a nanosecond count, so the +/// timeout is `200*(round-1) + 200` **nanoseconds** (e.g. round 2 = 400ns). +/// +/// The Go inline comment claims "400 milliseconds"; that mismatch is the +/// upstream bug tracked in ObolNetwork/charon#4537, which is NOT fixed at the +/// pinned v1.7.1. When the Charon pin moves past #4537 (the fix adds +/// `* time.Millisecond`), flip `Duration::from_nanos` back to +/// `Duration::from_millis` here and update the literal-value tests below. fn linear_subsequent_round_timeout(round: i64) -> Result { ensure_non_negative_round(round)?; - // Charon fixed the previous bare `time.Duration(...)` bug in - // ObolNetwork/charon#4537; subsequent linear rounds are milliseconds. let previous_round = round .checked_sub(1) .ok_or(Error::DurationOverflow { round })?; - let increment_millis = previous_round + let increment_nanos = previous_round .checked_mul(200) .ok_or(Error::DurationOverflow { round })?; - let timeout_millis = increment_millis + let timeout_nanos = increment_nanos .checked_add(200) .ok_or(Error::DurationOverflow { round })?; - let timeout_millis = - u64::try_from(timeout_millis).map_err(|_| Error::DurationOverflow { round })?; + let timeout_nanos = + u64::try_from(timeout_nanos).map_err(|_| Error::DurationOverflow { round })?; - Ok(Duration::from_millis(timeout_millis)) + Ok(Duration::from_nanos(timeout_nanos)) } /// Rejects negative consensus rounds before duration arithmetic. @@ -499,9 +508,9 @@ mod tests { } #[test_case(1, Duration::from_millis(1_000) ; "round_1")] - #[test_case(2, Duration::from_millis(400) ; "round_2")] - #[test_case(3, Duration::from_millis(600) ; "round_3")] - #[test_case(4, Duration::from_millis(800) ; "round_4")] + #[test_case(2, Duration::from_nanos(400) ; "round_2")] + #[test_case(3, Duration::from_nanos(600) ; "round_3")] + #[test_case(4, Duration::from_nanos(800) ; "round_4")] #[tokio::test(start_paused = true)] async fn linear_round_timer(round: i64, want: Duration) { let timer = LinearRoundTimer::new(); @@ -513,7 +522,28 @@ mod tests { }; assert_eq!(want, duration); - assert_fires_after(timeout, want, &format!("Timer(round {round}) did not fire")).await; + // Tokio's timer wheel has millisecond granularity, so the paused clock + // must advance by at least 1ms for the sub-millisecond (nanosecond) + // subsequent-round timeouts to fire. + let advance_by = want.max(Duration::from_millis(1)); + assert_fires_after( + timeout, + advance_by, + &format!("Timer(round {round}) did not fire"), + ) + .await; + } + + #[test_case(0, Duration::ZERO ; "round_0")] + #[test_case(2, Duration::from_nanos(400) ; "round_2")] + #[test_case(3, Duration::from_nanos(600) ; "round_3")] + #[test_case(4, Duration::from_nanos(800) ; "round_4")] + #[test_case(11, Duration::from_nanos(2_200) ; "round_11")] + fn linear_subsequent_round_timeout_is_nanoseconds(round: i64, want: Duration) { + // Mirrors Charon v1.7.1 roundtimer.go:243 `time.Duration(200*(round-1)+200)`, + // which is nanoseconds (no `* time.Millisecond`). Flip to `from_millis` + // expectations only when the Charon pin moves past charon#4537. + assert_eq!(want, must_duration(linear_subsequent_round_timeout(round))); } #[test] @@ -633,9 +663,16 @@ mod tests { .await; let timeout = must_timer(timer.timer(3)); - let want = Duration::from_millis(600); + let want = Duration::from_nanos(600); assert_eq!(want, must_duration(linear_subsequent_round_timeout(3))); - assert_fires_after(timeout, want, "round 3 proposer timer did not fire").await; + // Advance a full millisecond: tokio's timer wheel granularity is 1ms, + // so the 600ns timeout cannot fire on a smaller advance. + assert_fires_after( + timeout, + Duration::from_millis(1), + "round 3 proposer timer did not fire", + ) + .await; } #[test] diff --git a/crates/core/src/dutydb/memory.rs b/crates/core/src/dutydb/memory.rs index 61992bf3..cf8a794d 100644 --- a/crates/core/src/dutydb/memory.rs +++ b/crates/core/src/dutydb/memory.rs @@ -534,8 +534,21 @@ impl State { let committee_index = att.duty.committee_index; let validator_index = att.duty.validator_index; + // Real-index entries (full clash checks). self.store_att_pubkey(slot, duty_slot, committee_index, validator_index, pubkey)?; self.store_att_data(slot, committee_index, &att.data)?; + + // commIdx=0 dual-storage. Post-Electra the beacon-APIs spec default for + // produceAttestationData is committee_index=0, so a spec-correct VC + // requests attestation data with index 0 regardless of the real + // committee index. Mirror Charon `storeAttestationUnsafe` by also + // writing the pubkey/data under committee_index=0. When the real + // committee_index is already 0 these calls are no-ops: the entries were + // just written above and the clash checks pass against the identical + // value, so no duplicate key is appended to attestation_keys_by_slot. + self.store_att_pubkey(slot, duty_slot, 0, validator_index, pubkey)?; + self.store_att_data_commidx0(slot, &att.data)?; + Ok(()) } @@ -601,6 +614,38 @@ impl State { Ok(()) } + /// Stores attestation data under the hardcoded `committee_index = 0` key. + /// + /// Mirrors Charon's `storeAttestationUnsafe` commIdx=0 branch. Uses a + /// RELAXED clash check that compares only `source` and `target` and + /// deliberately ignores `beacon_block_root`: a slow beacon node can return + /// data with a stale head for some validators in the fetch loop while + /// returning the new head for others, so the head (beacon_block_root) may + /// legitimately differ for the same `(slot, committee_index=0)`. Only the + /// source/target checkpoints must match. + fn store_att_data_commidx0(&mut self, slot: u64, data: &phase0::AttestationData) -> Result<()> { + let att_key = AttKey { + slot, + committee_index: 0, + }; + if let Some(existing) = self.attestation_duties.get(&att_key) { + if existing.source != data.source || existing.target != data.target { + warn!( + slot, + committee_index = 0, + "dutydb: clashing attestation data (commidx=0 source/target)" + ); + return Err(Error::ClashingAttestationData { + slot, + committee_index: 0, + }); + } + } else { + self.attestation_duties.insert(att_key, data.clone()); + } + Ok(()) + } + fn store_agg_attestation(&mut self, agg: &VersionedAggregatedAttestation) -> Result<()> { let att_data = agg.data().ok_or(Error::InvalidAggregatedAttestation)?; let root = att_data.tree_hash_root().0; @@ -1141,6 +1186,246 @@ mod tests { ); } + #[tokio::test] + async fn commidx0_lookup_for_nonzero_committee() { + const SLOT: u64 = 700; + const COMM_IDX: u64 = 4; + const V_IDX: u64 = 9; + + let db = make_db(); + let pk = random_core_pub_key(); + let mut set = UnsignedDataSet::new(); + set.insert( + pk, + UnsignedDutyData::Attestation(att_data(SLOT, COMM_IDX, V_IDX)), + ); + db.store(Duty::new(SlotNumber::new(SLOT), DutyType::Attester), set) + .await + .unwrap(); + + // Real index resolves. + let real = db.await_attestation(SLOT, COMM_IDX).await.unwrap(); + assert_eq!(real.index, COMM_IDX); + assert_eq!( + db.pub_key_by_attestation(SLOT, COMM_IDX, V_IDX) + .await + .unwrap(), + pk + ); + + // committee_index = 0 also resolves (dual-storage). The stored data is + // the original att data, so its `index` field is still COMM_IDX. + let zero = db.await_attestation(SLOT, 0).await.unwrap(); + assert_eq!(zero.slot, SLOT); + assert_eq!(zero.index, COMM_IDX); + assert_eq!(db.pub_key_by_attestation(SLOT, 0, V_IDX).await.unwrap(), pk); + } + + #[tokio::test] + async fn commidx0_relaxed_clash_differing_head_ok() { + const SLOT: u64 = 710; + let db = make_db(); + + // Distinct real committee indices so the real-index AttKeys differ, + // isolating the commIdx=0 relaxed check. + let mut a = att_data(SLOT, 1, 1); + a.data.beacon_block_root = [0xaa; 32]; + let mut b = att_data(SLOT, 2, 2); + b.data.beacon_block_root = [0xbb; 32]; // different head, same default source/target + + let mut set = UnsignedDataSet::new(); + set.insert(random_core_pub_key(), UnsignedDutyData::Attestation(a)); + set.insert(random_core_pub_key(), UnsignedDutyData::Attestation(b)); + + // Must succeed: the commIdx=0 entry tolerates differing beacon_block_root. + db.store(Duty::new(SlotNumber::new(SLOT), DutyType::Attester), set) + .await + .unwrap(); + + // commIdx=0 lookup resolves (whichever was inserted first). + db.await_attestation(SLOT, 0).await.unwrap(); + } + + #[tokio::test] + async fn commidx0_clash_on_differing_source() { + const SLOT: u64 = 720; + let db = make_db(); + let duty = Duty::new(SlotNumber::new(SLOT), DutyType::Attester); + + let a = att_data(SLOT, 1, 1); // default source/target + let mut b = att_data(SLOT, 2, 2); + b.data.source = phase0::Checkpoint { + epoch: 5, + root: [0xcc; 32], + }; + + let mut set1 = UnsignedDataSet::new(); + set1.insert(random_core_pub_key(), UnsignedDutyData::Attestation(a)); + db.store(duty.clone(), set1).await.unwrap(); + + let mut set2 = UnsignedDataSet::new(); + set2.insert(random_core_pub_key(), UnsignedDutyData::Attestation(b)); + let err = db.store(duty, set2).await.unwrap_err(); + assert!( + matches!( + err, + Error::ClashingAttestationData { + committee_index: 0, + slot: SLOT + } + ), + "expected commIdx=0 ClashingAttestationData, got: {err}" + ); + } + + #[tokio::test] + async fn commidx0_clash_on_differing_target() { + const SLOT: u64 = 721; + let db = make_db(); + let duty = Duty::new(SlotNumber::new(SLOT), DutyType::Attester); + + let a = att_data(SLOT, 1, 1); // default source/target + let mut b = att_data(SLOT, 2, 2); + b.data.target = phase0::Checkpoint { + epoch: 6, + root: [0xdd; 32], + }; + + let mut set1 = UnsignedDataSet::new(); + set1.insert(random_core_pub_key(), UnsignedDutyData::Attestation(a)); + db.store(duty.clone(), set1).await.unwrap(); + + let mut set2 = UnsignedDataSet::new(); + set2.insert(random_core_pub_key(), UnsignedDutyData::Attestation(b)); + let err = db.store(duty, set2).await.unwrap_err(); + assert!( + matches!( + err, + Error::ClashingAttestationData { + committee_index: 0, + slot: SLOT + } + ), + "expected commIdx=0 ClashingAttestationData, got: {err}" + ); + } + + /// Guards against the relaxed commIdx=0 check leaking onto the real-index + /// path: same slot AND same real committee index with a differing + /// beacon_block_root must still clash on the REAL committee index. + #[tokio::test] + async fn commidx0_real_index_full_clash_still_fires() { + const SLOT: u64 = 725; + const COMM_IDX: u64 = 3; + + let db = make_db(); + let duty = Duty::new(SlotNumber::new(SLOT), DutyType::Attester); + + let mut a = att_data(SLOT, COMM_IDX, 1); + a.data.beacon_block_root = [0xaa; 32]; + let mut b = att_data(SLOT, COMM_IDX, 2); + b.data.beacon_block_root = [0xbb; 32]; + + let mut set1 = UnsignedDataSet::new(); + set1.insert(random_core_pub_key(), UnsignedDutyData::Attestation(a)); + db.store(duty.clone(), set1).await.unwrap(); + + let mut set2 = UnsignedDataSet::new(); + set2.insert(random_core_pub_key(), UnsignedDutyData::Attestation(b)); + let err = db.store(duty, set2).await.unwrap_err(); + assert!( + matches!( + err, + Error::ClashingAttestationData { + committee_index: COMM_IDX, + slot: SLOT + } + ), + "expected real-index ClashingAttestationData, got: {err}" + ); + } + + #[tokio::test] + async fn commidx0_entries_evicted_with_duty() { + let deadliner = far_future_handle(); + let (trim_tx, trim_rx) = channel::(64); + let db = make_db_with_deadliner(deadliner, trim_rx); + + const SLOT: u64 = 730; + const COMM_IDX: u64 = 6; + const V_IDX: u64 = 3; + + let mut set = UnsignedDataSet::new(); + set.insert( + random_core_pub_key(), + UnsignedDutyData::Attestation(att_data(SLOT, COMM_IDX, V_IDX)), + ); + db.store(Duty::new(SlotNumber::new(SLOT), DutyType::Attester), set) + .await + .unwrap(); + + // Both keys present before eviction. + db.pub_key_by_attestation(SLOT, COMM_IDX, V_IDX) + .await + .unwrap(); + db.pub_key_by_attestation(SLOT, 0, V_IDX).await.unwrap(); + + trim_tx + .send(Duty::new(SlotNumber::new(SLOT), DutyType::Attester)) + .await + .expect("trim_tx should be open"); + let mut set2 = UnsignedDataSet::new(); + set2.insert( + random_core_pub_key(), + UnsignedDutyData::Proposal(Box::new(phase0_proposal(SLOT.saturating_add(1), 0))), + ); + db.store( + Duty::new(SlotNumber::new(SLOT.saturating_add(1)), DutyType::Proposer), + set2, + ) + .await + .unwrap(); + + // Both keys gone after eviction. + assert!( + db.pub_key_by_attestation(SLOT, COMM_IDX, V_IDX) + .await + .is_err() + ); + assert!(db.pub_key_by_attestation(SLOT, 0, V_IDX).await.is_err()); + } + + /// When the real committee index is already 0, the commIdx=0 dual write is + /// a no-op: re-store is idempotent and no duplicate PkKey is appended to + /// `attestation_keys_by_slot`. + #[tokio::test] + async fn commidx0_when_real_index_is_zero_no_duplicate() { + const SLOT: u64 = 740; + const V_IDX: u64 = 2; + let db = make_db(); + let pk = random_core_pub_key(); + let duty = Duty::new(SlotNumber::new(SLOT), DutyType::Attester); + + let mut set = UnsignedDataSet::new(); + set.insert(pk, UnsignedDutyData::Attestation(att_data(SLOT, 0, V_IDX))); + db.store(duty.clone(), set.clone()).await.unwrap(); + // Idempotent re-store must not clash. + db.store(duty, set).await.unwrap(); + + assert_eq!(db.pub_key_by_attestation(SLOT, 0, V_IDX).await.unwrap(), pk); + db.await_attestation(SLOT, 0).await.unwrap(); + + // Exactly one PkKey recorded for the duty slot (no duplicate append). + let state = db.state.read().await; + assert_eq!( + state + .attestation_keys_by_slot + .get(&SLOT) + .map_or(0, Vec::len), + 1 + ); + } + #[tokio::test] async fn shutdown_wakes_waiting_await() { let db = Arc::new(make_db()); diff --git a/crates/core/src/qbft/mod.rs b/crates/core/src/qbft/mod.rs index 742f7f5d..ecc8d087 100644 --- a/crates/core/src/qbft/mod.rs +++ b/crates/core/src/qbft/mod.rs @@ -959,9 +959,8 @@ fn is_justified_round_change(d: &Definition, msg: &Msg) -> b let pr = msg.prepared_round(); let pv = msg.prepared_value(); - // The IBFT paper requires ROUND-CHANGE prepared_round to be lower than the - // target round. Go core currently omits this check, but valid Charon traffic - // already satisfies it. + // Accepted hardening over Go: reject prepared rounds outside `0 <= pr < round`. + // See `valid_round_change_prepared_round` for the full parity note. if !valid_round_change_prepared_round(msg) { return false; } @@ -999,6 +998,17 @@ fn is_justified_round_change(d: &Definition, msg: &Msg) -> b true } +/// Returns true if a ROUND-CHANGE's prepared round is in the valid range +/// `0 <= pr < round`. +/// +/// Parity note (accepted hardening): charon core/qbft/qbft.go @ v1.7.1 does NOT +/// perform this check in isJustifiedRoundChange / containsJustifiedQrc / +/// getJustifiedQrc — it trusts that valid Charon traffic always satisfies +/// `0 <= pr < round` (the IBFT paper requires the prepared round to be lower +/// than the target round). Pluto adds the explicit bound to reject negative or +/// not-yet-in-the-past prepared rounds from a buggy or byzantine peer. This is +/// a stricter superset of Go: every message Go accepts here Pluto also accepts, +/// so honest consensus is unaffected. fn valid_round_change_prepared_round(msg: &Msg) -> bool { let pr = msg.prepared_round(); pr >= 0 && pr < msg.round() @@ -1072,7 +1082,7 @@ fn contains_justified_qrc( ) -> Option { let qrc = filter_round_change(justification, round) .into_iter() - .filter(valid_round_change_prepared_round) + .filter(valid_round_change_prepared_round) // accepted hardening; see helper doc .collect::>(); if qrc.len() < d.quorum_count() { return None; @@ -1169,7 +1179,7 @@ fn get_justified_qrc( let round_changes = filter_round_change(all, round) .into_iter() - .filter(valid_round_change_prepared_round) + .filter(valid_round_change_prepared_round) // accepted hardening; see helper doc .collect::>(); for prepares in get_prepare_quorums(d, all) { diff --git a/crates/core/src/sigagg.rs b/crates/core/src/sigagg.rs index 04487b00..944d7094 100644 --- a/crates/core/src/sigagg.rs +++ b/crates/core/src/sigagg.rs @@ -209,22 +209,30 @@ impl Aggregator { } })?; - // Prefer a VersionedAttestation that has validator_index set — the local VC - // includes it, peers don't. Falling back to parSigs[0] is fine for all other - // duty types, and for attestations where no parSig carries a validator_index. - // All parSigs share the same unsigned payload (guaranteed by consensus), so - // any one works as a template. - let template = par_sigs - .iter() - .find_map(|ps| { - let att = ps - .signed_data - .as_any() - .downcast_ref::()?; - att.0.validator_index?; // return an error if validator_index is not set - Some(ps.signed_data.as_ref()) - }) - .unwrap_or_else(|| par_sigs[0].signed_data.as_ref()); + // Fix for validator index sent only by validator client and not peers + // (parity: charon core/sigagg/sigagg.go:118-134 @ v1.7.1). + // Mirror Go's loop exactly: the scan aborts on the first parSig that is + // not a VersionedAttestation (Go `break`s), so a non-attestation duty + // always uses parSigs[0]. Among attestations, prefer the first one + // whose validator_index is set — the local VC includes it, peers don't. + // All parSigs for one (pubkey, duty) share the same concrete type and + // unsigned payload (guaranteed by consensus), so the non-attestation + // slice is homogeneous and parSigs[0] is a valid template. + let mut full_sig: Option<&dyn SignedData> = None; + for ps in par_sigs { + let Some(att) = ps + .signed_data + .as_any() + .downcast_ref::() + else { + break; // first non-attestation aborts the scan, matching Go + }; + if att.0.validator_index.is_some() { + full_sig = Some(ps.signed_data.as_ref()); + break; + } + } + let template = full_sig.unwrap_or_else(|| par_sigs[0].signed_data.as_ref()); let agg_signed = template.set_signature_boxed(agg_bytes).map_err(|e| { error!(parent: &span, error = %e, "set_signature failed"); @@ -878,4 +886,43 @@ mod tests { "output must preserve validator_index from template" ); } + + /// A homogeneous non-attestation set aborts the template scan on the first + /// element (Go `break`s in sigagg.go:118-134 @ v1.7.1), so `par_sigs[0]` + /// is used as the template and aggregation succeeds with the same concrete + /// type. + #[tokio::test] + async fn first_non_attestation_aborts_scan() { + let ctx = make_bls_context(); + let par_sigs: Vec = ctx + .sigs + .iter() + .map(|(idx, sig)| ParSignedData::new(MockSignedData { sig: *sig }, *idx)) + .collect(); + + let captured: Arc>>> = Arc::new(Mutex::new(None)); + let captured_clone = captured.clone(); + + let mut agg = Aggregator::new(3, noop_verify()).unwrap(); + agg.subscribe(Arc::new(move |_, set: &AggSignedDataSet| { + let captured_clone = captured_clone.clone(); + let output = set.values().next().unwrap().clone(); + Box::pin(async move { + *captured_clone.lock().unwrap() = Some(output); + Ok(()) + }) + })); + + let mut set = HashMap::new(); + set.insert(PubKey::new(ctx.pubkey), par_sigs); + agg.aggregate(&Duty::new_randao_duty(1.into()), &set) + .await + .unwrap(); + + let output = captured.lock().unwrap().take().unwrap(); + assert!( + output.as_any().downcast_ref::().is_some(), + "output must keep the non-attestation template type (par_sigs[0])" + ); + } } diff --git a/crates/core/src/tracker/analysis.rs b/crates/core/src/tracker/analysis.rs index 00260dc7..b32aef37 100644 --- a/crates/core/src/tracker/analysis.rs +++ b/crates/core/src/tracker/analysis.rs @@ -272,10 +272,22 @@ pub(crate) fn analyse_fetcher_failed( analyse_fetcher_failed_sync_contribution(duty, all_events, fetch_err, feature_set) } _ => { - // TODO: when the fetcher is ported, add an `is_cancelled_error` check here - // (similar to `is_eth2_api_error`) so cancellation/timeout errors map to the - // default reason rather than `REASON_BUG_FETCH_ERROR`, matching Go's three-tier - // logic in `analyseFetcherFailed` (tracker.go:299–305). + // Parity: charon core/tracker/tracker.go:296-324 @ v1.7.1 + // classifies the fetch error in three tiers — (a) eth2 API error => + // REASON_FETCH_BN_ERROR, (b) context.Canceled / + // context.DeadlineExceeded => the default reason, (c) otherwise => + // REASON_BUG_FETCH_ERROR. Go computes this upfront for all duty + // types, but the proposer/aggregator/sync-contribution arms + // recompute their own reason, so the classification only affects + // this default arm. + // + // Accepted divergence (temporary): tiers (a) and (c) are + // implemented but NOT the cancellation tier (b), because the + // fetcher is not yet ported and there is no cancellation/timeout + // error variant to match against. When the fetcher lands, add an + // `is_cancelled_error(...)` check (mirroring `is_eth2_api_error` + // below) so cancellation/deadline errors map to the default reason + // rather than REASON_BUG_FETCH_ERROR. let reason = if let Some(e) = &fetch_err && is_eth2_api_error(e.as_ref()) { diff --git a/crates/core/src/tracker/inclusion.rs b/crates/core/src/tracker/inclusion.rs index d8336d8a..88b7d605 100644 --- a/crates/core/src/tracker/inclusion.rs +++ b/crates/core/src/tracker/inclusion.rs @@ -301,6 +301,14 @@ impl InclusionCore { .iter() .filter_map(|(key, sub)| match sub.duty.duty_type { DutyType::Proposer => (sub.duty.slot.inner() == slot).then(|| key.clone()), + // Parity: charon core/tracker/inclusion.go:289-291 @ v1.7.1 + // panics with "bug: unexpected type" here — CheckBlock (the + // non-attestation path) is only ever fed proposer submissions. + // `unreachable!` reproduces that panic with the same message. + // Accepted divergence in panic *site* only: Go panics while + // iterating the offending submission; Rust panics inside the + // `filter_map` closure on the same element — observably + // identical. _ => unreachable!("bug: unexpected type"), }) .collect(); @@ -809,6 +817,30 @@ mod tests { assert_eq!(scenario(1, false), 0, "slot mismatch, not found -> skipped"); } + /// Pins the faithful parity with Go's `panic("bug: unexpected type")` in + /// `CheckBlock` (charon core/tracker/inclusion.go:289-291 @ v1.7.1): the + /// non-attestation path only ever expects proposer submissions. + #[test] + #[should_panic(expected = "bug: unexpected type")] + fn check_block_panics_on_non_proposer_submission() { + let mut core = InclusionCore::with_handlers( + Box::new(|_d: &Duty, _pk, _err| {}), + Box::new(|_s: &Submission| {}), + Box::new(|_s: &Submission, _b: &Block| {}), + featureset(true), + ); + + core.submitted( + Duty::new_attester_duty(SlotNumber::new(7)), + pubkey(), + Box::new(Attestation::new(phase0_attestation(7))), + Duration::ZERO, + ) + .expect("submit attester"); + + core.check_block(7, true); + } + /// `trim` reports each removed submission as missed and never included, /// only for slots at or below the trim slot. #[test] diff --git a/crates/core/src/types.rs b/crates/core/src/types.rs index 882e2ef9..5ffc4740 100644 --- a/crates/core/src/types.rs +++ b/crates/core/src/types.rs @@ -1069,6 +1069,135 @@ mod tests { assert!(!DutyType::DutySentinel(Box::new(DutyType::Attester)).is_valid()); } + #[test] + fn duty_type_to_i32_literal_charon_numbers() { + // Numbers are the canonical Charon core/types.go @ v1.7.1 enum values + // and MUST NOT change (wire-compatibility). + let cases: [(DutyType, i32); 14] = [ + (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), + ]; + for (dt, n) in &cases { + assert_eq!(i32::try_from(dt), Ok(*n), "encoding {dt:?}"); + } + } + + #[test] + fn duty_type_i32_round_trip_both_directions() { + // Intentionally a duplicate of the table above: the duplication is + // documentation of the wire contract. + let cases: [(DutyType, i32); 14] = [ + (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), + ]; + for (dt, n) in &cases { + // i32 -> DutyType + let decoded = DutyType::try_from(*n).expect("decode known number"); + assert_eq!(&decoded, dt, "decoding {n}"); + // DutyType -> i32 -> DutyType round-trips back to the same number + let encoded = i32::try_from(&decoded).expect("re-encode"); + assert_eq!(encoded, *n, "round-trip {dt:?}"); + } + } + + #[test] + fn duty_type_conversion_error_edges() { + // i32 -> DutyType: anything outside 0..=13 is InvalidDuty. 14 is + // Charon's private dutySentinel value and must never decode. + for bad in [14_i32, 15, 99, -1, i32::MAX, i32::MIN] { + assert!( + matches!( + DutyType::try_from(bad), + Err(ParSigExCodecError::InvalidDuty) + ), + "i32 {bad} should be rejected", + ); + } + + // &DutyType -> i32: the sentinel variant is the only encoding error. + assert_eq!( + i32::try_from(&DutyType::DutySentinel(Box::new(DutyType::Unknown))), + Err(DutyTypeError::InvalidDutyType), + ); + assert_eq!( + i32::try_from(&DutyType::DutySentinel(Box::new(DutyType::Attester))), + Err(DutyTypeError::InvalidDutyType), + ); + // Unknown encodes to 0 (not an error) in this direction. + assert_eq!(i32::try_from(&DutyType::Unknown), Ok(0)); + } + + #[test] + fn duty_proto_round_trip_and_asymmetry() { + // Valid duty types round-trip slot + type through the proto. + for dt in DutyType::all() { + let duty = Duty::new(SlotNumber::new(424_242), dt.clone()); + let proto = pbcore::Duty::try_from(&duty).expect("encode valid duty"); + assert_eq!(proto.slot, 424_242); + assert_eq!(proto.r#type, i32::try_from(&dt).unwrap()); + let back = Duty::try_from(&proto).expect("decode valid duty"); + assert_eq!(back, duty); + } + + // Unknown: encodes to r#type 0 (Ok)... + let unknown = Duty::new(SlotNumber::new(7), DutyType::Unknown); + let proto_unknown = pbcore::Duty::try_from(&unknown).expect("Unknown encodes"); + assert_eq!(proto_unknown.r#type, 0); + assert_eq!(proto_unknown.slot, 7); + // ...but the proto -> Duty direction rejects it (invalid duty type). + // ParSigExCodecError has no PartialEq, so match instead of assert_eq!. + assert!(matches!( + Duty::try_from(&proto_unknown), + Err(ParSigExCodecError::InvalidDuty), + )); + + // Sentinel duty cannot be encoded at all. + let sentinel = Duty::new( + SlotNumber::new(7), + DutyType::DutySentinel(Box::new(DutyType::Unknown)), + ); + assert_eq!( + pbcore::Duty::try_from(&sentinel), + Err(DutyTypeError::InvalidDutyType), + ); + + // Out-of-range proto r#type is rejected on decode. + for bad in [14_i32, 99] { + let proto = pbcore::Duty { + slot: 7, + r#type: bad, + }; + assert!(matches!( + Duty::try_from(&proto), + Err(ParSigExCodecError::InvalidDuty), + )); + } + } + #[test] fn pub_key_from_bytes() { let bytes = [42u8; PK_LEN];