Address code review: add overflow protection, proper error handling, and use MAX_HALVINGS constant

Co-authored-by: Steake <530040+Steake@users.noreply.github.com>

Author: Copilot

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;
@@ -112,8 +112,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
@@ -255,8 +255,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 {
@@ -269,10 +276,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/rpc.rs

@@ -20,6 +20,7 @@ pub struct RpcState {
     pub tournament_manager: Option<Arc<TournamentManager>>,
     pub config: NodeConfig,
     pub node_type: String, // "validator", "miner", "full"
+    pub node_id: String,   // Hex-encoded public key
 }
 
 /// Start the RPC server
@@ -472,7 +473,7 @@ async fn eth_send_raw_transaction(state: &RpcState, params: Option<Value>) -> Re
 
 async fn bitcell_get_node_info(state: &RpcState) -> Result<Value, JsonRpcError> {
     Ok(json!({
-        "node_id": "TODO_NODE_ID", // TODO: Expose node ID from NetworkManager
+        "node_id": state.node_id,
         "version": "0.1.0",
         "protocol_version": "1",
         "network_id": "bitcell-testnet",

crates/bitcell-state/src/lib.rs

@@ -33,6 +33,9 @@ pub enum Error {
 
     #[error("Storage error: {0}")]
     StorageError(String),
+    
+    #[error("Balance overflow")]
+    BalanceOverflow,
 }
 
 /// Global state manager
@@ -217,16 +220,25 @@ impl StateManager {
     }
 
     /// Credit an account (minting/coinbase)
-    pub fn credit_account(&mut self, pubkey: [u8; 33], amount: u64) -> Hash256 {
+    pub fn credit_account(&mut self, pubkey: [u8; 33], amount: u64) -> Result<Hash256> {
         let mut account = self.accounts.get(&pubkey)
             .cloned()
             .unwrap_or(Account { balance: 0, nonce: 0 });
 
-        account.balance += amount;
+        account.balance = account.balance.checked_add(amount)
+            .ok_or(Error::BalanceOverflow)?;
+        
+        tracing::debug!(
+            pubkey = %hex::encode(&pubkey),
+            amount = amount,
+            new_balance = account.balance,
+            "Credited account"
+        );
+        
         self.accounts.insert(pubkey, account);
 
         self.recompute_root();
-        self.state_root
+        Ok(self.state_root)
     }
 }
 

crates/bitcell-zkp/src/battle_circuit.rs

@@ -87,7 +87,10 @@ use ark_snark::SNARK;
 use ark_std::rand::thread_rng;
 
 impl BattleCircuit {
-    pub fn setup() -> (ProvingKey<ark_bn254::Bn254>, VerifyingKey<ark_bn254::Bn254>) {
+    /// Setup the circuit and generate proving/verifying keys
+    ///
+    /// Returns an error if the circuit setup fails (e.g., due to constraint system issues).
+    pub fn setup() -> crate::Result<(ProvingKey<ark_bn254::Bn254>, VerifyingKey<ark_bn254::Bn254>)> {
         let rng = &mut thread_rng();
         Groth16::<ark_bn254::Bn254>::circuit_specific_setup(
             Self {
@@ -99,7 +102,7 @@ impl BattleCircuit {
             },
             rng,
         )
-        .unwrap()
+        .map_err(|e| crate::Error::ProofGeneration(format!("Circuit setup failed: {}", e)))
     }
 
     pub fn prove(
@@ -129,10 +132,10 @@ mod tests {
 
     #[test]
     fn test_battle_circuit_prove_verify() {
-        // 1. Setup
-        let (pk, vk) = BattleCircuit::setup();
+        // 1. Setup - now returns Result
+        let (pk, vk) = BattleCircuit::setup().expect("Circuit setup should succeed");
 
-        // 2. Create circuit instance
+        // 2. Create circuit instance with valid winner ID (1 = Player B wins)
         let circuit = BattleCircuit::new(
             Fr::one(), // Mock commitment A
             Fr::one(), // Mock commitment B

crates/bitcell-zkp/src/state_circuit.rs

@@ -36,7 +36,10 @@ impl StateCircuit {
         }
     }
 
-    pub fn setup() -> (ProvingKey<ark_bn254::Bn254>, VerifyingKey<ark_bn254::Bn254>) {
+    /// Setup the circuit and generate proving/verifying keys
+    ///
+    /// Returns an error if the circuit setup fails (e.g., due to constraint system issues).
+    pub fn setup() -> crate::Result<(ProvingKey<ark_bn254::Bn254>, VerifyingKey<ark_bn254::Bn254>)> {
         let rng = &mut thread_rng();
         Groth16::<ark_bn254::Bn254>::circuit_specific_setup(
             Self {
@@ -47,7 +50,7 @@ impl StateCircuit {
             },
             rng,
         )
-        .unwrap()
+        .map_err(|e| crate::Error::ProofGeneration(format!("Circuit setup failed: {}", e)))
     }
 
     pub fn prove(
@@ -109,8 +112,8 @@ mod tests {
 
     #[test]
     fn test_state_circuit_prove_verify() {
-        // 1. Setup
-        let (pk, vk) = StateCircuit::setup();
+        // 1. Setup - now returns Result
+        let (pk, vk) = StateCircuit::setup().expect("Circuit setup should succeed");
 
         // 2. Create circuit instance
         let circuit = StateCircuit::new(