[NIT-4255] Rust JIT validator: basic server #4173
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4173 +/- ##
==========================================
- Coverage 33.05% 32.94% -0.11%
==========================================
Files 459 459
Lines 55830 55830
==========================================
- Hits 18453 18395 -58
- Misses 34159 34214 +55
- Partials 3218 3221 +3 |
❌ 5 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
pmikolajczyk41
requested review from
KolbyML,
MishkaRogachev,
bragaigor,
rory-ocl and
tsahee
arbitrator/validator/src/main.rs
Outdated
|
|
||
| /// Initialize `tracing` logging based on the specified format. By default, the logging level is set | ||
| /// to "info" unless overridden by the `RUST_LOG` environment variable. | ||
| fn init_logging(format: LoggingFormat) -> Result<()> { |
There was a problem hiding this comment.
I know right now we have a simple server, but eventually this logic will grow, so wondering if it would be better to create a init.rs file (or telemetry.rs) and move this there. Also that way we keep main.rs simple
There was a problem hiding this comment.
I really like the comment - personally I have tendency to create multiple smaller, granular modules, but I didn't want to push it too far; but with your support, I extracted this function to logging.rs (I was afraid that init.rs might be too general for now)
arbitrator/validator/src/main.rs
Outdated
| .route( | ||
| &format!("{BASE_NAMESPACE}_wasmModuleRoots"), | ||
| get(spawner_endpoints::wasm_module_roots), | ||
| ); |
There was a problem hiding this comment.
same for this, would probably move to it's own function and probably to config.rs or router.rs? That way we can also move const BASE_NAMESPACE: &str = "/validation"; there as well? That should also make our lives easier for testing.
MishkaRogachev
left a comment
MishkaRogachev
left a comment
There was a problem hiding this comment.
A few minor questions
arbitrator/validator/src/config.rs
Outdated
| pub struct ServerConfig { | ||
| /// Host where the server should be run. | ||
| #[clap(long, default_value = "0.0.0.0")] | ||
| pub host: String, |
There was a problem hiding this comment.
I may be worth using SocketAddr to handle invalid host strings
arbitrator/validator/src/config.rs
Outdated
| Json, | ||
| } | ||
|
|
||
| impl FromStr for LoggingFormat { |
There was a problem hiding this comment.
you can add #[serde(rename_all = "lowercase")] to LoggingFormat and fully remove impl FromStr for LoggingFormat
There was a problem hiding this comment.
oh, I didn't know that trick; nice one! 217bd80
KolbyML
left a comment
KolbyML
left a comment
There was a problem hiding this comment.
Overall looks good, I have a few nits, but they aren't blockers by any means. Good work 🔥
| pub struct ValidationRequest { | ||
| id: u64, | ||
| has_delayed_msg: bool, | ||
| delayed_msg_nr: u64, |
There was a problem hiding this comment.
| delayed_msg_nr: u64, | |
| #[serde(rename = "DelayedMsgNr")] | |
| delayed_message_number: u64, |
What does nr mean? Maybe we can use #[serde(rename so we have clearer field names, I assume it means number, but that is a crazy shorthand
There was a problem hiding this comment.
it's pretty funny, because I was sure 'nr' is a commonly used abbreviation, but I just checked it and it turns out it's popular rather in central and eastern Europe 🙃
I followed the suggestion in 227fdf9
|
|
||
| /// Counterpart for Go struct `validator.BatchInfo`. | ||
| #[derive(Clone, Debug, Deserialize, Serialize)] | ||
| #[serde(rename_all = "PascalCase")] // Match Go struct field names |
There was a problem hiding this comment.
| #[serde(rename_all = "PascalCase")] // Match Go struct field names | |
| #[serde(rename_all = "PascalCase")] |
// Match Go struct field names for all these feels a bit redundant, maybe we can put that information in the files doc comment
| // TODO: Implement actual validation logic | ||
| serde_json::to_string(&request.start_state) | ||
| .map_err(|e| format!("Failed to serialize state: {e}",)) | ||
| } | ||
|
|
||
| pub async fn wasm_module_roots() -> impl IntoResponse { | ||
| "[]" // TODO: Figure this out from local replay.wasm |
There was a problem hiding this comment.
are we going to cross post these todo's and inline issue tracker links like was discussed a week or 2 ago?
There was a problem hiding this comment.
I created one ticket: https://linear.app/offchain-labs/issue/NIT-4265/rust-validator-figure-out-max-number-of-workers-optionally-make-it and the other todos are going to be tackled in the next steps (hopefully pretty soon)
| pub fn create_router() -> Router { | ||
| Router::new() | ||
| .route( | ||
| &format!("{BASE_NAMESPACE}_capacity"), |
There was a problem hiding this comment.
| pub fn create_router() -> Router { | |
| Router::new() | |
| .route( | |
| &format!("{BASE_NAMESPACE}_capacity"), | |
| pub fn create_router() -> Router { | |
| Router::new() | |
| .route( | |
| concat!(BASE_NAMESPACE, "_capacity"), |
This is a little more directed/concise I think. But doesn't matter either way
I wish the endpoints were
/validator/capacity but I guess we are conforming to the golang validator
There was a problem hiding this comment.
It might actually be cleaner to put the _ in BASE_NAMESPACE now that I think about it as well
There was a problem hiding this comment.
unfortunately concat doesn't work with constants nor variables, so I couldn't use BASE_NAMESPACE; because of that, I left the code as is, to avoid "{BASE_NAMESPACE}capacity"
pmikolajczyk41
dismissed stale reviews from KolbyML and MishkaRogachev
via
227fdf9
| type Hash = Bytes32; | ||
|
|
||
| /// Counterpart for Go struct `validator.ValidationInput`. | ||
| #[derive(Clone, Debug, Deserialize, Serialize)] | ||
| #[serde(rename_all = "PascalCase")] | ||
| pub struct ValidationRequest { | ||
| id: u64, | ||
| has_delayed_msg: bool, | ||
| #[serde(rename = "DelayedMsgNr")] | ||
| delayed_msg_number: u64, | ||
| preimages: HashMap<PreimageType, Hash>, | ||
| user_wasms: HashMap<String, HashMap<Hash, Vec<u8>>>, | ||
| batch_info: Vec<BatchInfo>, | ||
| delayed_msg: Vec<u8>, | ||
| start_state: GlobalState, | ||
| debug_chain: bool, | ||
| } | ||
|
|
||
| /// Counterpart for Go struct `validator.BatchInfo`. | ||
| #[derive(Clone, Debug, Deserialize, Serialize)] | ||
| #[serde(rename_all = "PascalCase")] | ||
| pub struct BatchInfo { | ||
| number: u64, | ||
| data: Vec<u8>, | ||
| } | ||
|
|
||
| /// Counterpart for Go struct `validator.GoGlobalState`. | ||
| #[derive(Clone, Debug, Deserialize, Serialize)] | ||
| #[serde(rename_all = "PascalCase")] | ||
| pub struct GlobalState { | ||
| block_hash: Hash, | ||
| send_root: Hash, | ||
| batch: u64, | ||
| pos_in_batch: u64, | ||
| } |
There was a problem hiding this comment.
Also shouldn't we avoid type alias's like type Hash = Bytes32;, you can already infer it is a hash by the field's name. Being able to read that the actual type is Bytes32 is very nice.
I think using type alias's for primitives like B256,u64 etc makes the code harder to read. Because then I need to check what the type alias is.
For more complex type definitions like some type with generics it can be nice to use a type alias. If we want to keep the type alias it would be nice if it was instead below the imports above the structs/enums/impl/funcs at least that is where I expect to see them or
use arbutil::{Bytes32 as Hash, PreimageType};
^ do this
But I think this type alias makes the code harder to read in this case, so we should remove it, but up to you.
There was a problem hiding this comment.
B256/Bytes32 is almost always hashes as well, I can't think of an example at the time of my head where you would need it other then hashes
There was a problem hiding this comment.
Yeah I agree with only type aliasing complex types.
Foundations of the Rust validator service:
validatorValidationSpawnerinterface for the validation client)arbutil::PreimageTypeCurrently we have no validation logic - once we have https://linear.app/offchain-labs/issue/NIT-4260/jit-enable-socketless-mode-for-jit-validator, we can integrate the
jitcrate here and go ahead with nitro integration (testing).part of NIT-4255