-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add private key file configuration for sequencer signing #161
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,7 +32,7 @@ use scroll_engine::{EngineDriver, ForkchoiceState}; | |
use scroll_migration::traits::ScrollMigrator; | ||
use scroll_network::ScrollNetworkManager; | ||
use scroll_wire::{ScrollWireConfig, ScrollWireProtocolHandler}; | ||
use std::{sync::Arc, time::Duration}; | ||
use std::{fs, sync::Arc, time::Duration}; | ||
use tokio::sync::mpsc::Sender; | ||
|
||
/// Implementing the trait allows the type to return whether it is configured for dev chain. | ||
|
@@ -70,6 +70,11 @@ impl RollupManagerAddOn { | |
<<N as FullNodeTypes>::Types as NodeTypes>::ChainSpec: ScrollHardforks + IsDevChain, | ||
N::Network: NetworkProtocols + FullNetwork<Primitives = ScrollNetworkPrimitives>, | ||
{ | ||
// Validate configuration before starting any components | ||
self.config | ||
.validate() | ||
.map_err(|e| eyre::eyre!("Configuration validation failed: {}", e))?; | ||
|
||
// Instantiate the network manager | ||
let (scroll_wire_handler, events) = | ||
ScrollWireProtocolHandler::new(ScrollWireConfig::new(true)); | ||
|
@@ -211,7 +216,20 @@ impl RollupManagerAddOn { | |
.then_some(ctx.node.network().eth_wire_block_listener().await?); | ||
|
||
// Instantiate the signer | ||
let signer = self.config.test.then_some(Signer::spawn(PrivateKeySigner::random())); | ||
let signer = if self.config.test { | ||
Some(Signer::spawn(PrivateKeySigner::random())) | ||
} else if let Some(key_file_path) = &self.config.signer_args.key_file { | ||
let key_bytes = fs::read(key_file_path).map_err(|e| { | ||
eyre::eyre!("Failed to read signer key file {}: {}", key_file_path.display(), e) | ||
})?; | ||
Comment on lines
+222
to
+224
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of expecting the file to contain raw bytes should we expect the private key to be hex encoded? How is this managed in geth/reth? What do you think? |
||
|
||
let private_key_signer = PrivateKeySigner::from_slice(&key_bytes) | ||
.map_err(|e| eyre::eyre!("Failed to create signer from key file: {}", e))?; | ||
|
||
Some(Signer::spawn(private_key_signer)) | ||
} else { | ||
None | ||
}; | ||
|
||
// Spawn the rollup node manager | ||
let rnm = RollupNodeManager::new( | ||
|
@@ -230,3 +248,54 @@ impl RollupManagerAddOn { | |
Ok((rnm, l1_notification_tx)) | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use alloy_signer_local::PrivateKeySigner; | ||
use std::io::Write; | ||
use tempfile::NamedTempFile; | ||
|
||
#[test] | ||
fn test_signer_initialization_with_valid_key_file() { | ||
// Test valid key file | ||
let mut temp_file = NamedTempFile::new().unwrap(); | ||
let private_key = [1u8; 32]; | ||
temp_file.write_all(&private_key).unwrap(); | ||
temp_file.flush().unwrap(); | ||
|
||
let key_bytes = std::fs::read(temp_file.path()).unwrap(); | ||
let result = PrivateKeySigner::from_slice(&key_bytes); | ||
|
||
assert!(result.is_ok()); | ||
} | ||
|
||
#[test] | ||
fn test_signer_initialization_with_invalid_key_file() { | ||
// Test invalid key file | ||
let mut temp_file = NamedTempFile::new().unwrap(); | ||
let invalid_key = b"invalid key data"; | ||
temp_file.write_all(invalid_key).unwrap(); | ||
temp_file.flush().unwrap(); | ||
|
||
let key_bytes = std::fs::read(temp_file.path()).unwrap(); | ||
let result = PrivateKeySigner::from_slice(&key_bytes); | ||
|
||
assert!(result.is_err()); | ||
|
||
let error = result.unwrap_err(); | ||
assert!(error.to_string().contains("signature error")); | ||
} | ||
|
||
#[test] | ||
fn test_signer_initialization_with_nonexistent_file() { | ||
// Test nonexistent file | ||
let nonexistent_path = "/nonexistent/path/to/key"; | ||
let result = std::fs::read(nonexistent_path); | ||
|
||
assert!(result.is_err()); | ||
|
||
let error = result.unwrap_err(); | ||
assert_eq!(error.kind(), std::io::ErrorKind::NotFound); | ||
assert!(error.to_string().contains("No such file or directory (os error 2)")); | ||
} | ||
} | ||
Comment on lines
+252
to
+301
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should move these tests to the signer crate. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,22 @@ pub struct ScrollRollupNodeConfig { | |
/// The network arguments | ||
#[command(flatten)] | ||
pub network_args: NetworkArgs, | ||
/// The signer arguments | ||
#[command(flatten)] | ||
pub signer_args: SignerArgs, | ||
} | ||
|
||
impl ScrollRollupNodeConfig { | ||
/// Validate that signer key file is provided when sequencer is enabled | ||
pub fn validate(&self) -> Result<(), String> { | ||
if !self.test && | ||
self.sequencer_args.sequencer_enabled && | ||
self.signer_args.key_file.is_none() | ||
{ | ||
return Err("Signer key file is required when sequencer is enabled".to_string()); | ||
} | ||
Ok(()) | ||
} | ||
} | ||
|
||
/// The database arguments. | ||
|
@@ -122,3 +138,89 @@ pub struct SequencerArgs { | |
)] | ||
pub l1_message_inclusion_mode: L1MessageInclusionMode, | ||
} | ||
|
||
/// The arguments for the signer. | ||
#[derive(Debug, Default, Clone, clap::Args)] | ||
pub struct SignerArgs { | ||
/// Path to the file containing the signer's private key | ||
#[arg( | ||
long = "signer.key-file", | ||
value_name = "FILE_PATH", | ||
help = "Path to the signer's private key file (required when sequencer is enabled)" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's migrate to hex-encoded key files and note that we expect hex encoding in |
||
)] | ||
pub key_file: Option<PathBuf>, | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
use std::path::PathBuf; | ||
|
||
#[test] | ||
fn test_validate_sequencer_enabled_without_key_file_fails() { | ||
let config = ScrollRollupNodeConfig { | ||
test: false, | ||
sequencer_args: SequencerArgs { sequencer_enabled: true, ..Default::default() }, | ||
signer_args: SignerArgs { key_file: None }, | ||
database_args: DatabaseArgs::default(), | ||
engine_driver_args: EngineDriverArgs::default(), | ||
l1_provider_args: L1ProviderArgs::default(), | ||
beacon_provider_args: BeaconProviderArgs::default(), | ||
network_args: NetworkArgs::default(), | ||
}; | ||
|
||
let result = config.validate(); | ||
assert!(result.is_err()); | ||
assert!(result | ||
.unwrap_err() | ||
.contains("Signer key file is required when sequencer is enabled")); | ||
} | ||
|
||
#[test] | ||
fn test_validate_sequencer_enabled_with_key_file_succeeds() { | ||
let config = ScrollRollupNodeConfig { | ||
test: false, | ||
sequencer_args: SequencerArgs { sequencer_enabled: true, ..Default::default() }, | ||
signer_args: SignerArgs { key_file: Some(PathBuf::from("/path/to/key")) }, | ||
database_args: DatabaseArgs::default(), | ||
engine_driver_args: EngineDriverArgs::default(), | ||
l1_provider_args: L1ProviderArgs::default(), | ||
beacon_provider_args: BeaconProviderArgs::default(), | ||
network_args: NetworkArgs::default(), | ||
}; | ||
|
||
assert!(config.validate().is_ok()); | ||
} | ||
|
||
#[test] | ||
fn test_validate_test_mode_without_key_file_succeeds() { | ||
let config = ScrollRollupNodeConfig { | ||
test: true, | ||
sequencer_args: SequencerArgs { sequencer_enabled: true, ..Default::default() }, | ||
signer_args: SignerArgs { key_file: None }, | ||
database_args: DatabaseArgs::default(), | ||
engine_driver_args: EngineDriverArgs::default(), | ||
l1_provider_args: L1ProviderArgs::default(), | ||
beacon_provider_args: BeaconProviderArgs::default(), | ||
network_args: NetworkArgs::default(), | ||
}; | ||
|
||
assert!(config.validate().is_ok()); | ||
} | ||
|
||
#[test] | ||
fn test_validate_sequencer_disabled_without_key_file_succeeds() { | ||
let config = ScrollRollupNodeConfig { | ||
test: false, | ||
sequencer_args: SequencerArgs { sequencer_enabled: false, ..Default::default() }, | ||
signer_args: SignerArgs { key_file: None }, | ||
database_args: DatabaseArgs::default(), | ||
engine_driver_args: EngineDriverArgs::default(), | ||
l1_provider_args: L1ProviderArgs::default(), | ||
beacon_provider_args: BeaconProviderArgs::default(), | ||
network_args: NetworkArgs::default(), | ||
}; | ||
|
||
assert!(config.validate().is_ok()); | ||
} | ||
} |
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 we should still include the
-d
argument, which specifies that we should disable discovery of other nodes. What do you think?