fix: address PR review feedback - security, safety, and code quality improvements

crates/bitcell-admin/src/api/wallet.rs

@@ -87,49 +87,14 @@ async fn get_balance(
 
 /// Send transaction
 async fn send_transaction(
-    State(config_manager): State<Arc<ConfigManager>>,
-    Json(req): Json<SendTransactionRequest>,
+    State(_config_manager): State<Arc<ConfigManager>>,
+    Json(_req): Json<SendTransactionRequest>,
 ) -> impl IntoResponse {
-    // Get config
-    let config = match config_manager.get_config() {
-        Ok(c) => c,
-        Err(_) => return (StatusCode::INTERNAL_SERVER_ERROR, "Failed to get config").into_response(),
-    };
-
+    // Transaction sending is not yet implemented.
     // In a real implementation, we would:
     // 1. Create a transaction object
     // 2. Sign it with a key managed by admin console (or passed in)
     // 3. Encode it
     // 4. Send via eth_sendRawTransaction
-
-    // For now, we'll just mock the RPC call with a dummy raw tx
-    let rpc_url = format!("http://{}:{}/rpc", config.wallet.node_rpc_host, config.wallet.node_rpc_port);
-    let client = reqwest::Client::new();
-
-    let dummy_signed_tx = "0x1234..."; // Placeholder
-
-    let rpc_req = json!({
-        "jsonrpc": "2.0",
-        "method": "eth_sendRawTransaction",
-        "params": [dummy_signed_tx],
-        "id": 1
-    });
-
-    match client.post(&rpc_url).json(&rpc_req).send().await {
-        Ok(resp) => {
-            if let Ok(json) = resp.json::<Value>().await {
-                if let Some(result) = json.get("result").and_then(|v| v.as_str()) {
-                    return Json(SendTransactionResponse {
-                        tx_hash: result.to_string(),
-                        status: "pending".to_string(),
-                    }).into_response();
-                }
-            }
-        }
-        Err(e) => {
-            tracing::error!("Failed to call RPC: {}", e);
-        }
-    }
-
-    (StatusCode::INTERNAL_SERVER_ERROR, "Failed to send transaction").into_response()
+    (StatusCode::NOT_IMPLEMENTED, "Transaction sending is not yet implemented. Please use the wallet CLI or GUI to send transactions.").into_response()
 }

crates/bitcell-admin/src/lib.rs

@@ -114,7 +114,6 @@ impl AdminConsole {
 
             .route("/api/blocks", get(api::blocks::list_blocks))
             .route("/api/blocks/:height", get(api::blocks::get_block))
-            .route("/api/blocks/:height", get(api::blocks::get_block))
             .route("/api/blocks/:height/battles", get(api::blocks::get_block_battles))
 
             // Wallet API

crates/bitcell-economics/src/constants.rs

@@ -17,6 +17,10 @@ pub const HALVING_INTERVAL: u64 = 210_000;
 /// Sum of geometric series: 50 * 210000 * (1 + 1/2 + 1/4 + ... + 1/2^63)
 pub const MAX_SUPPLY: u64 = 21_000_000 * COIN;
 
+/// Maximum number of halvings before reward becomes 0
+/// After 64 halvings, the reward would be less than 1 satoshi
+pub const MAX_HALVINGS: u64 = 64;
+
 /// ===== REWARD DISTRIBUTION =====
 
 /// Percentage of block reward to tournament winner

crates/bitcell-node/src/blockchain.rs

@@ -3,7 +3,7 @@
 use crate::{Result, MetricsRegistry};
 use bitcell_consensus::{Block, BlockHeader, Transaction, BattleProof};
 use bitcell_crypto::{Hash256, PublicKey, SecretKey};
-use bitcell_economics::{COIN, INITIAL_BLOCK_REWARD, HALVING_INTERVAL};
+use bitcell_economics::{COIN, INITIAL_BLOCK_REWARD, HALVING_INTERVAL, MAX_HALVINGS};
 use bitcell_state::StateManager;
 use std::sync::{Arc, RwLock};
 use std::collections::HashMap;
@@ -98,8 +98,8 @@ impl Blockchain {
     /// Calculate block reward based on height (halves every HALVING_INTERVAL blocks)
     pub fn calculate_block_reward(height: u64) -> u64 {
         let halvings = height / HALVING_INTERVAL;
-        if halvings >= 64 {
-            // After 64 halvings, reward is effectively 0
+        if halvings >= MAX_HALVINGS {
+            // After MAX_HALVINGS halvings, reward is effectively 0
             return 0;
         }
         INITIAL_BLOCK_REWARD >> halvings
@@ -207,8 +207,15 @@ impl Blockchain {
             // Apply block reward to proposer
             let reward = Self::calculate_block_reward(block_height);
             if reward > 0 {
-                state.credit_account(*block.header.proposer.as_bytes(), reward);
-                println!("Block reward credited: {} units to proposer", reward);
+                match state.credit_account(*block.header.proposer.as_bytes(), reward) {
+                    Ok(_) => {
+                        tracing::info!("Block reward credited: {} units to proposer", reward);
+                    }
+                    Err(e) => {
+                        tracing::error!("Failed to credit block reward: {:?}", e);
+                        return Err(crate::Error::Node("Failed to credit block reward".to_string()));
+                    }
+                }
             }
 
             for tx in &block.transactions {
@@ -221,10 +228,10 @@ impl Blockchain {
                 ) {
                     Ok(new_state_root) => {
                         // State updated successfully
-                        println!("Transaction applied, new state root: {:?}", new_state_root);
+                        tracing::debug!("Transaction applied, new state root: {:?}", new_state_root);
                     }
                     Err(e) => {
-                        println!("Failed to apply transaction: {:?}", e);
+                        tracing::warn!("Failed to apply transaction: {:?}", e);
                         // In production, this should rollback the entire block
                         // For now, we just skip the transaction
                     }

crates/bitcell-node/src/keys.rs

@@ -5,6 +5,7 @@ use std::fs;
 use std::path::Path;
 use crate::{Result, Error};
 use base64::{Engine as _, engine::general_purpose::STANDARD as BASE64};
+use tracing;
 
 /// Load a secret key from a file
 /// Supports:
@@ -122,36 +123,37 @@ pub fn resolve_secret_key(
 ) -> Result<SecretKey> {
     // Priority 1: Direct hex private key
     if let Some(hex) = private_key_hex {
-        println!("🔑 Loading key from hex string");
+        tracing::debug!("Loading key from hex string");
         return load_secret_key_from_hex(hex);
     }
 
     // Priority 2: Key file
     if let Some(path) = key_file_path {
-        println!("🔑 Loading key from file: {}", path.display());
+        tracing::debug!("Loading key from file: {}", path.display());
         return load_secret_key_from_file(path);
     }
 
     // Priority 3: Mnemonic phrase
     if let Some(phrase) = mnemonic {
-        println!("🔑 Deriving key from mnemonic phrase");
+        tracing::debug!("Deriving key from mnemonic phrase");
         return derive_secret_key_from_mnemonic(phrase);
     }
 
     // Priority 4: Simple seed
     if let Some(seed) = key_seed {
-        println!("🔑 Deriving key from seed: {}", seed);
+        tracing::debug!("Deriving key from seed");
         return Ok(derive_secret_key_from_seed(seed));
     }
 
     // Priority 5: Generate random
-    println!("🔑 Generating random key (no key specified)");
+    tracing::debug!("Generating random key (no key specified)");
     Ok(SecretKey::generate())
 }
 
 #[cfg(test)]
 mod tests {
     use super::*;
+    use std::io::Write;
 
     #[test]
     fn test_hex_key_loading() {
@@ -165,6 +167,19 @@ mod tests {
         let hex = "a".repeat(32);
         let result = load_secret_key_from_hex(&hex);
         assert!(result.is_err());
+
+        // Test with 65 characters (too long)
+        let hex_long = "a".repeat(65);
+        let result_long = load_secret_key_from_hex(&hex_long);
+        assert!(result_long.is_err());
+    }
+
+    #[test]
+    fn test_invalid_hex_characters() {
+        // Contains 'g' which is not a valid hex character
+        let hex = "g".repeat(64);
+        let result = load_secret_key_from_hex(&hex);
+        assert!(result.is_err());
     }
 
     #[test]
@@ -173,4 +188,122 @@ mod tests {
         let sk2 = derive_secret_key_from_seed("test-seed");
         assert_eq!(sk1.public_key(), sk2.public_key());
     }
+
+    #[test]
+    fn test_different_seeds_produce_different_keys() {
+        let sk1 = derive_secret_key_from_seed("seed-one");
+        let sk2 = derive_secret_key_from_seed("seed-two");
+        assert_ne!(sk1.public_key(), sk2.public_key());
+    }
+
+    #[test]
+    fn test_mnemonic_derivation() {
+        let mnemonic = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about";
+        let result = derive_secret_key_from_mnemonic(mnemonic);
+        assert!(result.is_ok());
+
+        // Same mnemonic produces same key
+        let result2 = derive_secret_key_from_mnemonic(mnemonic);
+        assert!(result2.is_ok());
+        assert_eq!(result.unwrap().public_key(), result2.unwrap().public_key());
+    }
+
+    #[test]
+    fn test_different_mnemonics_produce_different_keys() {
+        let mnemonic1 = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about";
+        let mnemonic2 = "zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo zoo wrong";
+
+        let sk1 = derive_secret_key_from_mnemonic(mnemonic1).unwrap();
+        let sk2 = derive_secret_key_from_mnemonic(mnemonic2).unwrap();
+        assert_ne!(sk1.public_key(), sk2.public_key());
+    }
+
+    #[test]
+    fn test_load_from_hex_file() {
+        let hex_key = "a".repeat(64);
+        let temp_dir = std::env::temp_dir();
+        let temp_file = temp_dir.join("test_key_hex.txt");
+
+        let mut file = std::fs::File::create(&temp_file).unwrap();
+        file.write_all(hex_key.as_bytes()).unwrap();
+
+        let result = load_secret_key_from_file(&temp_file);
+        assert!(result.is_ok());
+
+        std::fs::remove_file(temp_file).ok();
+    }
+
+    #[test]
+    fn test_load_from_pem_file() {
+        // Create a simple PEM file with valid key data
+        let pem_content = "-----BEGIN PRIVATE KEY-----
+MC4CAQAwBQYDK2VwBCIEIHJlYWxseWxvbmdzZWNyZXRrZXlieXRlczMyY2hhcnM=
+-----END PRIVATE KEY-----";
+        let temp_dir = std::env::temp_dir();
+        let temp_file = temp_dir.join("test_key.pem");
+
+        let mut file = std::fs::File::create(&temp_file).unwrap();
+        file.write_all(pem_content.as_bytes()).unwrap();
+
+        let result = load_secret_key_from_file(&temp_file);
+        // PEM parsing might fail due to key validation, but it should parse the format
+        // The important thing is it doesn't crash
+
+        std::fs::remove_file(temp_file).ok();
+    }
+
+    #[test]
+    fn test_load_from_nonexistent_file() {
+        let result = load_secret_key_from_file(Path::new("/nonexistent/path/key.txt"));
+        assert!(result.is_err());
+    }
+
+    #[test]
+    fn test_load_from_invalid_format_file() {
+        let temp_dir = std::env::temp_dir();
+        let temp_file = temp_dir.join("test_key_invalid.txt");
+
+        let mut file = std::fs::File::create(&temp_file).unwrap();
+        file.write_all(b"this is not a valid key format").unwrap();
+
+        let result = load_secret_key_from_file(&temp_file);
+        assert!(result.is_err());
+
+        std::fs::remove_file(temp_file).ok();
+    }
+
+    #[test]
+    fn test_resolve_secret_key_priority() {
+        // When private_key_hex is provided, it takes priority
+        let hex = "a".repeat(64);
+        let result = resolve_secret_key(Some(&hex), None, None, None);
+        assert!(result.is_ok());
+        let key_from_hex = result.unwrap();
+
+        // Same hex should produce same key
+        let result2 = resolve_secret_key(Some(&hex), None, Some("some mnemonic"), Some("some seed"));
+        assert!(result2.is_ok());
+        assert_eq!(key_from_hex.public_key(), result2.unwrap().public_key());
+    }
+
+    #[test]
+    fn test_resolve_secret_key_falls_back_to_seed() {
+        let result = resolve_secret_key(None, None, None, Some("test-seed"));
+        assert!(result.is_ok());
+
+        let expected = derive_secret_key_from_seed("test-seed");
+        assert_eq!(result.unwrap().public_key(), expected.public_key());
+    }
+
+    #[test]
+    fn test_resolve_secret_key_generates_random() {
+        let result1 = resolve_secret_key(None, None, None, None);
+        let result2 = resolve_secret_key(None, None, None, None);
+
+        assert!(result1.is_ok());
+        assert!(result2.is_ok());
+
+        // Random keys should be different
+        assert_ne!(result1.unwrap().public_key(), result2.unwrap().public_key());
+    }
 }

crates/bitcell-node/src/main.rs

@@ -108,7 +108,7 @@ async fn main() {
                 }
             };
 
-            println!("Validator Public Key: {:?}", secret_key.public_key());
+            tracing::debug!("Validator Public Key: {:?}", secret_key.public_key());
 
             // Initialize node with explicit secret key
             // Note: We need to modify ValidatorNode::new to accept an optional secret key or handle this differently
@@ -184,7 +184,7 @@ async fn main() {
                 }
             };
 
-            println!("Miner Public Key: {:?}", secret_key.public_key());
+            tracing::debug!("Miner Public Key: {:?}", secret_key.public_key());
 
             let mut node = MinerNode::with_key(config, secret_key);
 
@@ -246,7 +246,7 @@ async fn main() {
                 }
             };
 
-            println!("Full Node Public Key: {:?}", secret_key.public_key());
+            tracing::debug!("Full Node Public Key: {:?}", secret_key.public_key());
 
             // Reuse ValidatorNode for now as FullNode logic is similar (just no voting)
             let mut node = ValidatorNode::with_key(config, secret_key);