From d970501c857bf37979c91a5c958eb75b107b7662 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timo=20K=C3=B6sters?= Date: Fri, 16 Jul 2021 09:33:49 +0200 Subject: [PATCH] state-res: Revert calculating the auth chain in ruma In a previous commit I moved the auth chain calculation code to ruma because I thought I could optimize it by only taking auth chains from conflicted events instead of all events. It turned out that was wrong and now I removed that algorithm again (the full auth chains are now passed in as an argument to state_res::resolve again). --- crates/ruma-state-res/CHANGELOG.md | 2 +- .../ruma-state-res/benches/state_res_bench.rs | 24 ++++++-- crates/ruma-state-res/src/lib.rs | 56 ++++--------------- .../ruma-state-res/tests/res_with_auth_ids.rs | 8 +++ crates/ruma-state-res/tests/state_res.rs | 8 +++ crates/ruma-state-res/tests/utils.rs | 28 +++++++--- 6 files changed, 70 insertions(+), 56 deletions(-) diff --git a/crates/ruma-state-res/CHANGELOG.md b/crates/ruma-state-res/CHANGELOG.md index 2d16eaa0..89727056 100644 --- a/crates/ruma-state-res/CHANGELOG.md +++ b/crates/ruma-state-res/CHANGELOG.md @@ -2,7 +2,7 @@ Breaking changes: -* state_res::resolve now doesn't take auth_events anymore and calculates it on its own instead +* state_res::resolve auth_events type has been slightly changed and renamed to auth_chain_sets # 0.2.0 diff --git a/crates/ruma-state-res/benches/state_res_bench.rs b/crates/ruma-state-res/benches/state_res_bench.rs index 22980ac5..224b4b2e 100644 --- a/crates/ruma-state-res/benches/state_res_bench.rs +++ b/crates/ruma-state-res/benches/state_res_bench.rs @@ -66,6 +66,14 @@ fn resolution_shallow_auth_chain(c: &mut Criterion) { &room_id(), &RoomVersionId::Version6, &state_sets, + state_sets + .iter() + .map(|map| { + store + .auth_event_ids(&room_id(), &map.values().cloned().collect::>()) + .unwrap() + }) + .collect(), |id| ev_map.get(id).map(Arc::clone), ) { Ok(state) => state, @@ -81,7 +89,7 @@ fn resolve_deeper_event_set(c: &mut Criterion) { let ban = BAN_STATE_SET(); inner.extend(ban); - let _store = TestStore(inner.clone()); + let store = TestStore(inner.clone()); let state_set_a = [ inner.get(&event_id("CREATE")).unwrap(), @@ -115,6 +123,14 @@ fn resolve_deeper_event_set(c: &mut Criterion) { &room_id(), &RoomVersionId::Version6, &state_sets, + state_sets + .iter() + .map(|map| { + store + .auth_event_ids(&room_id(), &map.values().cloned().collect::>()) + .unwrap() + }) + .collect(), |id| inner.get(id).map(Arc::clone), ) { Ok(state) => state, @@ -159,8 +175,8 @@ impl TestStore { } /// Returns a Vec of the related auth events to the given `event`. - pub fn auth_event_ids(&self, room_id: &RoomId, event_ids: &[EventId]) -> Result> { - let mut result = vec![]; + pub fn auth_event_ids(&self, room_id: &RoomId, event_ids: &[EventId]) -> Result> { + let mut result = BTreeSet::new(); let mut stack = event_ids.to_vec(); // DFS for auth event chain @@ -170,7 +186,7 @@ impl TestStore { continue; } - result.push(ev_id.clone()); + result.insert(ev_id.clone()); let event = self.get_event(room_id, &ev_id)?; diff --git a/crates/ruma-state-res/src/lib.rs b/crates/ruma-state-res/src/lib.rs index ce2358f2..3ccd95c2 100644 --- a/crates/ruma-state-res/src/lib.rs +++ b/crates/ruma-state-res/src/lib.rs @@ -46,6 +46,9 @@ impl StateResolution { /// * `state_sets` - The incoming state to resolve. Each `StateMap` represents a possible fork /// in the state of a room. /// + /// * `auth_chain_sets` - The full recursive set of `auth_events` for each event in the + /// `state_sets`. + /// /// * `fetch_event` - Any event not found in the `event_map` will defer to this closure to find /// the event. /// @@ -58,6 +61,7 @@ impl StateResolution { room_id: &RoomId, room_version: &RoomVersionId, state_sets: &[StateMap], + auth_chain_sets: Vec>, fetch_event: F, ) -> Result> where @@ -98,8 +102,7 @@ impl StateResolution { } // The set of auth events that are not common across server forks - let mut auth_diff = - StateResolution::get_auth_chain_diff(room_id, &conflicting_state_sets, &fetch_event)?; + let mut auth_diff = StateResolution::get_auth_chain_diff(room_id, auth_chain_sets)?; // Add the auth_diff to conflicting now we have a full set of conflicting events auth_diff.extend(conflicting.values().cloned().flatten().filter_map(|o| o)); @@ -219,52 +222,17 @@ impl StateResolution { } /// Returns a Vec of deduped EventIds that appear in some chains but not others. - pub fn get_auth_chain_diff( + pub fn get_auth_chain_diff( _room_id: &RoomId, - conflicting_state_sets: &[BTreeSet], - fetch_event: F, - ) -> Result> - where - E: Event, - F: Fn(&EventId) -> Option>, - { - let mut chains = vec![]; - - // Conflicted state sets are just some top level state events. Now we fetch the complete - // auth chain of those events - for ids in conflicting_state_sets { - // TODO state store `auth_event_ids` returns self in the event ids list - // when an event returns `auth_event_ids` self is not contained - let mut todo = ids.iter().map(|e| e.clone()).collect::>(); - let mut auth_chain_ids = ids.clone(); // we also return the events we started with - - while let Some(event_id) = todo.iter().next().cloned() { - if let Some(pdu) = fetch_event(&event_id) { - todo.extend( - pdu.auth_events() - .clone() - .into_iter() - .collect::>() - .difference(&auth_chain_ids) - .cloned(), - ); - auth_chain_ids.extend(pdu.auth_events().into_iter()); - } else { - warn!("Could not find pdu mentioned in auth events."); - } - - todo.remove(&event_id); - } - - chains.push(auth_chain_ids); - } - - if let Some(first) = chains.first().cloned() { - let common = chains + auth_chain_sets: Vec>, + ) -> Result> { + if let Some(first) = auth_chain_sets.first().cloned() { + let common = auth_chain_sets .iter() + .skip(1) .fold(first, |a, b| a.intersection(&b).cloned().collect::>()); - Ok(chains.into_iter().flatten().filter(|id| !common.contains(&id)).collect()) + Ok(auth_chain_sets.into_iter().flatten().filter(|id| !common.contains(&id)).collect()) } else { Ok(btreeset![]) } diff --git a/crates/ruma-state-res/tests/res_with_auth_ids.rs b/crates/ruma-state-res/tests/res_with_auth_ids.rs index b1500eac..10be20ea 100644 --- a/crates/ruma-state-res/tests/res_with_auth_ids.rs +++ b/crates/ruma-state-res/tests/res_with_auth_ids.rs @@ -69,6 +69,14 @@ fn ban_with_auth_chains2() { &room_id(), &RoomVersionId::Version6, &state_sets, + state_sets + .iter() + .map(|map| { + store + .auth_event_ids(&room_id(), &map.values().cloned().collect::>()) + .unwrap() + }) + .collect(), |id| ev_map.get(id).map(Arc::clone), ) { Ok(state) => state, diff --git a/crates/ruma-state-res/tests/state_res.rs b/crates/ruma-state-res/tests/state_res.rs index b7463b59..fdfb13dc 100644 --- a/crates/ruma-state-res/tests/state_res.rs +++ b/crates/ruma-state-res/tests/state_res.rs @@ -258,6 +258,14 @@ fn test_event_map_none() { &room_id(), &RoomVersionId::Version2, &state_sets, + state_sets + .iter() + .map(|map| { + store + .auth_event_ids(&room_id(), &map.values().cloned().collect::>()) + .unwrap() + }) + .collect(), |id| ev_map.get(id).map(Arc::clone), ) { Ok(state) => state, diff --git a/crates/ruma-state-res/tests/utils.rs b/crates/ruma-state-res/tests/utils.rs index a31f4a4d..400b2562 100644 --- a/crates/ruma-state-res/tests/utils.rs +++ b/crates/ruma-state-res/tests/utils.rs @@ -109,10 +109,20 @@ pub fn do_check( .collect::>() ); - let resolved = - StateResolution::resolve(&room_id(), &RoomVersionId::Version6, &state_sets, |id| { - event_map.get(id).map(Arc::clone) - }); + let resolved = StateResolution::resolve( + &room_id(), + &RoomVersionId::Version6, + &state_sets, + state_sets + .iter() + .map(|map| { + store + .auth_event_ids(&room_id(), &map.values().cloned().collect::>()) + .unwrap() + }) + .collect(), + |id| event_map.get(id).map(Arc::clone), + ); match resolved { Ok(state) => state, Err(e) => panic!("resolution for {} failed: {}", node, e), @@ -217,8 +227,12 @@ impl TestStore { } /// Returns a Vec of the related auth events to the given `event`. - pub fn auth_event_ids(&self, room_id: &RoomId, event_ids: &[EventId]) -> Result> { - let mut result = vec![]; + pub fn auth_event_ids( + &self, + room_id: &RoomId, + event_ids: &[EventId], + ) -> Result> { + let mut result = BTreeSet::new(); let mut stack = event_ids.to_vec(); // DFS for auth event chain @@ -228,7 +242,7 @@ impl TestStore { continue; } - result.push(ev_id.clone()); + result.insert(ev_id.clone()); let event = self.get_event(room_id, &ev_id)?;