-
Notifications
You must be signed in to change notification settings - Fork 30
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.
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
Adding vbs support #2666
Adding vbs support #2666
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense to me overall -- left some minor comments + questions to check my understanding!
crates/constants/src/lib.rs
Outdated
@@ -1,6 +1,6 @@ | |||
//! configurable constants for hotshot | |||
|
|||
use serde::{Deserialize, Serialize}; | |||
use versioned_binary_serialization::version::{StaticVersion, Version}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check my understanding: is Version
a value-level version and StaticVersion
a type-level version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We need the type-level version in a number of places... if Rust's const generics were more mature, I'd be using it as a binding in place of the MAJOR/MINOR thing as well... but having a value Version is also useful in a number of places.
crates/constants/src/lib.rs
Outdated
/// Constant for protocol version 0.1. | ||
pub const VERSION_0_1: Version = Version { major: 0, minor: 1 }; | ||
|
||
/// Constant for protocol static version 0.1. | ||
pub const STATIC_V_0_1: StaticVersion<0, 1> = StaticVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit/minor (feel free to ignore): could we call this STATIC_VERSION_0_1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It got really verbose in places, I wanted a shorter name. But the _0_1
thing is going to go away as soon as I finish working out a few more things, in favor of generic _BASE
and _UPDATE
containers, and there will be a ::StaticVersion
on those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this better?
crates/constants/src/lib.rs
Outdated
/// Constants for WebServerNetwork and WebServer | ||
/// The Web CDN is not, strictly speaking, bound to the network; it can have its own versioning. | ||
/// Web Server CDN Version (major) | ||
pub const WEB_SERVER_MAJOR: u16 = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit/minor (feel free to ignore): could we call this WEB_SERVER_MAJOR_VERSION
?
@@ -263,12 +263,12 @@ mod tests { | |||
// Check the QC and the QCParams can be serialized / deserialized | |||
assert_eq!( | |||
qc, | |||
bincode::deserialize(&bincode::serialize(&qc).unwrap()).unwrap() | |||
Serializer::<0, 1>::deserialize(&Serializer::<0, 1>::serialize(&qc).unwrap()).unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this a round-trip tests that should really work in any version, should this (and L271 in the same file) be Serializer::CURRENT_STATIC_VERSION
? (Should we have a CURRENT_STATIC_VERSION
? We could alias it to STATIC_VERSION_0_1
, but then we wouldn't have to update it here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There does need to be a CURRENT_BASE_VERSION
, and a CURRENT_UPGRADE_VERSION
, and a compile-time check to determine if they are the same (e.g. no upgrade available), but aliasing won't work. I'm working on a solution using a generic type to impl on the core types, but I haven't worked it out yet. I left this (and a few other places) as-is because they're not actually hooked up to the real versioning.
@@ -64,7 +64,7 @@ pub async fn counter_handle_network_event( | |||
match event { | |||
IsBootstrapped => {} | |||
GossipMsg(m, _) | DirectResponse(m, _) => { | |||
if let Ok(msg) = bincode_opts().deserialize::<CounterMessage>(&m) { | |||
if let Ok(msg) = Serializer::<0, 1>::deserialize::<CounterMessage>(&m) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this (+ the other <0,1>
and STATIC_V_0_1
in this file) are hardcoded to a version, even though the functions they're called in are not explicitly versioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... this one, specifically, might be a problem. I was less stringent about using real versions in tests, but I'm not sure this is uniform in all directions. I'll take a closer look.
crates/orchestrator/src/lib.rs
Outdated
|
||
/// Orchestrator is not, strictly speaking, bound to the network; it can have its own versioning. | ||
/// Orchestrator Version (major) | ||
pub const ORCHESTRATOR_MAJOR: u16 = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit/minor (feel free to ignore): I think this should be ORCHESTRATOR_MAJOR_VERSION
@@ -184,7 +187,7 @@ async fn memory_network_direct_queue() { | |||
// Send messages | |||
for sent_message in first_messages { | |||
network1 | |||
.direct_message(sent_message.clone(), pub_key_2) | |||
.direct_message(sent_message.clone(), pub_key_2, STATIC_V_0_1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments re: hard-coding versions without versioning function
|
||
assert_eq!(version.major, major_version_read); | ||
assert_eq!(version.minor, minor_version_read); | ||
let version_read = Version::deserialize(&serialized_message).unwrap().0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this actually changes the test -- it's now testing a property of the Serializer
(round-trip version de/serialize = identity), where before it was testing that the binary representation actually had the version in the right place.
I think the previous test was good to have somewhere, though you probably already have these tests in vbs
. I think we could just delete this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there's tests in vbs... This probably should be deleted.
crates/types/src/message.rs
Outdated
@@ -34,9 +33,6 @@ use std::{fmt::Debug, marker::PhantomData}; | |||
#[derive(Serialize, Deserialize, Clone, Debug, Derivative, PartialEq, Eq, Hash)] | |||
#[serde(bound(deserialize = "", serialize = ""))] | |||
pub struct Message<TYPES: NodeType> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking for my own understanding: NodeType
is versioned right? (So Message
is implicitly versioned through that?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not right now, no. I'm working on a way to do that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, a bit of clarification. Nothing implements the trait Versioned
, which has been added in vbs
; it was meant to be added alongside Serialize and Deserialize as a feature macro, e.g.
#[derive(Serialize, Deserialize, Versioned)]
#[base_version(1, 3)]
#[max_version(1, 7)]
struct MyStruct {
}
but I've come to realize that we usually don't really know the actual version in crates that have generic types...
We may end up needing a crate-level version and a mechanism for mapping the versions of crates to the actual version, but that's not trivial. Another solution might be a tagged, but never versioned, repository containing a single "version" crate that gets bound in the executable target's Cargo.lock
, and injects versions into our library crates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Message
is not inherently bound to a specific protocol version, though a change to Message
will force versioning once Versioned
is implemented. Not by way of NodeType
, though.
I'm still trying to come up with a nice way to inject versions into the consensus by way of something coupled to NodeType
, but giving it generic parameters is catastrophically contagious.
Closes EspressoSystems/versioned-binary-serialization#6
This PR:
versioned-binary-serialization
in HotShot, including support through tide-disco and surf-disco.This PR does not:
Key places to review:
ConnectedNetwork
; note that the versions ofWebServerNetwork
andCombinedNetworks
are specific to that network, and not the same as the versions of payloads (broadcast_message
,direct_message
) over those networks.Libp2pNetwork
- I'm not sure I got that right, and it might need its own unique version for those.