From bd559a176a9996d961380f207831b1ba7043ec35 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Thu, 18 Jun 2026 16:39:53 +0000 Subject: [PATCH] feat(whitelist): restore holding scopes on whitelist updates --- docs/explanation/deletion-requests.md | 4 + docs/reference/configuration.md | 8 +- src/main.rs | 21 ++- src/metrics/mod.rs | 38 +++++ src/nostr/builder.rs | 121 +++++++++++++++ tests/nip09_blacklist_ops.rs | 209 ++++++++++++++++++++++++++ 6 files changed, 399 insertions(+), 2 deletions(-) diff --git a/docs/explanation/deletion-requests.md b/docs/explanation/deletion-requests.md index 8e3c268..724ee56 100644 --- a/docs/explanation/deletion-requests.md +++ b/docs/explanation/deletion-requests.md @@ -367,6 +367,10 @@ reconciled (for announcements that list this relay service but no longer match When `NGIT_BLACKLIST_AUTO_RESTORE=true`, startup also runs a blacklist restore pass between blacklist parity delete and whitelist parity delete. +Startup also runs a whitelist restore pass after whitelist parity delete. This +restores holding scopes tagged `holding-source=whitelist` when they now match +`repository_whitelist` (while still respecting blacklist precedence). + **Future Enhancement (Dynamic Updates):** - Watch for configuration file changes - Trigger deletion/restore immediately on blacklist addition/removal diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index bbb445a..828a75f 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -1006,17 +1006,23 @@ Blacklist does **not** affect NIP-11 metadata: **Behavior:** -- When `false` (default): startup runs delete-only parity reconciliation for blacklist/whitelist; unblacklisted repositories in holding are restored only by existing recovery triggers (for example re-announcement). +- When `false` (default): startup does not run blacklist-source auto-restore; unblacklisted repositories in holding are restored only by existing recovery triggers (for example re-announcement). - When `true`: startup also scans holding metadata with `holding-source=blacklist`, deduplicates owner+identifier scopes, and attempts restore for scopes that: - are still within `NGIT_HOLDING_RETENTION_SECS`, and - no longer match `NGIT_REPOSITORY_BLACKLIST`. - Scopes that are still blacklisted are skipped (not restored). +**Related startup behavior (always on when repository whitelist is enabled):** + +- Startup scans holding metadata with `holding-source=whitelist` and attempts restore for scopes that are within retention, now match `NGIT_REPOSITORY_WHITELIST`, and are not blacklisted. +- Scopes still not matching `NGIT_REPOSITORY_WHITELIST` are skipped. + **Startup ordering:** 1. Blacklist parity delete 2. Blacklist auto-restore (if enabled) 3. Whitelist parity delete +4. Whitelist restore (for scopes now matching `NGIT_REPOSITORY_WHITELIST`) This ordering keeps startup reconciliation deterministic when multiple policies interact. diff --git a/src/main.rs b/src/main.rs index 7ff3d52..090eaa1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -235,7 +235,9 @@ async fn run_relay(config: Config) -> Result<()> { // 2) blacklist auto-restore (optional, restore previously blacklisted // repos now unblacklisted and still within holding retention) // 3) whitelist parity delete (final acceptance reconciliation for - // service-listed repos, preventing restore/re-delete churn on later starts) + // service-listed repos) + // 4) whitelist restore (restore previously whitelist-deleted repos now + // matching repository_whitelist and still within holding retention) let blacklist_stats = relay_with_db .write_policy .run_startup_blacklist_parity_pass() @@ -284,6 +286,23 @@ async fn run_relay(config: Config) -> Result<()> { ); } + let whitelist_restore_stats = relay_with_db + .write_policy + .run_startup_whitelist_restore_pass() + .await; + if whitelist_restore_stats.scanned_scopes > 0 + || whitelist_restore_stats.attempted_restores > 0 + { + info!( + scanned_scopes = whitelist_restore_stats.scanned_scopes, + attempted = whitelist_restore_stats.attempted_restores, + succeeded = whitelist_restore_stats.successful_restores, + failed = whitelist_restore_stats.failed_restores, + skipped = whitelist_restore_stats.skipped_scopes, + "Startup whitelist restore pass completed" + ); + } + // Wire the GRASP-06 `/prs/` filesystem cleanup context into // purgatory so the standard expiry sweep can delete dangling // refs/nostr/ refs (and zero-ref bare repos) when a diff --git a/src/metrics/mod.rs b/src/metrics/mod.rs index 7c13ded..4b619fa 100644 --- a/src/metrics/mod.rs +++ b/src/metrics/mod.rs @@ -60,6 +60,20 @@ lazy_static! { .expect("register blacklist startup restore metric"); metric }; + static ref WHITELIST_STARTUP_RESTORE_TOTAL: CounterVec = { + let metric = CounterVec::new( + Opts::new( + "ngit_whitelist_startup_restore_total", + "Whitelist startup restore operations by result and reason", + ), + &["result", "reason"], + ) + .expect("build whitelist startup restore metric"); + REGISTRY + .register(Box::new(metric.clone())) + .expect("register whitelist startup restore metric"); + metric + }; static ref HOLDING_CLEANUP_RUNS_TOTAL: Counter = { let metric = Counter::with_opts(Opts::new( "ngit_holding_cleanup_runs_total", @@ -182,6 +196,30 @@ pub fn record_blacklist_startup_restore_skipped(reason: &str) { .inc(); } +pub fn record_whitelist_startup_restore_attempt() { + WHITELIST_STARTUP_RESTORE_TOTAL + .with_label_values(&["attempted", "none"]) + .inc(); +} + +pub fn record_whitelist_startup_restore_success() { + WHITELIST_STARTUP_RESTORE_TOTAL + .with_label_values(&["succeeded", "none"]) + .inc(); +} + +pub fn record_whitelist_startup_restore_failure() { + WHITELIST_STARTUP_RESTORE_TOTAL + .with_label_values(&["failed", "none"]) + .inc(); +} + +pub fn record_whitelist_startup_restore_skipped(reason: &str) { + WHITELIST_STARTUP_RESTORE_TOTAL + .with_label_values(&["skipped", reason]) + .inc(); +} + pub fn record_holding_cleanup_run( metadata_deleted: usize, archived_events_deleted: usize, diff --git a/src/nostr/builder.rs b/src/nostr/builder.rs index e115815..96b63f8 100644 --- a/src/nostr/builder.rs +++ b/src/nostr/builder.rs @@ -76,6 +76,15 @@ pub struct BlacklistRestoreStats { pub skipped_scopes: usize, } +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct WhitelistRestoreStats { + pub scanned_scopes: usize, + pub attempted_restores: usize, + pub successful_restores: usize, + pub failed_restores: usize, + pub skipped_scopes: usize, +} + impl std::fmt::Debug for Nip34WritePolicy { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("Nip34WritePolicy") @@ -434,6 +443,118 @@ impl Nip34WritePolicy { stats } + /// Startup-only whitelist restore pass. + /// + /// Scans holding metadata for scopes deleted by whitelist parity and restores + /// owner+identifier scopes that now match `repository_whitelist`, are not + /// blacklisted, and are still inside holding retention. + pub async fn run_startup_whitelist_restore_pass(&self) -> WhitelistRestoreStats { + let whitelist = self.ctx.config.repository_config(); + if !whitelist.enabled() { + return WhitelistRestoreStats::default(); + } + + let now = Timestamp::now(); + let retention = self.ctx.config.holding_retention(); + let scopes = self + .ctx + .holding + .eligible_recovery_scopes_by_source(DeletionSource::Whitelist, now, retention) + .await; + + let blacklist = self.ctx.config.blacklist_config(); + let mut stats = WhitelistRestoreStats { + scanned_scopes: scopes.len(), + ..WhitelistRestoreStats::default() + }; + + for scope in scopes { + let owner_pubkey = match PublicKey::from_hex(&scope.owner_pubkey_hex) { + Ok(pubkey) => pubkey, + Err(e) => { + stats.skipped_scopes += 1; + crate::metrics::record_whitelist_startup_restore_skipped("invalid_owner"); + tracing::warn!( + owner = %scope.owner_pubkey_hex, + identifier = %scope.identifier, + reason = "invalid_owner", + error = %e, + "Whitelist startup restore: skipping scope" + ); + continue; + } + }; + + let owner_npub = match owner_pubkey.to_bech32() { + Ok(npub) => npub, + Err(e) => { + stats.skipped_scopes += 1; + crate::metrics::record_whitelist_startup_restore_skipped("owner_bech32_failed"); + tracing::warn!( + owner = %scope.owner_pubkey_hex, + identifier = %scope.identifier, + reason = "owner_bech32_failed", + error = %e, + "Whitelist startup restore: skipping scope" + ); + continue; + } + }; + + if let Some(reason) = blacklist.check(&owner_npub, &scope.identifier) { + stats.skipped_scopes += 1; + crate::metrics::record_whitelist_startup_restore_skipped("still_blacklisted"); + tracing::info!( + owner = %scope.owner_pubkey_hex, + identifier = %scope.identifier, + reason = %reason, + "Whitelist startup restore: skipping scope still blacklisted" + ); + continue; + } + + if !whitelist.matches(&owner_npub, &scope.identifier) { + stats.skipped_scopes += 1; + crate::metrics::record_whitelist_startup_restore_skipped("not_whitelisted"); + tracing::info!( + owner = %scope.owner_pubkey_hex, + identifier = %scope.identifier, + reason = "not_whitelisted", + "Whitelist startup restore: skipping scope" + ); + continue; + } + + stats.attempted_restores += 1; + crate::metrics::record_whitelist_startup_restore_attempt(); + + if self + .maybe_recover_deleted_repository_for_scope(&owner_pubkey, &scope.identifier) + .await + { + stats.successful_restores += 1; + crate::metrics::record_whitelist_startup_restore_success(); + tracing::info!( + owner = %scope.owner_pubkey_hex, + identifier = %scope.identifier, + reason = "now_whitelisted_within_retention", + "Whitelist startup restore: restored repository scope" + ); + } else { + stats.failed_restores += 1; + crate::metrics::record_whitelist_startup_restore_failure(); + tracing::error!( + owner = %scope.owner_pubkey_hex, + identifier = %scope.identifier, + reason = "recovery_pipeline_failed", + "Whitelist startup restore: recovery pipeline failed" + ); + } + } + + stats + } + /// Set the local relay for purgatory notifications. /// /// This must be called after the relay is created since the relay depends diff --git a/tests/nip09_blacklist_ops.rs b/tests/nip09_blacklist_ops.rs index 0024320..5005ff2 100644 --- a/tests/nip09_blacklist_ops.rs +++ b/tests/nip09_blacklist_ops.rs @@ -366,6 +366,214 @@ async fn startup_whitelist_scan_leaves_matching_repositories_untouched() { .is_empty()); } +#[tokio::test] +async fn startup_whitelist_restore_recovers_now_whitelisted_scope_and_is_idempotent() { + let relay_dir = tempfile::tempdir().expect("relay tempdir"); + let git_dir = tempfile::tempdir().expect("git tempdir"); + let db: SharedDatabase = Arc::new(nostr_memory::MemoryDatabase::unbounded()); + let holding = HoldingStore::open_lmdb(relay_dir.path(), git_dir.path()) + .await + .expect("open holding lmdb"); + + let owner = Keys::generate(); + let owner_npub = owner.public_key().to_bech32().expect("owner npub"); + let announcement = + make_announcement_with_domain(&owner, "whitelist-restore-repo", "test.example.com"); + let issue = make_issue(&owner, &announcement); + + db.save_event(&announcement) + .await + .expect("save announcement"); + db.save_event(&issue).await.expect("save issue"); + + let repo_path = git_dir + .path() + .join(owner_npub.clone()) + .join("whitelist-restore-repo.git"); + std::fs::create_dir_all(repo_path.join("refs")).expect("create bare repo dir"); + + let deletion_policy = make_policy( + Config { + repository_whitelist: "allowed-repo".to_string(), + ..base_config() + }, + db.clone(), + holding.clone(), + git_dir.path(), + ); + let deletion_stats = deletion_policy.run_startup_whitelist_parity_pass().await; + assert_eq!(deletion_stats.successful_deletions, 1); + + if repo_path.exists() { + std::fs::remove_dir_all(&repo_path).expect("remove stale repo directory"); + } + + let announcement_meta = holding.metadata_for_event(&announcement.id).await; + let archive_rel = announcement_meta + .iter() + .flat_map(|m| m.tags.iter()) + .find_map(|tag| { + let v = tag.as_slice(); + (v.len() >= 2 && v[0] == HOLDING_ARCHIVE_PATH_TAG).then(|| v[1].clone()) + }) + .expect("whitelist deletion should record archive path"); + let archive_abs = holding + .archive_absolute_path(&archive_rel) + .expect("resolve archive path"); + assert!(archive_abs.exists(), "archive must exist before restore"); + + let restore_policy = make_policy( + Config { + repository_whitelist: format!("allowed-repo,{}", owner_npub), + ..base_config() + }, + db.clone(), + holding.clone(), + git_dir.path(), + ); + + let before = render_metrics_snapshot(); + let restore_stats = restore_policy.run_startup_whitelist_restore_pass().await; + assert_eq!(restore_stats.scanned_scopes, 1); + assert_eq!(restore_stats.attempted_restores, 1); + assert_eq!(restore_stats.successful_restores, 1); + assert_eq!(restore_stats.failed_restores, 0); + assert_eq!(restore_stats.skipped_scopes, 0); + + assert!( + db.event_by_id(&announcement.id) + .await + .expect("query announcement") + .is_some(), + "announcement should be restored from holding" + ); + assert!( + db.event_by_id(&issue.id) + .await + .expect("query issue") + .is_some(), + "dependent events should be restored from holding" + ); + assert!( + holding + .metadata_for_event(&announcement.id) + .await + .is_empty(), + "holding metadata should be cleaned after restore" + ); + assert!(!archive_abs.exists(), "consumed archive should be deleted"); + + let after = render_metrics_snapshot(); + let attempted_before = metric_value( + &before, + "ngit_whitelist_startup_restore_total", + &[("result", "attempted"), ("reason", "none")], + ); + let attempted_after = metric_value( + &after, + "ngit_whitelist_startup_restore_total", + &[("result", "attempted"), ("reason", "none")], + ); + assert_eq!(attempted_after, attempted_before + 1.0); + + let success_before = metric_value( + &before, + "ngit_whitelist_startup_restore_total", + &[("result", "succeeded"), ("reason", "none")], + ); + let success_after = metric_value( + &after, + "ngit_whitelist_startup_restore_total", + &[("result", "succeeded"), ("reason", "none")], + ); + assert_eq!(success_after, success_before + 1.0); + + let second_stats = restore_policy.run_startup_whitelist_restore_pass().await; + assert_eq!(second_stats.scanned_scopes, 0); + assert_eq!(second_stats.attempted_restores, 0); + assert_eq!(second_stats.successful_restores, 0); + assert_eq!(second_stats.failed_restores, 0); + assert_eq!(second_stats.skipped_scopes, 0); +} + +#[tokio::test] +async fn startup_whitelist_restore_skips_scopes_still_not_whitelisted() { + let relay_dir = tempfile::tempdir().expect("relay tempdir"); + let git_dir = tempfile::tempdir().expect("git tempdir"); + let db: SharedDatabase = Arc::new(nostr_memory::MemoryDatabase::unbounded()); + let holding = HoldingStore::open_lmdb(relay_dir.path(), git_dir.path()) + .await + .expect("open holding lmdb"); + + let owner = Keys::generate(); + let owner_npub = owner.public_key().to_bech32().expect("owner npub"); + let announcement = + make_announcement_with_domain(&owner, "whitelist-still-blocked-repo", "test.example.com"); + db.save_event(&announcement) + .await + .expect("save announcement"); + + std::fs::create_dir_all( + git_dir + .path() + .join(owner_npub) + .join("whitelist-still-blocked-repo.git") + .join("refs"), + ) + .expect("create bare repo dir"); + + let deletion_policy = make_policy( + Config { + repository_whitelist: "allowed-repo".to_string(), + ..base_config() + }, + db.clone(), + holding.clone(), + git_dir.path(), + ); + let deletion_stats = deletion_policy.run_startup_whitelist_parity_pass().await; + assert_eq!(deletion_stats.successful_deletions, 1); + + let restore_policy = make_policy( + Config { + repository_whitelist: "allowed-repo".to_string(), + ..base_config() + }, + db.clone(), + holding, + git_dir.path(), + ); + + let before = render_metrics_snapshot(); + let restore_stats = restore_policy.run_startup_whitelist_restore_pass().await; + assert_eq!(restore_stats.scanned_scopes, 1); + assert_eq!(restore_stats.attempted_restores, 0); + assert_eq!(restore_stats.successful_restores, 0); + assert_eq!(restore_stats.failed_restores, 0); + assert_eq!(restore_stats.skipped_scopes, 1); + + assert!( + db.event_by_id(&announcement.id) + .await + .expect("query announcement") + .is_none(), + "still-non-whitelisted scope must not be restored" + ); + + let after = render_metrics_snapshot(); + let skipped_before = metric_value( + &before, + "ngit_whitelist_startup_restore_total", + &[("result", "skipped"), ("reason", "not_whitelisted")], + ); + let skipped_after = metric_value( + &after, + "ngit_whitelist_startup_restore_total", + &[("result", "skipped"), ("reason", "not_whitelisted")], + ); + assert_eq!(skipped_after, skipped_before + 1.0); +} + #[tokio::test] async fn startup_blacklist_restore_recovers_unblacklisted_scope_and_is_idempotent() { let relay_dir = tempfile::tempdir().expect("relay tempdir"); @@ -699,6 +907,7 @@ async fn metrics_increment_on_blacklist_cleanup_recovery_and_manual_ejection_pat let rendered = render_metrics_snapshot(); assert!(rendered.contains("ngit_blacklist_deletions_total")); assert!(rendered.contains("ngit_blacklist_startup_restore_total")); + assert!(rendered.contains("ngit_whitelist_startup_restore_total")); assert!(rendered.contains("ngit_holding_cleanup_runs_total")); assert!(rendered.contains("ngit_recovery_total")); assert!(rendered.contains("ngit_manual_ejections_total"));