Skip to content

refactor(l1): improve error types with actionable context #6126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pablodeymo wants to merge 8 commits into
base: main
Choose a base branch
from
Open

refactor(l1): improve error types with actionable context #6126

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8c18ef1
refactor(l1): improve error types with actionable context
pablodeymo Feb 4, 2026
ce204dd
Merge branch 'main' into refactor/improve-error-types-with-context
pablodeymo Feb 4, 2026
d649175
Fix incorrect error message in handshake: the check for msg_size > P2…
pablodeymo Feb 4, 2026
d76fed8
Merge branch 'main' into refactor/improve-error-types-with-context
pablodeymo Feb 5, 2026
cdb35e7
Add with_context to more type decoders: Block, BlockHeader, BlockBody,
pablodeymo Feb 5, 2026
f4b33d1
Merge branch 'main' into refactor/improve-error-types-with-context
pablodeymo Feb 5, 2026
c28fa2c
Make RLPDecodeError helper constructors const fn and use malformed_da…
pablodeymo Feb 5, 2026
8ec9e3c
Merge branch 'main' into refactor/improve-error-types-with-context
pablodeymo Feb 5, 2026
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
95 changes: 48 additions & 47 deletions crates/common/rlp/decode.rs
View file
Open in desktop

Large diffs are not rendered by default.

66 changes: 50 additions & 16 deletions crates/common/rlp/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,64 @@
use thiserror::Error;

// TODO: improve errors
#[derive(Debug, Error, PartialEq, Eq)]
#[derive(Debug, Error, PartialEq, Eq, Clone)]
pub enum RLPDecodeError {
#[error("InvalidLength")]
InvalidLength,
#[error("MalformedData")]
MalformedData,
#[error("MalformedBoolean")]
MalformedBoolean,
#[error("UnexpectedList")]
UnexpectedList,
#[error("UnexpectedString")]
UnexpectedString,
#[error("InvalidCompression")]
#[error("Invalid RLP length{}", fmt_ctx(.0))]
InvalidLength(Option<&'static str>),
#[error("Malformed RLP data{}", fmt_ctx(.0))]
MalformedData(Option<&'static str>),
#[error("Malformed boolean: expected 0x80 or 0x01, got 0x{0:02x}")]
MalformedBoolean(u8),
#[error("Expected RLP string, got list{}", fmt_ctx(.0))]
UnexpectedList(Option<&'static str>),
#[error("Expected RLP list, got string{}", fmt_ctx(.0))]
UnexpectedString(Option<&'static str>),
#[error("Invalid compression: {0}")]
InvalidCompression(#[from] snap::Error),
#[error("IncompatibleProtocol: {0}")]
#[error("Incompatible protocol: {0}")]
IncompatibleProtocol(String),
#[error("{0}")]
Custom(String),
}

// TODO: improve errors
fn fmt_ctx(ctx: &Option<&'static str>) -> String {
ctx.map(|c| format!(" decoding {c}")).unwrap_or_default()

@ElFantasma ElFantasma Feb 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: fmt_ctx allocates a String on every Display call via format!(). In the None case, unwrap_or_default() returns an empty String (heap-allocated). Since RLP decode errors can appear in P2P logging hot paths, you could avoid the allocation by inlining the formatting:

#[error("Invalid RLP length{}", .0.map(|c| format!(" decoding {c}")).unwrap_or_default())]

Or change the helper to return &str:

fn fmt_ctx(ctx: &Option<&'static str>) -> &'static str {
    // Unfortunately doesn't work with format — 
    // you'd need a different approach
}

Actually the simplest fix: implement Display manually for the variants that need context, writing directly to the formatter without intermediate String. But this is minor — the current approach works, it's just doing a small heap allocation per error display.

}

impl RLPDecodeError {
pub fn invalid_length() -> Self {
Self::InvalidLength(None)
}

pub fn malformed_data() -> Self {
Self::MalformedData(None)
}

pub fn malformed_boolean(got: u8) -> Self {
Self::MalformedBoolean(got)
}

pub fn unexpected_list() -> Self {
Self::UnexpectedList(None)
}

pub fn unexpected_string() -> Self {
Self::UnexpectedString(None)
}

pub fn with_context(self, ctx: &'static str) -> Self {
match self {
Self::InvalidLength(_) => Self::InvalidLength(Some(ctx)),

@ElFantasma ElFantasma Feb 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

with_context unconditionally overwrites any existing context. Currently safe because nested type errors go through field_decode_error → Custom(...) which isn't touched here. But if someone later uses with_context on a type that's decoded inside another with_context-wrapped type without going through Decoder::decode_field, the inner context is silently lost.

Consider either:

  1. A comment documenting "outermost caller wins" behavior
  2. Or: Self::InvalidLength(existing) => Self::InvalidLength(Some(existing.unwrap_or(ctx))) to preserve inner context ("innermost wins")

Self::MalformedData(_) => Self::MalformedData(Some(ctx)),
Self::UnexpectedList(_) => Self::UnexpectedList(Some(ctx)),
Self::UnexpectedString(_) => Self::UnexpectedString(Some(ctx)),
other => other,
}
}
}

#[derive(Debug, Error)]
pub enum RLPEncodeError {
#[error("InvalidCompression")]
#[error("Invalid compression: {0}")]
InvalidCompression(#[from] snap::Error),
#[error("{0}")]
Custom(String),
Expand Down
4 changes: 2 additions & 2 deletions crates/common/rlp/structs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ impl<'a> Decoder<'a> {
pub fn new(buf: &'a [u8]) -> Result<Self, RLPDecodeError> {
match decode_rlp_item(buf)? {
(true, payload, remaining) => Ok(Self { payload, remaining }),
(false, _, _) => Err(RLPDecodeError::UnexpectedString),
(false, _, _) => Err(RLPDecodeError::unexpected_string()),
}
}

Expand Down Expand Up @@ -103,7 +103,7 @@ impl<'a> Decoder<'a> {
if self.payload.is_empty() {
Ok(self.remaining)
} else {
Err(RLPDecodeError::MalformedData)
Err(RLPDecodeError::MalformedData(None))
Comment thread
pablodeymo marked this conversation as resolved.
Outdated
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/common/trie/node_hash.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ impl RLPDecode for NodeHash {
let (hash, rest): (Vec<u8>, &[u8]);
(hash, rest) = RLPDecode::decode_unfinished(rlp)?;
if hash.len() > 32 {
return Err(RLPDecodeError::InvalidLength);
return Err(RLPDecodeError::invalid_length());
}
let hash = NodeHash::from_slice(&hash);
Ok((hash, rest))
Expand Down
8 changes: 4 additions & 4 deletions crates/common/types/account.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,10 +295,10 @@ impl RLPDecode for AccountStateSlimCodec {
let data;
(data, rlp) = rlp
.split_first_chunk::<32>()
.ok_or(RLPDecodeError::InvalidLength)?;
.ok_or(RLPDecodeError::invalid_length())?;
H256(*data)
}
_ => return Err(RLPDecodeError::InvalidLength),
_ => return Err(RLPDecodeError::invalid_length()),
};

Ok((Self(value), rlp))
Expand All @@ -314,10 +314,10 @@ impl RLPDecode for AccountStateSlimCodec {
let data;
(data, rlp) = rlp
.split_first_chunk::<32>()
.ok_or(RLPDecodeError::InvalidLength)?;
.ok_or(RLPDecodeError::invalid_length())?;
H256(*data)
}
_ => return Err(RLPDecodeError::InvalidLength),
_ => return Err(RLPDecodeError::invalid_length()),
};

Ok((Self(value), rlp))
Expand Down
2 changes: 1 addition & 1 deletion crates/common/types/receipt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ impl RLPDecode for ReceiptWithBloom {
// to check for bytes prefix to diferenticate between legacy receipts and non-legacy receipt payloads
let (tx_type, rlp) = if is_encoded_as_bytes(rlp)? {
let payload = get_rlp_bytes_item_payload(rlp)?;
let tx_type = match payload.first().ok_or(RLPDecodeError::InvalidLength)? {
let tx_type = match payload.first().ok_or(RLPDecodeError::invalid_length())? {
0x0 => TxType::Legacy,
0x1 => TxType::EIP2930,
0x2 => TxType::EIP1559,
Expand Down
144 changes: 76 additions & 68 deletions crates/common/types/transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,39 +83,43 @@ impl RLPEncode for P2PTransaction {

impl RLPDecode for P2PTransaction {
fn decode_unfinished(rlp: &[u8]) -> Result<(Self, &[u8]), RLPDecodeError> {
let (is_list, payload, remainder) = decode_rlp_item(rlp)?;
if !is_list {
let tx_type = payload.first().ok_or(RLPDecodeError::InvalidLength)?;
let tx_encoding = &payload.get(1..).ok_or(RLPDecodeError::InvalidLength)?;
// Look at the first byte to check if it corresponds to a TransactionType
match *tx_type {
// Legacy
0x0 => LegacyTransaction::decode(tx_encoding)
.map(|tx| (P2PTransaction::LegacyTransaction(tx), remainder)), // TODO: check if this is a real case scenario
// EIP2930
0x1 => EIP2930Transaction::decode(tx_encoding)
.map(|tx| (P2PTransaction::EIP2930Transaction(tx), remainder)),
// EIP1559
0x2 => EIP1559Transaction::decode(tx_encoding)
.map(|tx| (P2PTransaction::EIP1559Transaction(tx), remainder)),
// EIP4844
0x3 => WrappedEIP4844Transaction::decode(tx_encoding)
.map(|tx| (P2PTransaction::EIP4844TransactionWithBlobs(tx), remainder)),
// EIP7702
0x4 => EIP7702Transaction::decode(tx_encoding)
.map(|tx| (P2PTransaction::EIP7702Transaction(tx), remainder)),
// FeeToken
0x7d => FeeTokenTransaction::decode(tx_encoding)
.map(|tx| (P2PTransaction::FeeTokenTransaction(tx), remainder)),
ty => Err(RLPDecodeError::Custom(format!(
"Invalid transaction type: {ty}"
))),
}
} else {
// LegacyTransaction
LegacyTransaction::decode_unfinished(rlp)
.map(|(tx, rem)| (P2PTransaction::LegacyTransaction(tx), rem))
decode_p2p_transaction(rlp).map_err(|e| e.with_context("P2PTransaction"))
}
}

fn decode_p2p_transaction(rlp: &[u8]) -> Result<(P2PTransaction, &[u8]), RLPDecodeError> {
let (is_list, payload, remainder) = decode_rlp_item(rlp)?;
if !is_list {
let tx_type = payload.first().ok_or(RLPDecodeError::invalid_length())?;
let tx_encoding = &payload.get(1..).ok_or(RLPDecodeError::invalid_length())?;
// Look at the first byte to check if it corresponds to a TransactionType
match *tx_type {
// Legacy
0x0 => LegacyTransaction::decode(tx_encoding)
.map(|tx| (P2PTransaction::LegacyTransaction(tx), remainder)), // TODO: check if this is a real case scenario
// EIP2930
0x1 => EIP2930Transaction::decode(tx_encoding)
.map(|tx| (P2PTransaction::EIP2930Transaction(tx), remainder)),
// EIP1559
0x2 => EIP1559Transaction::decode(tx_encoding)
.map(|tx| (P2PTransaction::EIP1559Transaction(tx), remainder)),
// EIP4844
0x3 => WrappedEIP4844Transaction::decode(tx_encoding)
.map(|tx| (P2PTransaction::EIP4844TransactionWithBlobs(tx), remainder)),
// EIP7702
0x4 => EIP7702Transaction::decode(tx_encoding)
.map(|tx| (P2PTransaction::EIP7702Transaction(tx), remainder)),
// FeeToken
0x7d => FeeTokenTransaction::decode(tx_encoding)
.map(|tx| (P2PTransaction::FeeTokenTransaction(tx), remainder)),
ty => Err(RLPDecodeError::Custom(format!(
"Invalid transaction type: {ty}"
))),
}
} else {
// LegacyTransaction
LegacyTransaction::decode_unfinished(rlp)
.map(|(tx, rem)| (P2PTransaction::LegacyTransaction(tx), rem))
}
}

Expand Down Expand Up @@ -440,42 +444,46 @@ impl RLPDecode for Transaction {
/// B) Non legacy transactions: rlp(Bytes) where Bytes represents the canonical encoding for the transaction as a bytes object.
/// Checkout [Transaction::decode_canonical] for more information
fn decode_unfinished(rlp: &[u8]) -> Result<(Self, &[u8]), RLPDecodeError> {
let (is_list, payload, remainder) = decode_rlp_item(rlp)?;
if !is_list {
let tx_type = payload.first().ok_or(RLPDecodeError::InvalidLength)?;
let tx_encoding = &payload.get(1..).ok_or(RLPDecodeError::InvalidLength)?;
// Look at the first byte to check if it corresponds to a TransactionType
match *tx_type {
// Legacy
0x0 => LegacyTransaction::decode(tx_encoding)
.map(|tx| (Transaction::LegacyTransaction(tx), remainder)), // TODO: check if this is a real case scenario
// EIP2930
0x1 => EIP2930Transaction::decode(tx_encoding)
.map(|tx| (Transaction::EIP2930Transaction(tx), remainder)),
// EIP1559
0x2 => EIP1559Transaction::decode(tx_encoding)
.map(|tx| (Transaction::EIP1559Transaction(tx), remainder)),
// EIP4844
0x3 => EIP4844Transaction::decode(tx_encoding)
.map(|tx| (Transaction::EIP4844Transaction(tx), remainder)),
// EIP7702
0x4 => EIP7702Transaction::decode(tx_encoding)
.map(|tx| (Transaction::EIP7702Transaction(tx), remainder)),
// FeeToken
0x7d => FeeTokenTransaction::decode(tx_encoding)
.map(|tx| (Transaction::FeeTokenTransaction(tx), remainder)),
// PrivilegedL2
0x7e => PrivilegedL2Transaction::decode(tx_encoding)
.map(|tx| (Transaction::PrivilegedL2Transaction(tx), remainder)),
ty => Err(RLPDecodeError::Custom(format!(
"Invalid transaction type: {ty}"
))),
}
} else {
// LegacyTransaction
LegacyTransaction::decode_unfinished(rlp)
.map(|(tx, rem)| (Transaction::LegacyTransaction(tx), rem))
decode_transaction(rlp).map_err(|e| e.with_context("Transaction"))
}
}

fn decode_transaction(rlp: &[u8]) -> Result<(Transaction, &[u8]), RLPDecodeError> {
let (is_list, payload, remainder) = decode_rlp_item(rlp)?;
if !is_list {
let tx_type = payload.first().ok_or(RLPDecodeError::invalid_length())?;
let tx_encoding = &payload.get(1..).ok_or(RLPDecodeError::invalid_length())?;
// Look at the first byte to check if it corresponds to a TransactionType
match *tx_type {
// Legacy
0x0 => LegacyTransaction::decode(tx_encoding)
.map(|tx| (Transaction::LegacyTransaction(tx), remainder)), // TODO: check if this is a real case scenario
// EIP2930
0x1 => EIP2930Transaction::decode(tx_encoding)
.map(|tx| (Transaction::EIP2930Transaction(tx), remainder)),
// EIP1559
0x2 => EIP1559Transaction::decode(tx_encoding)
.map(|tx| (Transaction::EIP1559Transaction(tx), remainder)),
// EIP4844
0x3 => EIP4844Transaction::decode(tx_encoding)
.map(|tx| (Transaction::EIP4844Transaction(tx), remainder)),
// EIP7702
0x4 => EIP7702Transaction::decode(tx_encoding)
.map(|tx| (Transaction::EIP7702Transaction(tx), remainder)),
// FeeToken
0x7d => FeeTokenTransaction::decode(tx_encoding)
.map(|tx| (Transaction::FeeTokenTransaction(tx), remainder)),
// PrivilegedL2
0x7e => PrivilegedL2Transaction::decode(tx_encoding)
.map(|tx| (Transaction::PrivilegedL2Transaction(tx), remainder)),
ty => Err(RLPDecodeError::Custom(format!(
"Invalid transaction type: {ty}"
))),
}
} else {
// LegacyTransaction
LegacyTransaction::decode_unfinished(rlp)
.map(|(tx, rem)| (Transaction::LegacyTransaction(tx), rem))
}
}

Expand All @@ -498,7 +506,7 @@ impl RLPEncode for TxKind {

impl RLPDecode for TxKind {
fn decode_unfinished(rlp: &[u8]) -> Result<(Self, &[u8]), RLPDecodeError> {
let first_byte = rlp.first().ok_or(RLPDecodeError::InvalidLength)?;
let first_byte = rlp.first().ok_or(RLPDecodeError::invalid_length())?;
if *first_byte == RLP_NULL {
return Ok((Self::Create, &rlp[1..]));
}
Expand Down
2 changes: 1 addition & 1 deletion crates/networking/p2p/discv4/messages.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ impl Message {
let (enr_response_msg, _rest) = ENRResponseMessage::decode_unfinished(msg)?;
Ok(Message::ENRResponse(enr_response_msg))
}
_ => Err(RLPDecodeError::MalformedData),
_ => Err(RLPDecodeError::malformed_data()),
}
}

Expand Down
4 changes: 2 additions & 2 deletions crates/networking/p2p/discv5/messages.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ impl Message {
}

pub fn decode(message: &[u8]) -> Result<Message, RLPDecodeError> {
let &message_type = message.first().ok_or(RLPDecodeError::InvalidLength)?;
let &message_type = message.first().ok_or(RLPDecodeError::invalid_length())?;
match message_type {
0x01 => {
let ping = PingMessage::decode(&message[1..])?;
Expand Down Expand Up @@ -543,7 +543,7 @@ impl Message {
let ticket_msg = TicketMessage::decode(&message[1..])?;
Ok(Message::Ticket(ticket_msg))
}
_ => Err(RLPDecodeError::MalformedData),
_ => Err(RLPDecodeError::malformed_data()),
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions crates/networking/p2p/metrics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ impl Metrics {
.and_modify(|e| *e += 1)
.or_insert(1);
}
PeerConnectionError::NoMatchingCapabilities => {
PeerConnectionError::NoMatchingCapabilities(_) => {
failures_grouped_by_reason
.entry("NoMatchingCapabilities".to_owned())
.and_modify(|e| *e += 1)
Expand Down Expand Up @@ -398,7 +398,7 @@ impl Metrics {
.and_modify(|e| *e += 1)
.or_insert(1);
}
PeerConnectionError::InvalidPeerId => {
PeerConnectionError::InvalidPeerId(_) => {
failures_grouped_by_reason
.entry("InvalidPeerId".to_owned())
.and_modify(|e| *e += 1)
Expand All @@ -410,7 +410,7 @@ impl Metrics {
.and_modify(|e| *e += 1)
.or_insert(1);
}
PeerConnectionError::InvalidMessageLength => {
PeerConnectionError::InvalidMessageLength(_) => {
failures_grouped_by_reason
.entry("InvalidMessageLength".to_owned())
.and_modify(|e| *e += 1)
Expand Down
4 changes: 3 additions & 1 deletion crates/networking/p2p/rlpx/connection/codec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,9 @@ impl Decoder for RLPxCodec {
// Check that the size is not too large to avoid a denial of
// service attack where the server runs out of memory.
if padded_size > MAX_MESSAGE_SIZE {
return Err(PeerConnectionError::InvalidMessageLength);
return Err(PeerConnectionError::InvalidMessageLength(
"frame exceeds max size",
));
}

let total_message_size = (32 + padded_size + 16) as usize;
Expand Down
Loading