diff --git a/CHANGELOG.md b/CHANGELOG.md index b88ec9b..0a352e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Honor same-second lower-ID replacements when de-listing served repositories, + capturing superseded history, and choosing rollback or post-deletion state. + History capture time no longer overrides the original events' ID tie-break. + - Apply the lower-event-ID tie-break to same-second purgatory replacements. Git push authorization now selects the preferred matching state regardless of arrival order, and a winning announcement that removes this service diff --git a/docs/explanation/repository-lifecycle.md b/docs/explanation/repository-lifecycle.md index bd1f56a..fbd5932 100644 --- a/docs/explanation/repository-lifecycle.md +++ b/docs/explanation/repository-lifecycle.md @@ -722,14 +722,22 @@ behavior. The relay archives superseded 30617/30618 versions on write and uses that history at deletion time to pick rollback candidates for active `e`-target -deletions (announcement + state). +deletions (announcement + state). Same-second replacements use the NIP-01 +lower-event-ID tie-break for history capture, served-repository de-listing, +active-version selection and post-deletion state alignment. Losing replacements +and exact replays do not trigger history capture or de-list deletion. + +History candidates are ordered by their original event timestamp descending, +then original event ID ascending. Capture time and metadata ID only break ties +between records for the same original event; processing order cannot override +replacement preference. Timestamp cutoffs and author checks still apply. This architecture is covered by two integration-test layers: -- `tests/replaceable_history.rs` verifies the durable history substrate: +- `tests/lifecycle/replaceable_history.rs` verifies the durable history substrate: superseded 30617/30618 payloads are captured, queryable by coordinate/cutoff, survive LMDB restart, and do not change normal serving semantics. -- `tests/nip09_state_cascade.rs` verifies deletion behavior that consumes that +- `tests/lifecycle/nip09_state_cascade.rs` verifies deletion behavior that consumes that substrate: active 30618 `e` deletion restores the previous state event and realigns refs, active 30617 `e` deletion restores the previous announcement when history exists, coordinate deletion cutoff semantics do not incorrectly diff --git a/src/git/authorization.rs b/src/git/authorization.rs index dc4ac29..f64d92b 100644 --- a/src/git/authorization.rs +++ b/src/git/authorization.rs @@ -36,7 +36,7 @@ use std::collections::{HashMap, HashSet}; use std::sync::Arc; use tracing::{debug, info, warn}; -use crate::nostr::events::{RepositoryAnnouncement, RepositoryState}; +use crate::nostr::events::{compare_replacement_events, RepositoryAnnouncement, RepositoryState}; use crate::nostr::SharedDatabase; use crate::purgatory::Purgatory; use nostr_sdk::prelude::{Kind, PublicKey}; @@ -688,11 +688,7 @@ pub async fn get_state_authorization_for_selected_repo( // NIP-01 prefers the lower event ID when timestamps tie. let latest_authorized = authorized_events .iter() - .max_by(|left, right| { - left.created_at - .cmp(&right.created_at) - .then_with(|| right.id.cmp(&left.id)) - }) + .max_by(|left, right| compare_replacement_events(left, right)) .unwrap(); // Safe because we checked the vec is not empty // Parse the event into RepositoryState diff --git a/src/nostr/events.rs b/src/nostr/events.rs index 2489ea1..8f0c5fa 100644 --- a/src/nostr/events.rs +++ b/src/nostr/events.rs @@ -9,6 +9,14 @@ use anyhow::{anyhow, Result}; use nostr_sdk::prelude::{Event, Kind, PublicKey, ToBech32}; +/// Compare replacement preference within an already-selected event scope. +/// Greater means preferred: later timestamp, then lower event ID (NIP-01). +pub(crate) fn compare_replacement_events(left: &Event, right: &Event) -> std::cmp::Ordering { + left.created_at + .cmp(&right.created_at) + .then_with(|| right.id.cmp(&left.id)) +} + /// Whether a NIP-34 indexed role record is syntactically valid and active. /// /// Values after the pubkey alternate between numeric start and end diff --git a/src/nostr/lifecycle/deletion/rollback.rs b/src/nostr/lifecycle/deletion/rollback.rs index 4df9405..a73160b 100644 --- a/src/nostr/lifecycle/deletion/rollback.rs +++ b/src/nostr/lifecycle/deletion/rollback.rs @@ -8,7 +8,7 @@ use crate::git::authorization::{ collect_state_maintainers_by_coordinate, fetch_repository_data_excluding_purgatory, }; use crate::git::process; -use crate::nostr::events::RepositoryState; +use crate::nostr::events::{compare_replacement_events, RepositoryState}; use crate::nostr::lifecycle::RecoveryScope; #[derive(Debug, Clone, PartialEq, Eq, Hash)] @@ -169,11 +169,7 @@ impl DeletionPolicy { return None; }; - events.into_iter().max_by(|a, b| { - a.created_at - .cmp(&b.created_at) - .then_with(|| a.id.cmp(&b.id)) - }) + events.into_iter().max_by(compare_replacement_events) } pub(super) async fn rollback_deleted_active_replaceable(&self, plan: &ReplaceableRollbackPlan) { @@ -423,16 +419,7 @@ impl DeletionPolicy { continue; } - let latest_state = repo_data - .states - .iter() - .filter(|state| maintainers.contains(&state.event.pubkey.to_hex())) - .max_by(|a, b| { - a.event - .created_at - .cmp(&b.event.created_at) - .then_with(|| a.event.id.cmp(&b.event.id)) - }); + let latest_state = latest_authorized_state(&repo_data.states, maintainers); match latest_state { Some(state) => { @@ -488,3 +475,58 @@ impl DeletionPolicy { } } } + +fn latest_authorized_state<'a>( + states: &'a [RepositoryState], + maintainers: &[String], +) -> Option<&'a RepositoryState> { + states + .iter() + .filter(|state| maintainers.contains(&state.event.pubkey.to_hex())) + .max_by(|a, b| compare_replacement_events(&a.event, &b.event)) +} + +#[cfg(test)] +mod tests { + use super::*; + use nostr_sdk::prelude::{EventBuilder, FinalizeEvent, Keys, Tag}; + + #[test] + fn replacement_order_selects_authorized_state_for_realignment() { + let keys = [Keys::generate(), Keys::generate()]; + let make_state = |keys: &Keys, time| { + RepositoryState::from_event( + EventBuilder::new(Kind::RepoState, "") + .tags([ + Tag::identifier("repo"), + Tag::custom( + "refs/heads/main", + ["1234567890abcdef1234567890abcdef12345678"], + ), + ]) + .custom_created_at(Timestamp::from_secs(time)) + .finalize(keys) + .unwrap(), + ) + .unwrap() + }; + let mut candidates = [make_state(&keys[0], 100), make_state(&keys[1], 100)]; + candidates.sort_by_key(|state| state.event.id); + let [winner, loser] = candidates; + let authorized = keys.map(|keys| keys.public_key().to_hex()); + let outsider = make_state(&Keys::generate(), 200); + for states in [ + vec![winner.clone(), loser.clone(), outsider.clone()], + vec![loser.clone(), winner.clone(), outsider.clone()], + ] { + assert_eq!( + latest_authorized_state(&states, &authorized) + .unwrap() + .event + .id, + winner.event.id + ); + } + assert!(latest_authorized_state(&[outsider], &authorized).is_none()); + } +} diff --git a/src/nostr/lifecycle/deletion/service.rs b/src/nostr/lifecycle/deletion/service.rs index e5d614d..beca4f3 100644 --- a/src/nostr/lifecycle/deletion/service.rs +++ b/src/nostr/lifecycle/deletion/service.rs @@ -3,7 +3,7 @@ use nostr_sdk::prelude::{ nip62, Event, Filter, Kind, PublicKey, RelayUrl, SingleLetterTag, Timestamp, WritePolicyResult, }; -use crate::nostr::events::RepositoryAnnouncement; +use crate::nostr::events::{compare_replacement_events, RepositoryAnnouncement}; use crate::nostr::lifecycle::{RequestClassification, RequestLifecycleRecord}; use crate::nostr::policy::{duplicate, reject_error, reject_invalid, AnnouncementResult}; @@ -527,7 +527,7 @@ impl DeletionService { ); let current = match self.ctx.database().query(filter).await { - Ok(events) => events.into_iter().max_by_key(|event| event.created_at), + Ok(events) => events.into_iter().max_by(compare_replacement_events), Err(e) => { tracing::warn!( owner = %incoming.pubkey.to_hex(), @@ -544,7 +544,7 @@ impl DeletionService { }; // Not newer than what we already serve -> no runtime parity action. - if incoming.created_at <= current.created_at { + if !compare_replacement_events(incoming, ¤t).is_gt() { return; } @@ -631,7 +631,7 @@ impl DeletionService { }; for superseded in existing { - if superseded.id == incoming.id || superseded.created_at >= incoming.created_at { + if !compare_replacement_events(incoming, &superseded).is_gt() { continue; } @@ -894,6 +894,97 @@ mod tests { ) } + fn replacement_candidates(kind: Kind) -> [(Event, &'static str); 2] { + let keys = Keys::generate(); + let mut candidates = ["first.example", "second.example"].map(|domain| { + let event = EventBuilder::new(kind, domain) + .tags([ + Tag::identifier("replacement-order"), + Tag::custom("clone", [format!("https://{domain}/repo.git")]), + Tag::custom("relays", [format!("wss://{domain}")]), + ]) + .custom_created_at(Timestamp::from_secs(100)) + .finalize(&keys) + .unwrap(); + (event, domain) + }); + candidates.sort_by_key(|(event, _)| event.id); + candidates + } + + #[tokio::test] + async fn replacement_order_controls_served_delisting() { + let [(winner, winner_domain), (loser, loser_domain)] = + replacement_candidates(Kind::GitRepoAnnouncement); + for (current, domain, incoming, deleted) in [ + (&loser, loser_domain, &winner, true), + (&winner, winner_domain, &loser, false), + (&winner, winner_domain, &winner, false), + ] { + let directory = tempfile::tempdir().unwrap(); + let mut ctx = context(crate::config::Config::for_testing()); + ctx.domain = domain.into(); + ctx.config.domain = domain.into(); + ctx.git_data_path = directory.path().to_path_buf(); + ctx.purgatory = Arc::new(Purgatory::new(directory.path().to_path_buf())); + ctx.database.save_event(current).await.unwrap(); + let service = DeletionService::new(ctx); + service.maybe_apply_runtime_delist_deletion(incoming).await; + assert_eq!( + service + .ctx + .database + .event_by_id(¤t.id) + .await + .unwrap() + .is_none(), + deleted + ); + } + } + + #[tokio::test] + async fn replacement_order_controls_same_second_history_capture() { + for kind in [Kind::GitRepoAnnouncement, Kind::RepoState] { + let [(winner, _), (loser, _)] = replacement_candidates(kind); + for (current, incoming, captures) in [ + (&loser, &winner, 1), + (&winner, &loser, 0), + (&winner, &winner, 0), + ] { + let service = DeletionService::new(context(crate::config::Config::for_testing())); + service.ctx.database.save_event(current).await.unwrap(); + service + .capture_superseded_replaceable_history(incoming) + .await; + let coordinate = format!( + "{}:{}:replacement-order", + kind.as_u16(), + current.pubkey.to_hex() + ); + let records = service + .history() + .superseded_records_for_coordinate_before(&coordinate, incoming.created_at) + .await; + assert_eq!(records.len(), captures); + if captures == 1 { + assert_eq!(records[0].superseded_event_id, Some(current.id)); + assert_eq!(records[0].replaced_by, Some(incoming.id)); + assert_eq!( + service + .history() + .event_by_id(¤t.id) + .await + .unwrap() + .unwrap() + .id, + current.id + ); + } + } + } + } + fn deletion(keys: &Keys, target: EventId) -> Event { EventBuilder::new(Kind::EventDeletion, "") .tags(vec![Tag::event(target)]) diff --git a/src/nostr/lifecycle/history.rs b/src/nostr/lifecycle/history.rs index 53b98c7..28a7226 100644 --- a/src/nostr/lifecycle/history.rs +++ b/src/nostr/lifecycle/history.rs @@ -184,8 +184,9 @@ impl ReplaceableHistoryStore { /// /// Ordering is deterministic: /// 1. `superseded_at` descending (missing treated as 0) - /// 2. `captured_at` descending (missing treated as 0) - /// 3. `metadata_event_id` descending + /// 2. Superseded event ID ascending (NIP-01); missing IDs last + /// 3. `captured_at` descending (missing treated as 0) + /// 4. `metadata_event_id` descending for duplicate capture records pub async fn latest_superseded_before( &self, coordinate: &str, @@ -199,6 +200,12 @@ impl ReplaceableHistoryStore { b.superseded_at .unwrap_or_else(|| Timestamp::from_secs(0)) .cmp(&a.superseded_at.unwrap_or_else(|| Timestamp::from_secs(0))) + .then_with(|| { + a.superseded_event_id + .is_none() + .cmp(&b.superseded_event_id.is_none()) + }) + .then_with(|| a.superseded_event_id.cmp(&b.superseded_event_id)) .then_with(|| { b.captured_at .unwrap_or_else(|| Timestamp::from_secs(0)) @@ -262,6 +269,41 @@ mod tests { .unwrap() } + #[tokio::test] + async fn replacement_order_precedes_capture_time_for_rollback_history() { + let store = ReplaceableHistoryStore::in_memory(); + let keys = Keys::generate(); + let mut candidates = [ + announcement(&keys, "repo", 100, "a"), + announcement(&keys, "repo", 100, "b"), + ]; + candidates.sort_by_key(|event| event.id); + let [winner, loser] = candidates; + let coordinate = format!("30617:{}:repo", keys.public_key().to_hex()); + // Capture order is deliberately opposite to replacement preference. + for (event, captured) in [(&winner, "100"), (&loser, "200")] { + let metadata = EventBuilder::new(Kind::from(HISTORY_METADATA_KIND), "") + .tags([ + Tag::event(event.id), + Tag::custom("a", [coordinate.clone()]), + Tag::custom(HISTORY_SUPERSEDED_AT_TAG, ["100"]), + Tag::custom(HISTORY_CAPTURED_AT_TAG, [captured]), + ]) + .finalize(&store.metadata_signer) + .unwrap(); + store.db.save_event(&metadata).await.unwrap(); + } + let selected = store + .latest_superseded_before(&coordinate, Timestamp::from_secs(100)) + .await + .unwrap(); + assert_eq!(selected.superseded_event_id, Some(winner.id)); + assert!(store + .latest_superseded_before(&coordinate, Timestamp::from_secs(99)) + .await + .is_none()); + } + #[tokio::test] async fn archives_and_queries_superseded_records_by_coordinate_and_time() { let store = ReplaceableHistoryStore::in_memory(); diff --git a/src/nostr/policy/announcement.rs b/src/nostr/policy/announcement.rs index 1bc6f1a..bb84da8 100644 --- a/src/nostr/policy/announcement.rs +++ b/src/nostr/policy/announcement.rs @@ -8,7 +8,9 @@ use std::time::Duration; use super::PolicyContext; use crate::config::Config; -use crate::nostr::events::{validate_announcement, RepositoryAnnouncement}; +use crate::nostr::events::{ + compare_replacement_events, validate_announcement, RepositoryAnnouncement, +}; use crate::private::PrivateAccess; /// Result of announcement policy evaluation @@ -91,9 +93,7 @@ impl AnnouncementPolicy { .purgatory .find_announcement(&event.pubkey, &announcement.identifier) .is_some_and(|entry| { - event.created_at > entry.event.created_at - || (event.created_at == entry.event.created_at - && event.id < entry.event.id) + compare_replacement_events(event, &entry.event).is_gt() }); if should_evict { @@ -345,11 +345,7 @@ impl AnnouncementPolicy { Err(e) => return Err(format!("Database query failed: {}", e)), }; - Ok(events.into_iter().max_by(|left, right| { - left.created_at - .cmp(&right.created_at) - .then_with(|| right.id.cmp(&left.id)) - })) + Ok(events.into_iter().max_by(compare_replacement_events)) } /// Add an announcement to purgatory diff --git a/tests/lifecycle/replaceable_history.rs b/tests/lifecycle/replaceable_history.rs index 68948bc..14cb666 100644 --- a/tests/lifecycle/replaceable_history.rs +++ b/tests/lifecycle/replaceable_history.rs @@ -298,6 +298,23 @@ async fn same_second_30618_replacement_keeps_nip01_lowest_id() { "the lower-ID state must remain the NIP-01 winner after restart" ); + let deletion = EventBuilder::new(Kind::EventDeletion, "delete same-second winner") + .tag(Tag::event(replacement.id)) + .custom_created_at(timestamp_after(replacement.created_at)) + .finalize(client.keys()) + .expect("sign deletion with original author"); + restarted_client + .send_event(deletion) + .await + .expect("delete active winner"); + common::wait_for_event_served(restarted.url(), &existing.id, Duration::from_secs(10)) + .await + .expect("restore same-second predecessor from durable history"); + assert!(!restarted_client + .is_event_on_relay(replacement.id) + .await + .unwrap()); + restarted.stop().await; }