-
Notifications
You must be signed in to change notification settings - Fork 979
fix: throttle chain compaction with a 1h wall-clock gap #3892
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: staging
Are you sure you want to change the base?
Changes from all commits
948d581
2b3ba79
4799a59
b1923c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,7 @@ | |
| //! Adapters connecting new block, new transaction, and accepted transaction | ||
| //! events to consumers of those events. | ||
|
|
||
| use crate::util::RwLock; | ||
| use crate::util::{Mutex, RwLock}; | ||
| use std::collections::HashMap; | ||
| use std::fs::File; | ||
| use std::net::SocketAddr; | ||
|
|
@@ -61,6 +61,43 @@ const WORKER_CHANNEL_BUFFER_SIZE: usize = 64; | |
| const HEADER_SEGMENT_REQUEST_WINDOW_SECS: i64 = 60; | ||
| const MAX_HEADER_SEGMENT_REQUESTS_PER_WINDOW: usize = 120; | ||
|
|
||
| /// Wall-clock + probabilistic gate for chain compaction. If the dice hit and | ||
| /// at least `min_interval` has passed since the last recorded trigger (`None` | ||
| /// means never, always allowed), runs `start` and returns its result; the | ||
| /// trigger time is only recorded if `start` reports success. | ||
| /// | ||
| /// The lock is held across check+start+stamp so concurrent callers cannot | ||
| /// double-trigger in the same window, and `now` is read under the lock so an | ||
| /// older caller cannot move the timestamp backwards. | ||
| fn try_trigger_compaction<F>( | ||
| gate: &Mutex<Option<Instant>>, | ||
| min_interval: std::time::Duration, | ||
| dice_hit: bool, | ||
| start: F, | ||
| ) -> bool | ||
| where | ||
| F: FnOnce() -> bool, | ||
| { | ||
| if !dice_hit { | ||
| return false; | ||
| } | ||
| let mut last = gate.lock(); | ||
| let now = Instant::now(); | ||
| let wall_clock_ok = match *last { | ||
| None => true, | ||
| Some(t) => now | ||
| .checked_duration_since(t) | ||
| .map(|d| d >= min_interval) | ||
| .unwrap_or(true), | ||
|
wiesche89 marked this conversation as resolved.
Contributor
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. t was recorded from an earlier Instant::now() under the same mutex, so this None case should not be reachable. Could we simplify this to now.duration_since(t) >= min_interval? |
||
| }; | ||
| if wall_clock_ok && start() { | ||
| *last = Some(now); | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } | ||
|
|
||
| /// Implementation of the NetAdapter for the . Gets notified when new | ||
| /// blocks and transactions are received and forwards to the chain and pool | ||
| /// implementations. | ||
|
|
@@ -76,6 +113,9 @@ where | |
| config: ServerConfig, | ||
| hooks: Vec<Box<dyn NetEvents + Send + Sync>>, | ||
| header_segment_requests: RwLock<HashMap<SocketAddr, (DateTime<Utc>, usize)>>, | ||
| /// Wall-clock time of last successful compaction *trigger* (not completion). | ||
| /// Used with `MIN_COMPACTION_INTERVAL_SECS` to avoid compact storms during sync. | ||
| last_compact_trigger: Mutex<Option<Instant>>, | ||
|
Contributor
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. This state is local to the adapter, the sync loop’s direct Chain::compact() call bypasses it, and a restart resets it. Could we move the interval check to the shared compaction boundary so all trigger paths use the same policy, and define the restart behavior here too? |
||
| tx: mpsc::SyncSender<NetAdapterWorkerMessage>, | ||
| } | ||
|
|
||
|
|
@@ -708,6 +748,7 @@ where | |
| config, | ||
| hooks, | ||
| header_segment_requests: RwLock::new(HashMap::new()), | ||
| last_compact_trigger: Mutex::new(None), | ||
| tx, | ||
| }; | ||
| adapter.spawn_net_adapter_worker(Arc::downgrade(&chain), rx); | ||
|
|
@@ -966,19 +1007,32 @@ where | |
| } | ||
|
|
||
| fn check_compact(&self) { | ||
| // Roll the dice to trigger compaction at 1/COMPACTION_CHECK chance per block, | ||
| // uses a different thread to avoid blocking the caller thread (likely a peer) | ||
| let mut rng = thread_rng(); | ||
| if 0 == rng.gen_range(0, global::COMPACTION_CHECK) { | ||
| // Wall-clock throttle + height-based dice. During fast sync blocks arrive much | ||
| // faster than mainnet's 1/min, so the dice alone is too aggressive. | ||
| let min_interval = std::time::Duration::from_secs(global::MIN_COMPACTION_INTERVAL_SECS); | ||
| let dice_hit = { | ||
| let mut rng = thread_rng(); | ||
| 0 == rng.gen_range(0, global::COMPACTION_CHECK) | ||
| }; | ||
|
|
||
| try_trigger_compaction(&self.last_compact_trigger, min_interval, dice_hit, || { | ||
| let chain = self.chain(); | ||
| let _ = thread::Builder::new() | ||
| let syncing = self.sync_state.is_syncing(); | ||
| match thread::Builder::new() | ||
| .name("compactor".to_string()) | ||
| .spawn(move || { | ||
| info!("check_compact: starting compaction (syncing={})", syncing); | ||
| if let Err(e) = chain.compact() { | ||
| error!("Could not compact chain: {:?}", e); | ||
| } | ||
| }); | ||
| } | ||
| }) { | ||
| Ok(_) => true, | ||
| Err(e) => { | ||
| error!("Could not spawn compactor thread: {:?}", e); | ||
| false | ||
| } | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| fn request_transaction(&self, h: Hash, peer_info: &PeerInfo) { | ||
|
|
@@ -1301,3 +1355,98 @@ impl pool::BlockChain for PoolToChainAdapter { | |
| .map_err(|_| pool::PoolError::ImmatureTransaction) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
|
Contributor
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. Could we reduce this to one focused interval test and one concurrency test? Six tests and xxx lines feel excessive for this small gate.
Contributor
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. Two tests is much cleaner. Could we also replace the 50k loop and zero-duration stand in with a small boundary setup using timestamps just inside and outside the interval? That would test the actual comparison more directly. |
||
| mod tests { | ||
| use super::*; | ||
| use std::cell::Cell; | ||
| use std::time::Duration; | ||
|
|
||
| /// Simulate fast sync through the real gate: many "blocks" with the dice | ||
| /// always hitting must trigger compaction exactly once per wall-clock window. | ||
| /// Also covers dice misses and a failed start not consuming the window. | ||
| #[test] | ||
| fn compaction_gate_triggers_at_most_once_per_wall_clock_window() { | ||
| let min = Duration::from_secs(global::MIN_COMPACTION_INTERVAL_SECS); | ||
| let gate: Mutex<Option<Instant>> = Mutex::new(None); | ||
| let triggers = Cell::new(0u32); | ||
| let start = || { | ||
| triggers.set(triggers.get() + 1); | ||
| true | ||
| }; | ||
|
|
||
| // 50k "blocks" in one wall-clock window (worst-case sync). | ||
| for _ in 0..50_000 { | ||
| try_trigger_compaction(&gate, min, true, start); | ||
| } | ||
| assert_eq!( | ||
| triggers.get(), | ||
| 1, | ||
| "expected exactly one compact trigger in a single wall-clock window" | ||
| ); | ||
|
|
||
| // Dice miss never triggers, even on a fresh gate. | ||
| assert!(!try_trigger_compaction( | ||
| &Mutex::new(None), | ||
| min, | ||
| false, | ||
| start | ||
| )); | ||
| assert_eq!(triggers.get(), 1); | ||
|
|
||
| // A failed start must not consume the window: the next successful | ||
| // start is still allowed. | ||
| let gate = Mutex::new(None); | ||
| assert!(!try_trigger_compaction(&gate, min, true, || false)); | ||
| assert!(try_trigger_compaction(&gate, min, true, start)); | ||
| assert_eq!(triggers.get(), 2); | ||
|
|
||
| // Once the interval has elapsed the gate opens again (zero interval | ||
| // stands in for elapsed time, since `Instant` cannot be advanced). | ||
| assert!(try_trigger_compaction( | ||
| &gate, | ||
| Duration::from_secs(0), | ||
| true, | ||
| start | ||
| )); | ||
| assert_eq!(triggers.get(), 3); | ||
| } | ||
|
|
||
| /// Concurrent callers racing on the shared gate must trigger at most once. | ||
| #[test] | ||
| fn concurrent_compaction_gate_triggers_at_most_once() { | ||
| use std::sync::atomic::{AtomicU32, Ordering}; | ||
| use std::sync::Barrier; | ||
|
|
||
| let min = Duration::from_secs(global::MIN_COMPACTION_INTERVAL_SECS); | ||
| let gate: Arc<Mutex<Option<Instant>>> = Arc::new(Mutex::new(None)); | ||
| let triggers = Arc::new(AtomicU32::new(0)); | ||
| let num_threads = 64; | ||
| let barrier = Arc::new(Barrier::new(num_threads)); | ||
|
|
||
| let handles: Vec<_> = (0..num_threads) | ||
| .map(|_| { | ||
| let gate = gate.clone(); | ||
| let triggers = triggers.clone(); | ||
| let barrier = barrier.clone(); | ||
| thread::spawn(move || { | ||
| barrier.wait(); | ||
| try_trigger_compaction(&gate, min, true, || { | ||
| triggers.fetch_add(1, Ordering::SeqCst); | ||
| true | ||
| }); | ||
| }) | ||
| }) | ||
| .collect(); | ||
|
|
||
| for h in handles { | ||
| h.join().unwrap(); | ||
| } | ||
|
|
||
| assert_eq!( | ||
| triggers.load(Ordering::SeqCst), | ||
| 1, | ||
| "concurrent callers sharing the gate must trigger compaction at most once per window" | ||
| ); | ||
| } | ||
| } | ||
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.
The same rationale is repeated around the constant, helper, field, production call, and tests. Could we keep the motivation once near the constant and trim the other comments to just the non obvious locking invariant?