Reshare Megolm session after the other party unwedges

[uhoreg]

Fixes element-hq/element-meta#2389

For each recipient device, we keep an "Olm wedging index" that indicates (approximately) how many times they tried to unwedge the Olm session. When we share a Megolm session, we store the Olm wedging index. This allows us to tell whether the Olm session was unwedged since we shared the Megolm session, so that we know if we should re-share it.

We detect an attempt to unwedge the Olm session by checking if the Olm message that we decrypted is from a brand new Olm session. This is not entirely accurate, since this could happen, for example, if Alice and Bob create Olm sessions at the same time. This may result in some Megolm sessions being re-shared unnecessarily, but should not make a huge difference.

[codecov]

Codecov Report

Attention: Patch coverage is 87.87879% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.18%. Comparing base (1c92633) to head (7273f28).

Files	Patch %	Lines
...trix-sdk-crypto/src/olm/group_sessions/outbound.rs	75.00%	3 Missing ⚠️
...k-crypto/src/session_manager/group_sessions/mod.rs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3604   +/-   ##
=======================================
  Coverage   84.17%   84.18%           
=======================================
  Files         255      255           
  Lines       26515    26530   +15     
=======================================
+ Hits        22319    22334   +15     
  Misses       4196     4196           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[poljar]

Quite the clever implementation, nice. I left some smaller nits we should address before we merge.

[poljar]

This could take some docs, a single u32 could be figured out relatively easily, two of them double the confusion. It probably makes sense to convert the enum variant from a tuple variant to one with named fields, i.e.:

Shared {
    megolm_index: u32,
    wedging_foo: u32,
}

[poljar]

This section is clustered together as well, please add some whitespace to make this easier to read.

[poljar]

This seems to be wrapped at a really small width, we wrap at 80 chars, cargo fmt will do this automatically for you if the lines are too long.

[poljar]

This seems to be also wrapped manually, though it doesn't look as comical as the other doc comment, wouldn't hurt to let cargo fmt format this as well.

[poljar]

Technically this can panic, what do you think about using our SequenceNumber type for this. Not that it's terribly important but since we have such a type, seems like we could just use it for correctness sake:

matrix-rust-sdk/crates/matrix-sdk-crypto/src/store/caches.rs

Lines 191 to 229 in 3be84a5

	/// A numeric type that can represent an infinite ordered sequence.
	///
	/// It uses wrapping arithmetic to make sure we never run out of numbers. (2**64
	/// should be enough for anyone, but it's easy enough just to make it wrap.)
	//
	/// Internally it uses a *signed* counter so that we can compare values via a
	/// subtraction. For example, suppose we've just overflowed from i64::MAX to
	/// i64::MIN. (i64::MAX.wrapping_sub(i64::MIN)) is -1, which tells us that
	/// i64::MAX comes before i64::MIN in the sequence.
	#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)]
	pub(crate) struct SequenceNumber(i64);
	impl Display for SequenceNumber {
	fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
	self.0.fmt(f)
	}
	}
	impl PartialOrd for SequenceNumber {
	fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
	Some(self.0.wrapping_sub(other.0).cmp(&0))
	}
	}
	impl Ord for SequenceNumber {
	fn cmp(&self, other: &Self) -> std::cmp::Ordering {
	self.0.wrapping_sub(other.0).cmp(&0)
	}
	}
	impl SequenceNumber {
	fn increment(&mut self) {
	self.0 = self.0.wrapping_add(1)
	}
	fn previous(&self) -> Self {
	Self(self.0.wrapping_sub(1))
	}
	}

You would need to derive Serialize and Deserialize for it with the transparent attribute, but that's about it.

[uhoreg]

I think I've addressed all the review comments

[poljar]

This still isn't documented.

[uhoreg]

Sorry, I misunderstood your original comment and thought that switching to named fields would avoid the need for documentation. I've added docs, and while I was there, added docs for the rest of the enum too.

[poljar]

Nice, thanks for adding the docs.