diff --git a/crates/core/src/validatorapi/component.rs b/crates/core/src/validatorapi/component.rs index 3da601ef..0e9bd91c 100644 --- a/crates/core/src/validatorapi/component.rs +++ b/crates/core/src/validatorapi/component.rs @@ -529,7 +529,7 @@ impl Component { self.dutydb .await_proposal(slot) .await - .map_err(map_dutydb_error) + .map_err(|err| map_dutydb_error("proposal", err)) } /// Resolves the validator index for a VC-submitted attestation. @@ -1067,7 +1067,7 @@ impl Handler for Component { "attestation data not available before deadline", ) })? - .map_err(map_dutydb_error)?; + .map_err(|err| map_dutydb_error("attestation", err))?; Ok(AttestationDataResponse { data }) } @@ -2045,20 +2045,27 @@ fn upstream_unexpected(endpoint: &'static str, response: R) } /// Maps a [`crate::dutydb::Error`] into the `ApiError` returned to the client -/// when an `attestation_data` await fails. `Shutdown` propagates as 503 so the -/// VC can retry; `AwaitDutyExpired` propagates as 408 — same as a timeout — -/// since the duty is gone and the data will never arrive. Anything else is a -/// programming error here and becomes 500. -fn map_dutydb_error(err: DutyDbError) -> ApiError { +/// when a duty-data await fails. `Shutdown` propagates as 503 so the VC can +/// retry; `AwaitDutyExpired` propagates as 408 — same as a timeout — since the +/// duty is gone and the data will never arrive. Anything else is a programming +/// error here and becomes 500. +/// +/// `duty` names the duty being awaited (e.g. `"attestation"`, `"proposal"`) so +/// the client-visible message matches the request that produced it; this mapper +/// is shared by more than one endpoint. +fn map_dutydb_error(duty: &'static str, err: DutyDbError) -> ApiError { let (status, message) = match err { - DutyDbError::Shutdown => (StatusCode::SERVICE_UNAVAILABLE, "dutydb is shutting down"), + DutyDbError::Shutdown => ( + StatusCode::SERVICE_UNAVAILABLE, + "dutydb is shutting down".to_string(), + ), DutyDbError::AwaitDutyExpired => ( StatusCode::REQUEST_TIMEOUT, - "attestation duty expired before data was stored", + format!("{duty} duty expired before data was stored"), ), _ => ( StatusCode::INTERNAL_SERVER_ERROR, - "await attestation failed", + format!("await {duty} failed"), ), }; ApiError::new(status, message).with_source(err) @@ -3169,19 +3176,35 @@ mod tests { #[test] fn map_dutydb_error_status_codes() { assert_eq!( - map_dutydb_error(DutyDbError::Shutdown).status_code, + map_dutydb_error("attestation", DutyDbError::Shutdown).status_code, StatusCode::SERVICE_UNAVAILABLE ); assert_eq!( - map_dutydb_error(DutyDbError::AwaitDutyExpired).status_code, + map_dutydb_error("attestation", DutyDbError::AwaitDutyExpired).status_code, StatusCode::REQUEST_TIMEOUT ); assert_eq!( - map_dutydb_error(DutyDbError::UnsupportedDutyType).status_code, + map_dutydb_error("attestation", DutyDbError::UnsupportedDutyType).status_code, StatusCode::INTERNAL_SERVER_ERROR ); } + /// The expiry message names the duty that was awaited: the mapper is shared + /// by `attestation_data` and `await_proposal`, and previously reported + /// every timeout — including a missed block proposal — as an + /// attestation. + #[test] + fn map_dutydb_error_message_names_the_duty() { + assert_eq!( + map_dutydb_error("proposal", DutyDbError::AwaitDutyExpired).message, + "proposal duty expired before data was stored" + ); + assert_eq!( + map_dutydb_error("attestation", DutyDbError::AwaitDutyExpired).message, + "attestation duty expired before data was stored" + ); + } + /// `upstream_status_error` keeps the upstream response body out of the /// client-visible message but preserves it on `source()` so it lands in /// the debug log. diff --git a/crates/core/src/validatorapi/error.rs b/crates/core/src/validatorapi/error.rs index e13c440d..453d8c9e 100644 --- a/crates/core/src/validatorapi/error.rs +++ b/crates/core/src/validatorapi/error.rs @@ -100,6 +100,28 @@ struct ErrorBody { impl IntoResponse for ApiError { fn into_response(self) -> Response { + // The `source` never reaches the client (it can carry internal detail), + // but it is the only place the underlying cause is recorded — e.g. which + // field an SSZ/JSON body failed to decode. Log it here, on the single + // path every error response takes, otherwise it is silently dropped. + if let Some(source) = &self.source { + if self.status_code.is_server_error() { + tracing::error!( + status = self.status_code.as_u16(), + message = %self.message, + source = %DisplayChain(source.as_ref()), + "validator api error" + ); + } else { + tracing::debug!( + status = self.status_code.as_u16(), + message = %self.message, + source = %DisplayChain(source.as_ref()), + "validator api error" + ); + } + } + let body = ErrorBody { code: self.status_code.as_u16(), message: self.message, @@ -108,3 +130,20 @@ impl IntoResponse for ApiError { (self.status_code, Json(body)).into_response() } } + +/// Renders an error together with its `source()` chain, so a wrapped cause +/// (such as the inner `ssz::DecodeError` behind a decode failure) is not +/// truncated to just the outermost message. +struct DisplayChain<'a>(&'a (dyn std::error::Error + 'static)); + +impl fmt::Display for DisplayChain<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.0)?; + let mut current = self.0.source(); + while let Some(err) = current { + write!(f, ": {err}")?; + current = err.source(); + } + Ok(()) + } +} diff --git a/crates/eth2api/src/spec/bellatrix.rs b/crates/eth2api/src/spec/bellatrix.rs index 4162d27e..ed8e552d 100644 --- a/crates/eth2api/src/spec/bellatrix.rs +++ b/crates/eth2api/src/spec/bellatrix.rs @@ -22,9 +22,22 @@ pub const MAX_BYTES_PER_TRANSACTION: usize = 1_073_741_824; pub type BaseFeePerGas = U256; /// Raw execution transaction bytes. +/// +/// Spec: `Transaction = ByteList[MAX_BYTES_PER_TRANSACTION]` — a bare SSZ list, +/// not a container. `struct_behaviour = "transparent"` is therefore required: +/// without it `ssz_derive` frames every transaction as a single-field container +/// (a spurious 4-byte offset prefix), which makes any block carrying at least +/// one transaction fail to decode. Charon models the same type as a plain +/// `type Transaction []byte`. +/// +/// `TreeHash` intentionally keeps the derived container behaviour: merkleizing +/// a one-field container yields the single leaf unchanged, so the root already +/// equals the bare list's root (locked in by the `*_beacon_block_body_root` +/// spec-vector tests, whose fixtures carry transactions). #[serde_as] #[derive(Debug, Clone, PartialEq, Eq, Encode, Decode, TreeHash, Serialize, Deserialize)] #[serde(transparent)] +#[ssz(struct_behaviour = "transparent")] pub struct Transaction { /// Transaction bytes. #[serde_as(as = "pluto_ssz::serde_utils::Hex0x")] @@ -358,4 +371,53 @@ mod tests { fn json_matches_vector(actual: serde_json::Value, expected_json: &'static str) { test_fixtures::assert_json_eq(actual, expected_json); } + + /// `Transaction` is `ByteList[MAX_BYTES_PER_TRANSACTION]`, so its SSZ form + /// is the raw transaction bytes with no framing. Without + /// `struct_behaviour = "transparent"` the derive emitted a leading 4-byte + /// offset, which made every block carrying a transaction undecodable. + #[test] + fn transaction_ssz_is_the_bare_byte_list() { + use ssz::{Decode, Encode}; + + // A realistic type-3 (blob) transaction prefix: the first four bytes are + // not a valid container offset, which is exactly why container framing + // broke decoding. + let raw = vec![0x03, 0xf8, 0xb9, 0x83, 0xde, 0xad, 0xbe, 0xef]; + let tx = super::Transaction::from(raw.clone()); + + assert_eq!(tx.as_ssz_bytes(), raw, "encoding must not add framing"); + assert_eq!(tx.ssz_bytes_len(), raw.len()); + assert_eq!( + super::Transaction::from_ssz_bytes(&raw).expect("raw bytes decode"), + tx + ); + // Empty transactions are a valid zero-length list. + assert!(super::Transaction::from_ssz_bytes(&[]).is_ok()); + } + + /// An execution payload carrying transactions must survive an SSZ round + /// trip. Previously the fixtures only exercised `TreeHash` and JSON, so + /// the broken SSZ framing went unnoticed. + #[test] + fn execution_payload_with_transactions_ssz_round_trips() { + use ssz::{Decode, Encode}; + + let payload = test_fixtures::bellatrix_execution_payload_fixture(); + assert!( + !payload.transactions.0.is_empty(), + "fixture must carry transactions for this to be meaningful" + ); + + let bytes = payload.as_ssz_bytes(); + let decoded = + super::ExecutionPayload::from_ssz_bytes(&bytes).expect("payload round trips via ssz"); + assert_eq!(decoded, payload); + + // The transactions list is `List[Transaction, N]`: a 4-byte offset per + // element followed by each element's bare bytes. + let txs = &payload.transactions.0; + let expected_len = txs.len() * 4 + txs.iter().map(|tx| tx.bytes.0.len()).sum::(); + assert_eq!(payload.transactions.as_ssz_bytes().len(), expected_len); + } }