Implement the sender side of the validation communication protocol in Rust #4280
Conversation
…s more generic (do not require BufReader or BufWriter)
❌ 9 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #4280 +/- ##
===========================================
- Coverage 57.12% 32.59% -24.54%
===========================================
Files 476 476
Lines 56785 56785
===========================================
- Hits 32441 18511 -13930
- Misses 19506 35038 +15532
+ Partials 4838 3236 -1602 |
bragaigor
left a comment
bragaigor
left a comment
There was a problem hiding this comment.
Overall looks good and seems that most of my comments are nitpicks :)
changelog/pmikolajczyk-nit-4364.md
Outdated
| @@ -0,0 +1,3 @@ | |||
| ### Ignored | |||
There was a problem hiding this comment.
| ### Ignored | |
| ### Internal |
There was a problem hiding this comment.
I missed that we added this section, thanks! fixed in eab1044
There was a problem hiding this comment.
The idea is that the ones introduced in #4269 to be substituted for these ones right?
There was a problem hiding this comment.
exactly - actually, I was partly basing this PR on your code
| if input.has_delayed_msg { | ||
| env.delayed_messages | ||
| .insert(input.delayed_msg_nr, input.delayed_msg); | ||
| } |
There was a problem hiding this comment.
will there always be just one delayed_message? Asking because before, just like sequencer_messages, delayed_messages was wrapped in a while loop
There was a problem hiding this comment.
citing Tsahi:
Currently - it can only contain a single delayed message. There are future thoughs about having multiple delayed messages in a single block but that'll only happen after MEL and we could tackle it then.
There was a problem hiding this comment.
so there's some lack of consistency in the code, but right now I tried to persist the existing behavior and data structs
crates/jit/src/wavmio.rs
Outdated
| let hash = socket::read_bytes32(stream)?; | ||
| let preimage = socket::read_bytes(stream)?; | ||
| for (preimage_type, preimages) in input.preimages { | ||
| let map = env.preimages.entry(preimage_type).or_default(); |
There was a problem hiding this comment.
nitpick: map is a bit too generic and I know it was already there, maybe we could rename to something like preimage_map? Up to you
| batch_info: inbox, | ||
| delayed_msg: delayed_message.data, | ||
| start_state, | ||
| user_wasms: HashMap::from([(local_target(), user_wasms)]), |
There was a problem hiding this comment.
why having a HashMap instead of just user_wasms?
There was a problem hiding this comment.
because ValidationInput has HashMap here, in case someone wants to pass wasms for multiple different archs
| pub type IOResult<T> = Result<T, io::Error>; | ||
|
|
||
| const SUCCESS: u8 = 0x0; | ||
| const FAILURE: u8 = 0x1; | ||
| // const PREIMAGE: u8 = 0x2; // legacy, not used | ||
| const ANOTHER: u8 = 0x3; | ||
| const READY: u8 = 0x4; |
There was a problem hiding this comment.
nitpick: I don't have a strong opinion but maybe these could go into their own file constants.rs? and keep mod.rs with just the mods?
There was a problem hiding this comment.
moved to markers.rs to make it more explicit what constants they are
crates/validation/src/lib.rs
Outdated
| if self.has_delayed_msg { | ||
| Some(BatchInfo { | ||
| number: self.delayed_msg_nr, | ||
| data: self.delayed_msg.clone(), | ||
| }) | ||
| } else { | ||
| None | ||
| } |
There was a problem hiding this comment.
nitpick suggestion
| if self.has_delayed_msg { | |
| Some(BatchInfo { | |
| number: self.delayed_msg_nr, | |
| data: self.delayed_msg.clone(), | |
| }) | |
| } else { | |
| None | |
| } | |
| self.has_delayed_msg.then(|| BatchInfo { | |
| number: self.delayed_msg_nr, | |
| data: self.delayed_msg.clone(), | |
| }) |
crates/validation/src/lib.rs
Outdated
| pub fn local_target() -> String { | ||
| match (env::consts::OS, env::consts::ARCH) { | ||
| ("linux", "aarch64") => "arm64", | ||
| ("linux", "x86_64") => "amd64", | ||
| _ => "host", | ||
| } | ||
| .to_string() | ||
| } |
There was a problem hiding this comment.
nitpick suggestion coming from #4269
| pub fn local_target() -> String { | |
| match (env::consts::OS, env::consts::ARCH) { | |
| ("linux", "aarch64") => "arm64", | |
| ("linux", "x86_64") => "amd64", | |
| _ => "host", | |
| } | |
| .to_string() | |
| } | |
| pub fn local_target() -> &'static str { | |
| if cfg!(all(target_os = "linux", target_arch = "aarch64")) { | |
| TARGET_ARM_64 | |
| } else if cfg!(all(target_os = "linux", target_arch = "x86_64")) { | |
| TARGET_AMD_64 | |
| } else { | |
| TARGET_HOST | |
| } | |
| } |
then the const can go into the contstants.rs file if you decide to create one:
pub(crate) const TARGET_ARM_64: &str = "arm64";
pub(crate) const TARGET_AMD_64: &str = "amd64";
pub(crate) const TARGET_HOST: &str = "host";Again up to you
There was a problem hiding this comment.
applied suggestion ✅
| use std::io::ErrorKind::InvalidData; | ||
| use std::io::{Error, Write}; | ||
|
|
||
| pub fn send_validation_input(writer: &mut impl Write, input: &ValidationInput) -> IOResult<()> { |
There was a problem hiding this comment.
it seems like this and send_batches, send_preimages, and send_user_wasms are only used in tests? Unless I missed their caller? If so, wouldn't it be best to move them down to the test file itself? Or we have plans to use them in the future somewhere?
There was a problem hiding this comment.
hopefully all these will be used in your continuous mode (replacing existing inlined protocol) :)
There was a problem hiding this comment.
true, that would simplify things
jittovalidationcrate.ReadandWritetraits, rather than concrete types).Note: the API is synchronous. We don't need async ops at the moment.
closes NIT-4364