Look up RK instead of looping

[arminsabouri]

Follow up from #995. We can replace a loop over all active session ids with a WHERE clause.

Pull Request Checklist

Please confirm the following before requesting review:

• I have disclosed my use of
  AI
  in the body of this PR.
• I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

This code was produced with cursor as a companion

[coveralls]

Pull Request Test Coverage Report for Build 17305051001

Details

• 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
• 5 unchanged lines in 1 file lost coverage.
• Overall coverage decreased (-0.06%) to 85.711%

---

Files with Coverage Reduction	New Missed Lines	%
payjoin-cli/src/db/v2.rs	5	95.27%

Totals
Change from base Build 17301530031:	-0.06%
Covered Lines:	8002
Relevant Lines:	9336

---

💛 - Coveralls

[nothingmuch]

i think the bip21 URI data, or at least the payjoin specific parts of it, should be fully checked for consistency before resuming

[nothingmuch]

nit: i'm not convinced this is really easier to follow with the transpose, but None => None always kinda seems unusual to me

suggestion is untested and definitely needs formatting it's probably multipline

Suggested change

	let session_id = self.db.get_send_session_id_with_receiver_pk(receiver_pubkey)?;
	let sender_state = match session_id {
	Some(session_id) => {
	let sender_persister =
	SenderPersister::from_id(self.db.clone(), session_id)?;
	let (send_session, _) = replay_sender_event_log(&sender_persister)?;
	Some((send_session, sender_persister))
	}
	None => None,
	};
	let sender_state = self.db.get_send_session_id_with_receiver_pk(receiver_pubkey)?.map(|session_id| {
	let sender_persister =
	SenderPersister::from_id(self.db.clone(), session_id)?;
	let (send_session, _) = replay_sender_event_log(&sender_persister)?;
	Ok((send_session, sender_persister))
	}).transpose()?;

[nothingmuch]

firstly, i think this can be renamed get_resumable_session_id_with_receiver_pk and have a WHERE completed_at IS NOT NULL

secondly, this assumes no duplicate receiver pubkeys will ever be added to the database, i think that's fine for now (modulu suggestion re comparing other params) but more future proof would be to avoid query_row, whose docs say:

If the query returns more than one row, all rows except the first are ignored.

being a bit more strict about this would allow us to reliably detect errors in the future even if we eventually decide change the behavior to allow the sender to respond to a buggy receoiver which reuses ephemeral keys for some reason, but if the behavior is to ignore any subsequent matching sessions and with no ORDER BY clause may result in undefined or even non-deterministic behavior, but realistically always return the oldest such session (which is least likely to the one intended for resumption)

[nothingmuch]

the other bip21 params should be compared after recoveing

if feerate is being explicitly overridden that's fine, but if the requested amount or bitcoin address has changed that would be problematic (no harm in comparing pjos param but it's irrelevant here since this is for v2 sender)

[arminsabouri]

Good point. We would need to expose new methods to session history to extract amount and payee spk. What is the expected behavior if the amount and address are not the same? Reject the send or resume? In any case we should not create a new session with different params for the same rk.

[arminsabouri]

And if we going to resume the existing session then I don't think we need to do any additional checks. Perhaps an explicit error makes the most sense here -- since the behavior is unexpected and problematic?

[nothingmuch]

definitely error IMO, even if we wanted to take the complexity hit in order to facilitate resuming in more situations, i don't see how that really makes sense because:

1. it indicates the receiver is deviating from the protocol (repeating ephemeral keys)
2. even if we tolerate that deviation, it's ambiguous if two separate payments for different amounts were intended, or if the existing session should be resumed and modified, or the new params ignored
3. conflicting params don't make sense in the response phase of the session, so the only kind of resumption that is sensible is if there was a transport error from the ohttp directory etc, so the sender should basically create the same message encapsulated with a new ohttp request and submit it to a relay again, in which case i think it's more intuitive to just abandon the previous session and create a new one so that the message and state reflect the replacement parameters

[nothingmuch]

thinking about this more, i don't even know if a receiver can resume its own receive session based on the URL that it gave out previously, that seems kind of weird, i'm almost more inclined to say we should remove the behavior of resuming only a specific session by redoing "send" and just allow sessions, print the session ID and allow that to be specified as a parameter to resume instead... is there a really good UX reason to prefer the already implemented behavior?

(and if you agree then any use of a duplicate URI or duplicate RK should error saying that that payment request was already allocated a session, and print its ID and/or original URI?)

[DanGould]

My thinking was that sending to the same BIP21 implied resumption. However, I do think a clearer interface is one where an error (~"existing session for . use resume to resume) is reported instead of resumption of a send with potentially different parameters.

[nothingmuch]

so i think we can do the following:

1. remove the entire resumption path in send command for 1.0
2. error whenever any duplicate RK is found, even if the rest of the params are identical, but it may make sense to report if they diverge. print the internal session ID
3. before or after 1.0, add a --session-id param to the resume command that works for both sender and receiver sessions

[nothingmuch]

I opened an issue relating to this #1032, tl;dr we should also prevent concurrent resumes