diff --git a/server/Cargo.toml b/server/Cargo.toml index 5fffec6349..0db78e321a 100644 --- a/server/Cargo.toml +++ b/server/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "stratum-server" -version = "5.7.4" +version = "5.7.5" authors = ["Sean Kilgarriff sean@urkel.com"] rust-version = "1.67.1" edition = "2021" diff --git a/server/src/config.rs b/server/src/config.rs index 03ed5e9e23..382cd1727c 100644 --- a/server/src/config.rs +++ b/server/src/config.rs @@ -7,7 +7,6 @@ pub struct ConfigManager { config: Arc, } -//@todo just looking at this now, it seems highly highly inefficient if we are moving or cloning impl ConfigManager { pub(crate) fn new(config: Config) -> Self { Self { @@ -32,6 +31,10 @@ impl ConfigManager { &self.config.difficulty } + pub(crate) fn connection_config(&self) -> &ConnectionConfig { + &self.config.connection + } + pub(crate) fn ban_manager_enabled(&self) -> bool { self.config.bans.enabled } @@ -71,6 +74,9 @@ pub struct ConnectionConfig { pub(crate) max_connections: Option, /// Active Timeout is how long with no activity before we disconnect a miner. pub(crate) active_timeout: u64, + /// Initial Timeout is how long we wait for an initial message from a miner. Once they are + /// active, we switch to the active timeout setting. + pub(crate) inital_timeout: u64, //@todo maybe move this to a new struct called MinerConfig, but for now I think it's ok. /// Check Threshold is how many shares until we consider a ban on a miner pub(crate) check_threshold: u64, @@ -85,6 +91,7 @@ impl Default for ConnectionConfig { proxy_protocol: false, max_connections: None, active_timeout: 600, + inital_timeout: 15, check_threshold: 500, invalid_percent: 50.0, } diff --git a/server/src/connection.rs b/server/src/connection.rs index 5c3530d26e..d969f03e45 100644 --- a/server/src/connection.rs +++ b/server/src/connection.rs @@ -161,6 +161,8 @@ impl ConnectionReader { continue; } + //@todo I think we may want to log the buf here if it fails on trace - Right now we + //can't see what these connections are sending. let msg: Request = serde_json::from_str(&buf)?; return Ok(Some(Frame::V1(msg))); diff --git a/server/src/miner.rs b/server/src/miner.rs index bfac935e36..0e9a463a65 100644 --- a/server/src/miner.rs +++ b/server/src/miner.rs @@ -1,5 +1,8 @@ use crate::{ - types::{ConnectionID, Difficulties, Difficulty, DifficultySettings, VarDiffBuffer}, + types::{ + BanStats, ConnectionID, Difficulties, Difficulty, DifficultySettings, MinerStats, + VarDiffBuffer, VarDiffStats, + }, utils, ConfigManager, SessionID, }; use parking_lot::Mutex; @@ -25,10 +28,12 @@ pub(crate) struct Inner { pub(crate) name: Option, } +//@todo random reminder for myself here -> Would it be more efficient to wrap this entire struct in +//a Mutex? My guess is no... But let's jsut re-review. #[derive(Debug)] pub(crate) struct Shared { difficulties: Mutex, - needs_ban: Mutex, + ban_stats: Mutex, stats: Mutex, var_diff_stats: Mutex, difficulty_settings: Mutex, @@ -49,7 +54,10 @@ impl Miner { let shared = Shared { difficulties: Mutex::new(Difficulties::new_only_current(difficulty.default)), - needs_ban: Mutex::new(false), + ban_stats: Mutex::new(BanStats { + last_ban_check_share: 0, + needs_ban: false, + }), stats: Mutex::new(MinerStats { accepted: 0, stale: 0, @@ -82,34 +90,52 @@ impl Miner { } } - // pub(crate) fn id(&self) -> Uuid { - // self.inner.id - // } - pub(crate) fn ban(&self) { - *self.shared.needs_ban.lock() = true; + //@todo I now set needs ban in consider ban so that I don't have to drop the Mutex. In + //here, we just really want to either contact the session, or disconnect the miner + // let mut ban_stats = self.shared.ban_stats.lock(); + // ban_stats.needs_ban = true; //@todo I think we need to disconnect here as well. + //@todo ok as far as I can tell, a ban will *not* lead to a miner being disconnected from + //the pool right now. + // + //Couple of thoughts. + // + //1. Let's go with the most complex scenario. Single connection with multiple miners. We + // want to disconnect this miner ONLY - How we do that will be complex because we will + // want the other miners in this connection to still work. + // + //2. If it is a single miner and single connection, I think this becomes a bit easier. We + // probably need a check in the tcp loop where we see if there are still remaining miners + // on a connection? Or we just send this individual miner a ban signal (tbd) + } + + pub fn needs_ban(&self) -> bool { + self.shared.ban_stats.lock().needs_ban } pub fn consider_ban(&self) { let stats = self.shared.stats.lock(); + let mut ban_stats = self.shared.ban_stats.lock(); //@note this could possibly possibly possibly overflow - let's just think about that as we //move forward. + //@todo I think this needs to be moved to like how retarget it used. Last ban check etc. let total = stats.accepted + stats.stale + stats.rejected; - let config = &self.config_manager.current_config().connection; + let config = &self.config_manager.connection_config(); - if total >= config.check_threshold { + if total - ban_stats.last_ban_check_share >= config.check_threshold { let percent_bad: f64 = ((stats.stale + stats.rejected) as f64 / total as f64) * 100.0; + ban_stats.last_ban_check_share = total; + if percent_bad < config.invalid_percent { - //@todo do we want to reset though? - //Although if we don't, then this will trigger on every new share after 500. - //So we could switch it to modulo 500 == 0 - //@todo make this possible. Reset stats to 0. - // self.stats.lock().await = MinerStats::default(); + //Does not need a ban + //@todo not sure if this is a good idea. Basically what we are saying is if the + //miner doesn't get banned in time, they can redeem themselves. + ban_stats.needs_ban = false; } else { warn!( id = ?self.inner.connection_id, @@ -120,6 +146,8 @@ impl Miner { stats.stale + stats.rejected, total ); + ban_stats.needs_ban = true; + self.ban(); } } @@ -153,9 +181,7 @@ impl Miner { self.consider_ban(); - //@todo see below - //I don't think we want to retarget on invalid shares, but let's double check later. - // self.retarget().await; + self.retarget(); } pub fn rejected_share(&self) { @@ -168,21 +194,18 @@ impl Miner { self.consider_ban(); - //@todo see below - //I don't think we want to retarget on invalid shares, but let's double check later. - // self.retarget().await; + self.retarget(); } - //@todo note, this only can be sent over ExMessage when it's hit a certain threshold. - //@todo self.set_difficulty - //@todo self.set_next_difficulty - //@todo does this need to return a result? Ideally not, but if we send difficulty, then maybe. - //@todo see if we can solve a lot of these recasting issues. - //@todo wrap u64 with a custom difficulty type. fn retarget(&self) { //This is in milliseconds let now = utils::now(); - // let retarget_time = self.config_manager.difficulty_config().retarget_time() * 1000.0; + let difficulty_config = self.config_manager.difficulty_config(); + + //@todo why not just store this as u128... Let's do that now. + let retarget_time = difficulty_config.retarget_time as u128 * 1000; + let retarget_share_amount = difficulty_config.retarget_share_amount; + //@todo see above, should we just store this as f64 * 1000.0? let mut difficulties = self.shared.difficulties.lock(); let mut var_diff_stats = self.shared.var_diff_stats.lock(); @@ -193,53 +216,35 @@ impl Miner { var_diff_stats.vardiff_buf.append(since_last); var_diff_stats.last_timestamp = now; - //@todo review this code, see if we can make this easier. - if !(((stats.accepted - var_diff_stats.last_retarget_share) - >= self - .config_manager - .difficulty_config() - .retarget_share_amount) - || (now - var_diff_stats.last_retarget) - >= (self.config_manager.difficulty_config().retarget_time as u128 * 1000)) - { + //@todo add this as a function on miner stats please. + let total = stats.accepted + stats.rejected + stats.stale; + + //This is the amoutn of shares we've added since the last retarget + let share_difference = total - var_diff_stats.last_retarget_share; + let time_difference = now - var_diff_stats.last_retarget; + + if !((share_difference >= retarget_share_amount) || time_difference >= retarget_time) { return; } - // dbg!(stats.accepted); - // - // dbg!(var_diff_stats.last_retarget_share); - // dbg!( - // self.config_manager - // .difficulty_config() - // .retarget_share_amount - // ); - var_diff_stats.last_retarget = now; var_diff_stats.last_retarget_share = stats.accepted; - // let variance = self.options.target_time * (self.options.variance_percent as f64 / 100.0); - // let time_min = self.options.target_time as f64 * 0.40; - // let time_max = self.options.target_time as f64 * 1.40; - //This average is in milliseconds let avg = var_diff_stats.vardiff_buf.avg(); - // debug!(average = ?avg); - // dbg!(avg); - if avg <= 0.0 { return; } let mut new_diff; - //@todo figure out what else needs to come from config here, and comment out this function. - let target_time = self.config_manager.difficulty_config().target_time as f64 * 1000.0; + let target_time = difficulty_config.target_time as f64 * 1000.0; + //@todo these variances should probs come from config. if avg > target_time { //@todo this needs to just be target_time since we multiplied it above. - if (avg / (self.config_manager.difficulty_config().target_time as f64 * 1000.0)) <= 1.5 - { + if (avg / target_time) <= 1.5 { return; } new_diff = difficulties.current().as_u64() / 2; @@ -251,7 +256,7 @@ impl Miner { new_diff = new_diff.clamp( self.shared.difficulty_settings.lock().minimum.as_u64(), - self.config_manager.difficulty_config().maximum_difficulty, + difficulty_config.maximum_difficulty, ); if new_diff != difficulties.current().as_u64() { @@ -289,23 +294,6 @@ impl Miner { } } -//@todo either wrap miner in a folder, or move these both to types -#[derive(Debug, Clone)] -pub struct MinerStats { - accepted: u64, - stale: u64, - rejected: u64, - last_active: u128, -} - -#[derive(Debug)] -pub struct VarDiffStats { - last_timestamp: u128, - last_retarget_share: u64, - last_retarget: u128, - vardiff_buf: VarDiffBuffer, -} - #[cfg(test)] mod test { use std::thread::sleep; @@ -356,6 +344,39 @@ mod test { //@todo we need some actual result here lol } + #[test] + fn test_ban() { + let connection_id = ConnectionID::new(); + let worker_id = Uuid::new_v4(); + let session_id = SessionID::from(1); + + let config = Config::default(); + let config_manager = ConfigManager::new(config.clone()); + + let diff_settings = DifficultySettings { + default: Difficulty::from(config.difficulty.initial_difficulty), + minimum: Difficulty::from(config.difficulty.minimum_difficulty), + }; + let miner = Miner::new( + connection_id, + worker_id, + session_id, + None, + None, + config_manager, + diff_settings, + ); + + miner.valid_share(); + + //Note Check threshold for miner bans is 500. + for _ in 0..500 { + miner.stale_share(); + } + + assert!(miner.needs_ban()); + } + #[test] fn test_retarget() { let connection_id = ConnectionID::new(); @@ -382,7 +403,7 @@ mod test { // OK what do we need to test here.... // 1. We need to solve the issue with why difficulty is flucuating so much with the single - // miner that is on the pool right now from CLSK. + // miner that is on the pool right now. // // Scenario: // The current miner should be at roughly 120 TH/s diff --git a/server/src/session.rs b/server/src/session.rs index 66915e3b5f..52e249343e 100644 --- a/server/src/session.rs +++ b/server/src/session.rs @@ -7,6 +7,7 @@ use extended_primitives::Buffer; use parking_lot::{Mutex, RwLock}; use serde::Serialize; use std::{ + fmt::Display, net::SocketAddr, sync::Arc, time::{Duration, Instant, SystemTime}, @@ -62,6 +63,22 @@ pub enum SendInformation { Raw(Buffer), } +impl Display for SendInformation { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + SendInformation::Json(s) => { + write!(f, "{}", s) + } + SendInformation::Text(s) => { + write!(f, "{}", s) + } + SendInformation::Raw(b) => { + write!(f, "{}", b) + } + } + } +} + //@todo thought process -> Rather than have this boolean variables that slowly add up over time, we //should add a new type of "SessionType". This will allow us to also incorporate other types of //connections that are developed in the future or that are already here and enables a lot easier @@ -79,7 +96,6 @@ pub struct Session { difficulty_settings: Arc>, } -//@todo I want to get IP here and return it struct Inner { pub id: ConnectionID, pub session_id: SessionID, @@ -167,11 +183,10 @@ impl Session { return Ok(()); } - debug!("Sending message: {}", serde_json::to_string(&message)?); - - //@todo implement Display on SendInformation. let msg = SendInformation::Json(serde_json::to_string(&message)?); + debug!("Sending message: {}", msg); + //@todo it may make sense to keep the sender inside of session here - not sure why it's in //connection like the way it is. //@todo this feels inefficient, maybe we do send bytes here. diff --git a/server/src/tcp.rs b/server/src/tcp.rs index b4997450fd..3a7574616b 100644 --- a/server/src/tcp.rs +++ b/server/src/tcp.rs @@ -72,8 +72,9 @@ impl break can return a value, and so we may @@ -88,7 +89,7 @@ impl { match res { Err(e) => { - warn!("Session: {} errored with the following error: {}", session.id(), e); + warn!(ip = session.ip().to_string(), "Session: {} errored with the following error: {}", session.id(), e); break; }, Ok(frame) => frame, @@ -155,9 +156,6 @@ impl