Skip to content
Merged

TLV PoC #6082

Show file tree
Hide file tree
Changes from 3 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
29 changes: 23 additions & 6 deletions gossip/src/contact_info.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
pub use solana_client::connection_cache::Protocol;
use {
crate::{crds_data::MAX_WALLCLOCK, legacy_contact_info::LegacyContactInfo},
crate::{
crds_data::MAX_WALLCLOCK,
define_tlv_enum,
legacy_contact_info::LegacyContactInfo,
tlv::{self, TlvRecord},
},
assert_matches::{assert_matches, debug_assert_matches},
serde::{Deserialize, Deserializer, Serialize},
solana_pubkey::Pubkey,
Expand Down Expand Up @@ -113,8 +118,19 @@ struct SocketEntry {
offset: u16, // Port offset with respect to the previous entry.
}

#[derive(Clone, Debug, Eq, PartialEq, Deserialize, Serialize)]
enum Extension {}
define_tlv_enum!(
/// TLV encoded Extensions in ContactInfo messages
///
/// On the wire each record is: [type: u8][len: varint][bytes]
/// Extensions with unknown types are skipped by tlv::parse,
/// so new types can be added without breaking legacy code,
/// and support by all clients is not required.
///
/// Always add new TLV records to the end of this enum.
/// Never reorder or reuse a type.
/// Ensure new type collisions do not happen.
pub(crate) enum Extension {}
);

// As part of deserialization, self.addrs and self.sockets should be cross
// verified and self.cache needs to be populated. This type serves as a
Expand All @@ -132,8 +148,9 @@ struct ContactInfoLite {
addrs: Vec<IpAddr>,
#[serde(with = "short_vec")]
sockets: Vec<SocketEntry>,
#[allow(dead_code)]
#[serde(with = "short_vec")]
extensions: Vec<Extension>,
extensions: Vec<TlvRecord>,
Comment on lines -136 to +153

Choose a reason for hiding this comment

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

ahh this is nice. so rollout strategy here is switch to Vec<TlvRecord> and leave empty until everyone adopts. since those not updated will fail to accept/deser ContactInfo with non-empty vecs here. and then we can start populating extensions with TlvRecords.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, pretty much. At that point any client can do whatever they want with Extensions as long as we do not get conflicting tags.

}

macro_rules! get_socket {
Expand Down Expand Up @@ -220,7 +237,7 @@ impl ContactInfo {
version: solana_version::Version::default(),
addrs: Vec::<IpAddr>::default(),
sockets: Vec::<SocketEntry>::default(),
extensions: Vec::<Extension>::default(),
extensions: Vec::default(),
cache: EMPTY_SOCKET_ADDR_CACHE,
}
}
Expand Down Expand Up @@ -549,7 +566,7 @@ impl TryFrom<ContactInfoLite> for ContactInfo {
version,
addrs,
sockets,
extensions,
extensions: tlv::parse(&extensions),
cache: EMPTY_SOCKET_ADDR_CACHE,
};
// Populate node.cache.
Expand Down
2 changes: 2 additions & 0 deletions gossip/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ pub mod epoch_specs;
pub mod gossip_error;
pub mod gossip_service;
#[macro_use]
mod tlv;
#[macro_use]
mod legacy_contact_info;
pub mod ping_pong;
mod protocol;
Expand Down
159 changes: 159 additions & 0 deletions gossip/src/tlv.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
use {
serde::{Deserialize, Serialize},
solana_short_vec as short_vec,
};

/// Type-Length-Value encoding wrapper for bincode
#[derive(Clone, Debug, Eq, PartialEq, Deserialize, Serialize)]
pub(crate) struct TlvRecord {
// type
pub(crate) typ: u8,
// length and serialized bytes of the value
#[serde(with = "short_vec")]
pub(crate) bytes: Vec<u8>,
}

/// Macro that provides a quick and easy way to define TLV compatible enums
#[macro_export]
macro_rules! define_tlv_enum {
(
$(#[$meta:meta])*
$vis:vis enum $enum_name:ident {
$($typ:literal => $variant:ident($inner:ty)),* $(,)?
}
) => {
// add the doc-comment if present
$(#[$meta])*
// define the enum itself
#[derive(Debug, Clone, Eq, PartialEq)]
$vis enum $enum_name {
$(
$variant($inner),
)*
}

// Serialize enum by first converting into TlvRecord, and then serializing that
impl serde::Serialize for $enum_name {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: serde::Serializer,
{
let tlv_rec = TlvRecord::try_from(self).map_err(|e| serde::ser::Error::custom(e))?;
tlv_rec.serialize(serializer)
}
}

// define conversion from TLV wire format
impl TryFrom<&TlvRecord> for $enum_name {
type Error = bincode::Error;
fn try_from(value: &TlvRecord) -> Result<Self, Self::Error> {
use serde::de::Error;
match value.typ {
$(
$typ => Ok(Self::$variant(bincode::deserialize::<$inner>(&value.bytes)?)),
)*
_ => Err(bincode::Error::custom("Invalid type")),

Choose a reason for hiding this comment

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

maybe would be nice to differentiate between invalid type and some other decode failure.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure how that would work to be honest, can you elaborate?

Choose a reason for hiding this comment

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

well the code kind of differentiates already. since if the type is valid we just return some bincode::Error::<error> and if the type is invalid then we get bincode::Error::custom("invalid type").

but from a caller error handling/clarity/debugging perspective it may be nice to provide a way to match on the error rather than having to do match err.kind() { ... } and possibly have to do string matching.
we could wrap the error in some TlvDecodeError like:

pub enum TlvDecodeError {
    #[error("Unknown type: {0}")]
    UnknownType(u8),
    #[error("Malformed payload: {0}")]
    MalformedPayload(#[from] bincode::Error),
}

or change the error string to 'invalid tlv type" so we don't get crossed up with someone in the future defining a bincode error with "invalid type".

won't die on this hill. but would just clean/clear up error messages and handling.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah good idea, I'll put that in. Thanks!

}
}
}
// define conversion into TLV wire format
impl TryFrom<&$enum_name> for TlvRecord {
type Error = bincode::Error;
fn try_from(value: &$enum_name) -> Result<Self, Self::Error> {
use serde::ser::Error;
match value {
$(
$enum_name::$variant(inner) => Ok(TlvRecord {
typ: $typ,
bytes: bincode::serialize(inner)?,
}),
)*
#[allow(unreachable_patterns)]
_ => Err(bincode::Error::custom("Unsupported enum variant")),
}
}
}
};
}

/// Parses a slice of serialized TLV records into a provided type. Unsupported
/// TLV records are ignored.
pub(crate) fn parse<'a, T: TryFrom<&'a TlvRecord>>(entries: &'a [TlvRecord]) -> Vec<T> {
entries.iter().filter_map(|v| T::try_from(v).ok()).collect()
}
Comment on lines +88 to +90

Choose a reason for hiding this comment

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

may not be a huge deal since its a sender issue but if there are multiple TlvRecords with the same typ we just keep the last one.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure why that would be the case. It will skip invalid entries, but will happily produce as many identical TLVs as there are in the original entries slice. Granted, the Vec allocation here is grotesque, but its a PoC =)

Choose a reason for hiding this comment

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

ya turns out i can't read lol. ignore


#[cfg(test)]
mod tests {
use crate::{define_tlv_enum, tlv::TlvRecord};

define_tlv_enum! (pub(crate) enum ExtensionNew {
1=>Test(u64),
2=>LegacyString(String),
3=>NewString(String),
});

define_tlv_enum! ( pub(crate) enum ExtensionLegacy {
1=>Test(u64),
2=>LegacyString(String),
});

/// Test that TLV encoded data is backwards-compatible,
/// i.e. that new TLV data can be decoded by a new
/// receiver where possible, and skipped otherwise
#[test]
fn test_tlv_backwards_compat() {
let new_tlv_data = vec![
ExtensionNew::Test(42),
ExtensionNew::NewString(String::from("bla")),
];

let new_bytes = bincode::serialize(&new_tlv_data).unwrap();
let tlv_vec: Vec<TlvRecord> = bincode::deserialize(&new_bytes).unwrap();
// check that both TLV are encoded correctly
let new: Vec<ExtensionNew> = crate::tlv::parse(&tlv_vec);
assert!(matches!(new[0], ExtensionNew::Test(42)));
if let ExtensionNew::NewString(s) = &new[1] {
assert_eq!(s, "bla");
} else {
panic!("Wrong deserialization")
};
// Make sure legacy recover works correctly
let legacy: Vec<ExtensionLegacy> = crate::tlv::parse(&tlv_vec);
assert!(matches!(legacy[0], ExtensionLegacy::Test(42)));
assert_eq!(
legacy.len(),
1,
"Legacy parser should only recover 1 entry"
)
}

/// Test that TLV encoded data is forwards-compatible,
/// i.e. that legacy TLV data can be decoded by a new
/// receiver
#[test]
fn test_tlv_forward_compat() {
let legacy_tlv_data = vec![
ExtensionLegacy::Test(42),
ExtensionLegacy::LegacyString(String::from("foo")),
];
let legacy_bytes = bincode::serialize(&legacy_tlv_data).unwrap();

let tlv_vec: Vec<TlvRecord> = bincode::deserialize(&legacy_bytes).unwrap();
// Just in case make sure that legacy data is serialized correctly
let legacy: Vec<ExtensionLegacy> = crate::tlv::parse(&tlv_vec);
assert!(matches!(legacy[0], ExtensionLegacy::Test(42)));
if let ExtensionLegacy::LegacyString(s) = &legacy[1] {
assert_eq!(s, "foo");
} else {
panic!("Wrong deserialization")
};
// Parse the same bytes using new parser
let new: Vec<ExtensionNew> = crate::tlv::parse(&tlv_vec);
assert!(matches!(new[0], ExtensionNew::Test(42)));
if let ExtensionNew::LegacyString(s) = &new[1] {
assert_eq!(s, "foo");
} else {
panic!("Wrong deserialization")
};
}
}
Loading