cas error on newer signed packages

[SHAcollision]

Hey @Nuhvi ! First of all, I am sorry if this is a very stupid question here. I am trying to understand the logic behind CasFailed errors. It seems we are throwing CasFailed errors when the new package to publish has an older timestamp that the existing package in cache as demonstrated by this test:

pkarr/pkarr/src/client/tests.rs

Lines 431 to 463 in 4269e68

	async fn conflict_301_cas(#[case] networks: Networks) {
	let testnet = mainline::Testnet::new(10).unwrap();
	let relay = Relay::run_test(&testnet).await.unwrap();
	let client = builder(&relay, &testnet, networks).build().unwrap();
	let keypair = Keypair::random();
	let signed_packet_builder =
	SignedPacket::builder().txt("foo".try_into().unwrap(), "bar".try_into().unwrap(), 30);
	let t1 = Timestamp::now();
	let t2 = Timestamp::now();
	client
	.publish(
	&signed_packet_builder
	.clone()
	.timestamp(t2)
	.sign(&keypair)
	.unwrap(),
	None,
	)
	.await
	.unwrap();
	assert!(matches!(
	client
	.publish(&signed_packet_builder.sign(&keypair).unwrap(), Some(t1))
	.await,
	Err(PublishError::Concurrency(ConcurrencyError::CasFailed))
	));
	}

I am not sure if the test above is meant to test that t1 is older than t2 or that they are different. The check condition that throws this error checks for inequality:

pkarr/pkarr/src/client.rs

Line 302 in 4269e68

if cached.timestamp() != cas {

If we swap t1 and t2 around -we are publishing a newer timestamp than existed- we are still throwing the CasFailed error. This is easy to observe as swapping t2 and t1 in the test above still passes (same CasFailed error).

So here is the stupid question: is the intention of the CasFailed error to avoid republishing records that are already in cache? Or should it be possible to re-publish an existing signed packet as long as the timestamp is newer than the one found in cache?

[Nuhvi]

I am trying to understand the logic behind CasFailed errors.
The logic behind CasFailed errors is exactly the same logic behind CaS in CPUs or the If-Unmodified-Since conditional header in HTTP.

If CasFailed behaves differently from If-Unmodified-Since then there is a bug, otherwise, it is working as intended.

I am not sure if the test above is meant to test that t1 is older than t2 or that they are different.
It is meant to simulate a situation where you are trying to publish a packet, unaware (see the cas field is positively set to what is not t2, that is all that matters), So the client is telling you hey... you built this new packet based on t1` packet, but the packet was "modified since" (see how that is similar to the HTTP conditional header?).

If you on the other hand tried to replace Some(t1) with None, you should publish fine, because you are recklessly ignoring checking previous packets before building a new one.

If we swap t1 and t2 around -we are publishing a newer timestamp than existed- we are still throwing the CasFailed error. This is easy to observe as swapping t2 and t1 in the test above still passes (same CasFailed error).

You are publishing a newer timestamp than existed regardless, the only thing that matters here is are you aware of the previous packet that was published or not (cas matches), and do you care at all or not (cas is Some or None).

is the intention of the CasFailed error to avoid republishing records that are already in cache?
No because you can do that by reading the packet from the cache (or create a new one) and just set the cas argument to None.

should it be possible to re-publish an existing signed packet as long as the timestamp is newer than the one found in cache?
Same answer, the timestamp of the published packet doesn't affect the CasFailed error, if anything it can only result in a NotMostRecent error, if you are publishing an old packet, while someone else published a more recent version already.

Long story short, if you don't want to deal with CasFailed errors, and don't care about losing information from previous packets, just make sure the argument is set to None, you shouldn't see that error then.

Note: the exception in practice is that if there is an "inflight" publish request going on, on the DHT or Relays, and you set the cas to None, you won't be able to override that inflight request, you have to set its timestamp in the cas argument, just to tell the client you are aware of it and you want to override it, otherwise you will get ConflictRisk error. We can debate whether that is more harmful than useful, but remember that unless someone else has your keys this shouldn't be a problem if you "read before write" (which is good practice in general), because they can't keep publishing fresh new packets, and if they do have your key, then you have a bigger problem.

[Nuhvi]

Here is an example of publishing new packet with cas is set to None and everything working fine https://github.com/pubky/pkarr/pull/127/files#diff-579775f5675fe1e3141871a72c320205074fdd88fae24749643851502ac2e395R592

[SHAcollision]

Understood, thank you! Issue can be closed.