Add ipc support in --execution-endpoint - #8802
Conversation
|
|
|
Hi team, I took the liberty of opening this PR — we can use it as a starting point for discussion. I’m happy to add test cases for the IPC transport (similar to HTTP); I just wanted to get some feedback on the implementation first. Please let me know your thoughts! @michaelsproul for vis |
1e692da to
233c21b
Compare
|
ummgh, seems |
233c21b to
eb9190e
Compare
|
Since it is no longer possible to fix URL parsing inside SensitiveUrl, I am adding localhost to the IPC URL under the hood so that it passes SensitiveUrl parsing. Buttom line is, it's working and it's ready for first review. |
|
You could also upstream changes to |
|
Thanks for pointing me to the I am happy to accommodate any feedback you have regarding the PR. |
|
@michaelsproul : just a friendly ping |
|
Hey @simonmichal we are all in on Gloas at the moment with little time to review. We'll try to find some time to review this in the coming weeks. |
macladson
left a comment
macladson
left a comment
There was a problem hiding this comment.
In the meantime, can you change the target branch to unstable and we'll get CI running
|
Target branch check will fail until you push a new commit, but looks like there's a small conflict anyway. So should be fine once that's addressed |
michaelsproul
added
the
waiting-on-author
|
@michaelsproul , @macladson : as requested, I've rebase on |
| @@ -544,8 +547,20 @@ impl<E: EthSpec> ExecutionLayer<E> { | |||
| let engine: Engine = { | |||
| let auth = Auth::new(jwt_key, jwt_id, jwt_version); | |||
There was a problem hiding this comment.
So here, we define the authentication to connect to the execution, yet the JWT isn't used at all during IPC. However --execution-jwt is still required for IPC connections. It would be nice to remove the requirement to pass in a jwt if you're connecting over IPC
There was a problem hiding this comment.
Thanks for pointing it out. Done!
| async fn ensure_connected(&self) -> Result<(), Error> { | ||
| let mut conn_guard = self.connection.lock().await; | ||
|
|
||
| if conn_guard.is_none() { | ||
| let connection = Self::create_connection(&self.path).await?; | ||
| *conn_guard = Some(connection); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
I haven't actually tested this, but I believe since we are only checking for conn_guard.is_none(), once we have established a connection, conn_guard will then forever be Some. So if the connection ever drops (the EL goes down briefly or restarts) we will never be able to reconnect since the connection is closed at the kernel level.
We would need to add error detection and if there is an IO error we set conn_guard back to None so it grabs a fresh UnixStream
There was a problem hiding this comment.
Thanks for the feedback! I have added error handling logic that on connection erros will reset the connection to None - tested it by killing and then restarting reth, which now works. Let me know if it looks OK now?
macladson
left a comment
macladson
left a comment
There was a problem hiding this comment.
Oh one more thing, can you add a CLI test for the IPC version of the --execution-endpoint? It will need new_with_no_execution_endpoint so you can override the flag like we do for this test:
lighthouse/lighthouse/tests/beacon_node.rs
Lines 266 to 268 in 7dab32d
| conn.writer.flush().await?; | ||
|
|
||
| let mut response = String::new(); | ||
| conn.reader.read_line(&mut response).await?; |
There was a problem hiding this comment.
Related to the connection reset comment from earlier:
read_line will return Ok(0) if the UnixStream is closed. https://docs.rs/tokio/1.50.0/tokio/net/struct.UnixStream.html
So we should check the length of bytes read from read_line and if it is empty raise an error so that we can reset the connection.
Currently it will raise an error during deserialization at from_str but we don't necessarily want to reset the connection on those errors.
There was a problem hiding this comment.
Again, thanks for pointing it out! This case is also included in the error handling logic - upone graceful shutdown will reset the connection to None - tested it by shuting down and then restarting reth, which now works.
b444024 to
8e69030
Compare
@macladson : done, let me know if it looks OK? |
macladson
left a comment
macladson
left a comment
There was a problem hiding this comment.
Hi @simonmichal looks like the Cargo.lock file is stale, it should regenerate on its own if you run something like make lint. If you can get CI passing I'll give it another review
@macladson Thanks for your reply and guidance - done! |
macladson
removed
the
waiting-on-author
rebased on unstable |
|
@macladson : just a friendly ping. |
macladson
added
ready-for-review
|
This pull request has merge conflicts. Could you please resolve them @simonmichal? 🙏 |
mergify
Bot
added
waiting-on-author
@macladson , @michaelsproul please let me know if you have any change requests! |
|
Hi @simonmichal, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time. |
mergify
Bot
added
the
stale
macladson
removed
the
stale
|
@macladson : I've just rebased on |
|
Hi @simonmichal, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time. |
mergify
Bot
added
the
stale
macladson
removed
the
stale
ackintosh
left a comment
ackintosh
left a comment
There was a problem hiding this comment.
Thanks for working this! I left a couple of suggestions around the JWT handling for the IPC path.
| // Use the default jwt secret path if not provided via cli. | ||
| let secret_file = secret_file.unwrap_or_else(|| default_datadir.join(DEFAULT_JWT_FILE)); | ||
|
|
||
| let jwt_key = if secret_file.exists() { | ||
| // Read secret from file if it already exists | ||
| std::fs::read_to_string(&secret_file) | ||
| .map_err(|e| format!("Failed to read JWT secret file. Error: {:?}", e)) | ||
| .and_then(|ref s| { | ||
| let secret = JwtKey::from_slice( | ||
| &hex::decode(strip_prefix(s.trim_end())) | ||
| .map_err(|e| format!("Invalid hex string: {:?}", e))?, | ||
| )?; | ||
| Ok(secret) | ||
| }) | ||
| .map_err(Error::InvalidJWTSecret) | ||
| } else { | ||
| // Create a new file and write a randomly generated secret to it if file does not exist | ||
| warn!(path = %secret_file.display(),"No JWT found on disk. Generating"); | ||
| std::fs::File::options() | ||
| .write(true) | ||
| .create_new(true) | ||
| .open(&secret_file) | ||
| .map_err(|e| format!("Failed to open JWT secret file. Error: {:?}", e)) | ||
| .and_then(|mut f| { | ||
| let secret = auth::JwtKey::random(); | ||
| f.write_all(secret.hex_string().as_bytes()) | ||
| .map_err(|e| format!("Failed to write to JWT secret file: {:?}", e))?; | ||
| Ok(secret) | ||
| }) | ||
| .map_err(Error::InvalidJWTSecret) | ||
| }?; |
There was a problem hiding this comment.
These lines read (and create if missing) the JWT secret, but that's only needed for the http/https transport — IPC doesn't use JWT auth. The secret_file.unwrap_or_else(...) fallback also defeats the None we set for IPC in config.rs, so a JWT file still gets generated on disk even when using IPC.
I'd recommend moving this whole block into the http | https arm of the match.
| @@ -329,6 +333,9 @@ pub fn get_config<E: EthSpec>( | |||
| e | |||
| ) | |||
| })?; | |||
| secret_file = Some(secret_file_buf); | |||
| } else if execution_endpoint.expose_full().scheme() == "ipc" { | |||
| secret_file = None; | |||
| } else { | |||
| return Err("Error! Please set either --execution-jwt file_path or --execution-jwt-secret-key directly via cli when using --execution-endpoint".to_string()); | |||
| } | |||
There was a problem hiding this comment.
Given that a user could supply an IPC endpoint via --execution-endpoint together with --execution-jwt / --execution-jwt-secret-key, it would be better to check the endpoint scheme first, to avoid the inconsistent state where the endpoint is IPC but secret_file is not None. Since IPC doesn't use JWT auth, we can also warn! when a JWT flag was supplied but has no effect:
if execution_endpoint.expose_full().scheme() == "ipc" {
secret_file = None;
// IPC transport doesn't use JWT authentication.
if cli_args.get_one::<String>("execution-jwt").is_some()
|| cli_args.get_one::<String>("execution-jwt-secret-key").is_some()
{
warn!("--execution-jwt/--execution-jwt-secret-key is ignored when using an IPC execution endpoint");
}
} else if let Some(secret_files) = cli_args.get_one::<String>("execution-jwt") {
// set `secret_file` from --execution-jwt
secret_file = Some(parse_only_one_value(...
} else if let Some(jwt_secret_key) = cli_args.get_one::<String>("execution-jwt-secret-key") {
// generate the file from --execution-jwt-secret-key
// ...
} else {
return Err("Error! Please set either --execution-jwt ...".to_string());
}4 participants
Issue Addressed
This PR aims to address #7336
Proposed Changes
This PR adds IPC transport.
Additional Info
I run following test to valided it: