Skip to content

fix: --p2p-secret-key silently reverts to default location if file is not found (#2498) #2508

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.

Sign up for GitHub

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

Closed
mightypenguin wants to merge 2 commits into from
Closed

fix: --p2p-secret-key silently reverts to default location if file is not found (#2498) #2508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8484329
fix: --p2p-secret-key silently reverts to default location if file is…
mightypenguin May 3, 2023
0218e18
fix: --p2p-secret-key silently reverts to default location if file is…
mightypenguin May 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin/reth/src/args/secret_key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ pub fn get_secret_key(secret_key_path: &Path) -> Result<SecretKey, SecretKeyErro
Ok(false) => {
if let Some(dir) = secret_key_path.parent() {
// Create parent directory
std::fs::create_dir_all(dir)?
std::fs::create_dir_all(dir)?;
}

let secret = rng_secret_key();
Expand Down
12 changes: 11 additions & 1 deletion bin/reth/src/node/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,9 +229,19 @@ impl Command {
}

info!(target: "reth::cli", "Connecting to P2P network");
let secret_key_path = self.p2p_secret_key.clone().unwrap_or_default();
let default_secret_key_path = data_dir.p2p_secret_path();

let mut secret_key_path = secret_key_path.as_path();
if secret_key_path.to_str() == Some("") {
secret_key_path = default_secret_key_path.as_path();
}
Comment on lines +235 to +238
Copy link
Copy Markdown
Collaborator

@mattsse mattsse May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the reasoning for this here?

Copy link
Copy Markdown
Author

@mightypenguin mightypenguin May 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code always returns the default secret_key_path and does NOT use the --p2p-secret-key parameter if set.
My changes use "p2p-secret-key" if specified, else, use default value.

Copy link
Copy Markdown
Collaborator

@mattsse mattsse May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, perhaps I misunderstood

so the actual bug here is that the user input is ignored?

silently reverts to default location if file is not found

Copy link
Copy Markdown
Collaborator

@mattsse mattsse May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how exactly does your change fix

Setup Reth so that the default location for the p2p secret key is not accessible (e.g. systemd ProtectHome=True).
Set --p2p-secret-key to a non existent file.
This results in the following very confusing failure:

Copy link
Copy Markdown
Author

@mightypenguin mightypenguin May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problems Address here:

  1. User input is ignored.
    This is a new problem introduced the the last couple days that I ran across and tried to fix here.
    Blame Commit:
    feat: use chain-specific data dirs (feat: use chain-specific data dirs #2495)
    ecc7ae9

  2. Original problem
    The --p2p-path-key set by user.
    If the file does not exist, then the default_path is silently used/created without erroring.

This PR Addresses 2 cases:

  1. If p2p-secret-key IS set, try to load it, if it doesn't exist, create it.
  2. If p2p-secret-key is NOT set, use default path, if it doesn't exist create it.


let default_peers_path = data_dir.known_peers_path();
let secret_key = get_secret_key(&default_secret_key_path)?;

info!(target: "reth::cli", path = secret_key_path.to_str(), "Loading p2p-secret-key");
let secret_key = get_secret_key(secret_key_path)?;

let network_config = self.load_network_config(
&config,
Arc::clone(&db),
Expand Down