protocol: Refactor encoding

What made the issue fixed in the parent commit harder to spot
for me was the multitude of concepts and terms involved in
encoding: What's *serializing* vs. *encoding*? Why is there
this odd `Frame::to_bytes`?

So I decided to clean up a bit. I removed `fn serialize` so that the
logic stays close to the data, and instead of it provide
`fn encode_vec` in `trait Encode` so that we don't add up with
one-offs like `Frame::to_bytes` again.

Also we make use of `encode_vec` in the tests.
This commit is contained in:
Lorenz Leutgeb 2025-08-20 15:34:02 +02:00 committed by Fintan Halpenny
parent a8426dfdac
commit cbd2a7070c
7 changed files with 90 additions and 94 deletions

View File

@ -427,7 +427,8 @@ where
stream: task.stream,
},
);
self.actions.push_back(Action::Send(fd, frame.to_bytes()));
self.actions
.push_back(Action::Send(fd, frame.encode_to_vec()));
}
} else {
// If the peer disconnected, we'll get here, but we still want to let the service know
@ -479,7 +480,7 @@ where
ChannelEvent::Eof => Frame::control(*link, frame::Control::Eof { stream }),
};
self.actions
.push_back(reactor::Action::Send(fd, frame.to_bytes()));
.push_back(reactor::Action::Send(fd, frame.encode_to_vec()));
}
}
@ -1126,7 +1127,7 @@ where
self.actions.push_back(Action::Send(
fd,
Frame::<service::Message>::control(link, frame::Control::Open { stream })
.to_bytes(),
.encode_to_vec(),
));
}
}
@ -1264,7 +1265,7 @@ mod test {
frame::StreamId::gossip(Link::Outbound).encode(&mut stream);
// Serialize gossip message with some extension fields.
let mut gossip = wire::serialize(&pong);
let mut gossip = pong.encode_to_vec();
String::from("extra").encode(&mut gossip);
48u8.encode(&mut gossip);

View File

@ -10,7 +10,7 @@ use crate::service::message::{
Announcement, AnnouncementMessage, InventoryAnnouncement, NodeAnnouncement, RefsAnnouncement,
};
use crate::wire;
use crate::wire::Decode;
use crate::wire::{Decode as _, Encode as _};
use radicle::node::Database;
use radicle::node::NodeId;
use radicle::prelude::Timestamp;
@ -117,17 +117,17 @@ impl Store for Database {
AnnouncementMessage::Node(msg) => {
stmt.bind((2, sql::Value::String(String::new())))?;
stmt.bind((3, &GossipType::Node))?;
stmt.bind((4, msg))?;
stmt.bind((4, &msg.encode_to_vec()[..]))?;
}
AnnouncementMessage::Refs(msg) => {
stmt.bind((2, &msg.rid))?;
stmt.bind((3, &GossipType::Refs))?;
stmt.bind((4, msg))?;
stmt.bind((4, &msg.encode_to_vec()[..]))?;
}
AnnouncementMessage::Inventory(msg) => {
stmt.bind((2, sql::Value::String(String::new())))?;
stmt.bind((3, &GossipType::Inventory))?;
stmt.bind((4, msg))?;
stmt.bind((4, &msg.encode_to_vec()[..]))?;
}
}
stmt.bind((5, &ann.signature))?;
@ -231,12 +231,6 @@ impl TryFrom<&sql::Value> for NodeAnnouncement {
}
}
impl sql::BindableWithIndex for &NodeAnnouncement {
fn bind<I: sql::ParameterIndex>(self, stmt: &mut sql::Statement<'_>, i: I) -> sql::Result<()> {
wire::serialize(self).bind(stmt, i)
}
}
impl TryFrom<&sql::Value> for RefsAnnouncement {
type Error = sql::Error;
@ -254,12 +248,6 @@ impl TryFrom<&sql::Value> for RefsAnnouncement {
}
}
impl sql::BindableWithIndex for &RefsAnnouncement {
fn bind<I: sql::ParameterIndex>(self, stmt: &mut sql::Statement<'_>, i: I) -> sql::Result<()> {
wire::serialize(self).bind(stmt, i)
}
}
impl TryFrom<&sql::Value> for InventoryAnnouncement {
type Error = sql::Error;
@ -277,12 +265,6 @@ impl TryFrom<&sql::Value> for InventoryAnnouncement {
}
}
impl sql::BindableWithIndex for &InventoryAnnouncement {
fn bind<I: sql::ParameterIndex>(self, stmt: &mut sql::Statement<'_>, i: I) -> sql::Result<()> {
wire::serialize(self).bind(stmt, i)
}
}
impl From<wire::Error> for sql::Error {
fn from(other: wire::Error) -> Self {
sql::Error {

View File

@ -16,6 +16,7 @@ use crate::bounded::BoundedVec;
use crate::service::filter::Filter;
use crate::service::{Link, NodeId, Timestamp};
use crate::wire;
use crate::wire::Encode as _;
/// Maximum number of addresses which can be announced to other nodes.
pub const ADDRESS_LIMIT: usize = 16;
@ -69,20 +70,21 @@ impl NodeAnnouncement {
/// Proof-of-work uses the [`scrypt`] algorithm with the parameters in
/// [`Announcement::POW_PARAMS`]. The "work" is calculated by counting the number of leading
/// zero bits after running `scrypt` on a serialized [`NodeAnnouncement`] using
/// [`wire::serialize`].
/// [`Encode::encode_to_vec`].
///
/// In other words, `work = leading-zeros(scrypt(serialize(announcement)))`.
///
/// Higher numbers mean higher difficulty. For each increase in work, difficulty is doubled.
/// For instance, an output of `7` is *four* times more work than an output of `5`.
///
/// [`Encode::encode_to_vec`]: crate::wire::Encode::encode_to_vec
pub fn work(&self) -> u32 {
let (n, r, p) = Announcement::POW_PARAMS;
let params = scrypt::Params::new(n, r, p, 32).expect("proof-of-work parameters are valid");
let mut output = [0u8; 32];
scrypt::scrypt(
wire::serialize(self).as_ref(),
&self.encode_to_vec(),
Announcement::POW_SALT,
&params,
&mut output,
@ -265,7 +267,8 @@ impl AnnouncementMessage {
{
use crypto::signature::Signer as _;
let msg = wire::serialize(&self);
let msg = self.encode_to_vec();
let signature = signer.sign(&msg);
Announcement {
@ -365,7 +368,7 @@ impl Announcement {
/// Verify this announcement's signature.
pub fn verify(&self) -> bool {
let msg = wire::serialize(&self.message);
let msg = self.message.encode_to_vec();
self.node.verify(msg, &self.signature).is_ok()
}
@ -425,8 +428,8 @@ impl PartialOrd for Message {
impl Ord for Message {
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
let this = wire::serialize(self);
let other = wire::serialize(other);
let this = self.encode_to_vec();
let other = other.encode_to_vec();
this.cmp(&other)
}
@ -678,7 +681,6 @@ mod tests {
use radicle::git::raw;
use super::*;
use crate::wire::Encode;
use localtime::LocalTime;
use radicle::test::arbitrary;

View File

@ -7,6 +7,7 @@ pub use message::{AddressType, MessageType};
use std::collections::BTreeMap;
use std::convert::TryFrom;
use std::fmt::Debug;
use std::mem;
use std::ops::Deref;
use std::str::FromStr;
@ -94,7 +95,16 @@ impl From<bytes::TryGetError> for Error {
/// Things that can be encoded as binary.
pub trait Encode {
/// Encode self by writing it to the given buffer.
fn encode(&self, buffer: &mut impl BufMut);
/// A convenience wrapper around [`Encode::encode`]
/// that allocates a [`Vec`].
fn encode_to_vec(&self) -> Vec<u8> {
let mut buf = Vec::new();
self.encode(&mut buf);
buf
}
}
/// Things that can be decoded from binary.
@ -102,13 +112,6 @@ pub trait Decode: Sized {
fn decode(buffer: &mut impl Buf) -> Result<Self, Error>;
}
/// Encode an object into a byte vector.
pub fn serialize<E: Encode + ?Sized>(data: &E) -> Vec<u8> {
let mut buffer = Vec::new();
data.encode(&mut buffer);
buffer
}
/// Decode an object from a slice.
pub fn deserialize<T: Decode>(mut data: &[u8]) -> Result<T, Error> {
let result = T::decode(&mut data)?;
@ -544,22 +547,22 @@ mod tests {
#[quickcheck]
fn prop_u8(input: u8) {
assert_eq!(deserialize::<u8>(&serialize(&input)).unwrap(), input);
assert_eq!(deserialize::<u8>(&input.encode_to_vec()).unwrap(), input);
}
#[quickcheck]
fn prop_u16(input: u16) {
assert_eq!(deserialize::<u16>(&serialize(&input)).unwrap(), input);
assert_eq!(deserialize::<u16>(&input.encode_to_vec()).unwrap(), input);
}
#[quickcheck]
fn prop_u32(input: u32) {
assert_eq!(deserialize::<u32>(&serialize(&input)).unwrap(), input);
assert_eq!(deserialize::<u32>(&input.encode_to_vec()).unwrap(), input);
}
#[quickcheck]
fn prop_u64(input: u64) {
assert_eq!(deserialize::<u64>(&serialize(&input)).unwrap(), input);
assert_eq!(deserialize::<u64>(&input.encode_to_vec()).unwrap(), input);
}
#[quickcheck]
@ -567,7 +570,10 @@ mod tests {
if input.len() > u8::MAX as usize {
return qcheck::TestResult::discard();
}
assert_eq!(deserialize::<String>(&serialize(&input)).unwrap(), input);
assert_eq!(
deserialize::<String>(&input.encode_to_vec()).unwrap(),
input
);
qcheck::TestResult::passed()
}
@ -575,38 +581,44 @@ mod tests {
#[quickcheck]
fn prop_vec(input: BoundedVec<String, 16>) {
assert_eq!(
deserialize::<BoundedVec<String, 16>>(&serialize(&input.as_slice())).unwrap(),
deserialize::<BoundedVec<String, 16>>(&input.encode_to_vec()).unwrap(),
input
);
}
#[quickcheck]
fn prop_pubkey(input: PublicKey) {
assert_eq!(deserialize::<PublicKey>(&serialize(&input)).unwrap(), input);
assert_eq!(
deserialize::<PublicKey>(&input.encode_to_vec()).unwrap(),
input
);
}
#[quickcheck]
fn prop_filter(input: filter::Filter) {
assert_eq!(
deserialize::<filter::Filter>(&serialize(&input)).unwrap(),
deserialize::<filter::Filter>(&input.encode_to_vec()).unwrap(),
input
);
}
#[quickcheck]
fn prop_id(input: RepoId) {
assert_eq!(deserialize::<RepoId>(&serialize(&input)).unwrap(), input);
assert_eq!(
deserialize::<RepoId>(&input.encode_to_vec()).unwrap(),
input
);
}
#[quickcheck]
fn prop_refs(input: Refs) {
assert_eq!(deserialize::<Refs>(&serialize(&input)).unwrap(), input);
assert_eq!(deserialize::<Refs>(&input.encode_to_vec()).unwrap(), input);
}
#[quickcheck]
fn prop_tuple(input: (String, String)) {
assert_eq!(
deserialize::<(String, String)>(&serialize(&input)).unwrap(),
deserialize::<(String, String)>(&input.encode_to_vec()).unwrap(),
input
);
}
@ -616,7 +628,7 @@ mod tests {
let signature = Signature::from(input);
assert_eq!(
deserialize::<Signature>(&serialize(&signature)).unwrap(),
deserialize::<Signature>(&signature.encode_to_vec()).unwrap(),
signature
);
}
@ -625,13 +637,13 @@ mod tests {
fn prop_oid(input: [u8; 20]) {
let oid = git::Oid::try_from(input.as_slice()).unwrap();
assert_eq!(deserialize::<git::Oid>(&serialize(&oid)).unwrap(), oid);
assert_eq!(deserialize::<git::Oid>(&oid.encode_to_vec()).unwrap(), oid);
}
#[quickcheck]
fn prop_signed_refs(input: SignedRefs<Unverified>) {
assert_eq!(
deserialize::<SignedRefs<Unverified>>(&serialize(&input)).unwrap(),
deserialize::<SignedRefs<Unverified>>(&input.encode_to_vec()).unwrap(),
input
);
}
@ -639,7 +651,7 @@ mod tests {
#[test]
fn test_string() {
assert_eq!(
serialize(&String::from("hello")),
String::from("hello").encode_to_vec(),
vec![5, b'h', b'e', b'l', b'l', b'o']
);
}
@ -647,7 +659,7 @@ mod tests {
#[test]
fn test_alias() {
assert_eq!(
serialize(&Alias::from_str("hello").unwrap()),
Alias::from_str("hello").unwrap().encode_to_vec(),
vec![5, b'h', b'e', b'l', b'l', b'o']
);
}
@ -656,7 +668,7 @@ mod tests {
fn test_filter_invalid() {
let b = bloomy::BloomFilter::with_size(filter::FILTER_SIZE_M / 3);
let f = filter::Filter::from(b);
let bytes = serialize(&f);
let bytes = f.encode_to_vec();
assert_matches!(
deserialize::<filter::Filter>(&bytes).unwrap_err(),
@ -667,10 +679,10 @@ mod tests {
#[test]
fn test_bounded_vec_limit() {
let v: BoundedVec<u8, 2> = vec![1, 2].try_into().unwrap();
let buf = serialize(&v);
let buf = &v.encode_to_vec();
assert_matches!(
deserialize::<BoundedVec<u8, 1>>(&buf),
deserialize::<BoundedVec<u8, 1>>(buf),
Err(Error::InvalidSize {
expected: 1,
actual: 2
@ -679,7 +691,7 @@ mod tests {
);
assert!(
deserialize::<BoundedVec<u8, 2>>(&buf).is_ok(),
deserialize::<BoundedVec<u8, 2>>(buf).is_ok(),
"successfully decode vector of same size",
);
}

View File

@ -234,13 +234,6 @@ impl<M> Frame<M> {
}
}
impl<M: wire::Encode> Frame<M> {
/// Serialize frame to bytes.
pub fn to_bytes(&self) -> Vec<u8> {
wire::serialize(self)
}
}
/// Frame payload.
#[derive(Debug, PartialEq, Eq)]
pub enum FrameData<M> {
@ -363,7 +356,7 @@ impl<M: wire::Encode> wire::Encode for Frame<M> {
match &self.data {
FrameData::Control(ctrl) => ctrl.encode(buf),
FrameData::Git(data) => varint::payload::encode(data, buf),
FrameData::Gossip(msg) => varint::payload::encode(&wire::serialize(msg), buf),
FrameData::Gossip(msg) => varint::payload::encode(&msg.encode_to_vec(), buf),
}
}
}
@ -393,6 +386,8 @@ mod test {
#[test]
fn test_encode_git_large() {
use wire::Encode as _;
let size = u16::MAX as usize * 3;
assert!(
size > (wire::Size::MAX as usize * 2),
@ -404,7 +399,7 @@ mod test {
let frame: Frame<Message> = Frame::git(StreamId(0u8.into()), a_lot_of_data);
// In previous versions since 3c5668e this would panic.
let bytes = wire::serialize(&frame);
let bytes = frame.encode_to_vec();
assert!(
bytes.len() > wire::Size::MAX as usize * 2,

View File

@ -446,7 +446,7 @@ mod tests {
});
let ann = ann.signed(&signer);
let msg = Message::Announcement(ann);
let data = wire::serialize(&msg);
let data = msg.encode_to_vec();
assert!(data.len() < wire::Size::MAX as usize);
}
@ -461,7 +461,7 @@ mod tests {
});
let ann = ann.signed(&signer);
let msg = Message::Announcement(ann);
let data = wire::serialize(&msg);
let data = msg.encode_to_vec();
assert!(data.len() < wire::Size::MAX as usize);
}
@ -482,44 +482,48 @@ mod tests {
});
let ann = ann.signed(&signer);
let msg = Message::Announcement(ann);
let data = wire::serialize(&msg);
let data = msg.encode_to_vec();
assert!(data.len() < wire::Size::MAX as usize);
}
#[test]
fn test_pingpong_encode_max_size() {
wire::serialize(&Message::Ping(Ping {
Message::Ping(Ping {
ponglen: 0,
zeroes: ZeroBytes::new(Ping::MAX_PING_ZEROES),
}));
})
.encode_to_vec();
wire::serialize(&Message::Pong {
(Message::Pong {
zeroes: ZeroBytes::new(Ping::MAX_PONG_ZEROES),
});
})
.encode_to_vec();
}
#[test]
#[should_panic(expected = "advance out of bounds")]
fn test_ping_encode_size_overflow() {
wire::serialize(&Message::Ping(Ping {
Message::Ping(Ping {
ponglen: 0,
zeroes: ZeroBytes::new(Ping::MAX_PING_ZEROES + 1),
}));
})
.encode_to_vec();
}
#[test]
#[should_panic(expected = "advance out of bounds")]
fn test_pong_encode_size_overflow() {
wire::serialize(&Message::Pong {
Message::Pong {
zeroes: ZeroBytes::new(Ping::MAX_PONG_ZEROES + 1),
});
}
.encode_to_vec();
}
#[quickcheck]
fn prop_message_encode_decode(message: Message) {
let encoded = &wire::serialize(&message);
let decoded = wire::deserialize::<Message>(encoded).unwrap();
let encoded = message.encode_to_vec();
let decoded = wire::deserialize::<Message>(&encoded).unwrap();
assert_eq!(message, decoded);
}
@ -552,7 +556,7 @@ mod tests {
let zeroes = ZeroBytes::new(zeroes);
assert_eq!(
wire::deserialize::<ZeroBytes>(&wire::serialize(&zeroes)).unwrap(),
wire::deserialize::<ZeroBytes>(&zeroes.encode_to_vec()).unwrap(),
zeroes
);
}
@ -565,7 +569,7 @@ mod tests {
#[quickcheck]
fn prop_addr(addr: Address) {
assert_eq!(
wire::deserialize::<Address>(&wire::serialize(&addr)).unwrap(),
wire::deserialize::<Address>(&addr.encode_to_vec()).unwrap(),
addr
);
}

View File

@ -213,7 +213,7 @@ mod test {
#[quickcheck]
fn prop_encode_decode(input: VarInt) {
let encoded = wire::serialize(&input);
let encoded = input.encode_to_vec();
let decoded: VarInt = wire::deserialize(&encoded).unwrap();
assert_eq!(decoded, input);
@ -222,30 +222,30 @@ mod test {
#[test]
#[should_panic]
fn test_encode_overflow() {
wire::serialize(&VarInt(u64::MAX));
VarInt(u64::MAX).encode_to_vec();
}
#[test]
fn test_encoding() {
assert_eq!(wire::serialize(&VarInt(0)), vec![0x0]);
assert_eq!(wire::serialize(&VarInt(1)), vec![0x01]);
assert_eq!(wire::serialize(&VarInt(10)), vec![0x0a]);
assert_eq!(wire::serialize(&VarInt(37)), vec![0x25]);
assert_eq!(VarInt(0).encode_to_vec(), vec![0x0]);
assert_eq!(VarInt(1).encode_to_vec(), vec![0x01]);
assert_eq!(VarInt(10).encode_to_vec(), vec![0x0a]);
assert_eq!(VarInt(37).encode_to_vec(), vec![0x25]);
assert_eq!(
wire::deserialize::<VarInt>(&[0x40, 0x25]).unwrap(),
VarInt(37)
);
assert_eq!(wire::serialize(&VarInt(15293)), vec![0x7b, 0xbd]);
assert_eq!(VarInt(15293).encode_to_vec(), vec![0x7b, 0xbd]);
assert_eq!(
wire::serialize(&VarInt(494878333)),
VarInt(494878333).encode_to_vec(),
vec![0x9d, 0x7f, 0x3e, 0x7d],
);
assert_eq!(
wire::serialize(&VarInt(151288809941952652)),
VarInt(151288809941952652).encode_to_vec(),
vec![0xc2, 0x19, 0x7c, 0x5e, 0xff, 0x14, 0xe8, 0x8c]
);
assert_eq!(
wire::serialize(&VarInt(10000000000)),
VarInt(10000000000).encode_to_vec(),
vec![0xc0, 0x00, 0x00, 0x02, 0x54, 0x0b, 0xe4, 0x00],
);
}