Add receiver pk to session metadata

[arminsabouri]

This allows the refrence to find a resumable session without having to replay each session. And removes a misuse of pj endpoint when trying to filter resumable sessions.

Related ticket: #336

---

Could also do something similar for the receiver where you first generate an ephemeral session id, generate your HPKE keypair then find and replace the session id -- but that is ugly. A much better approach would be a receiver builder model where you first generate the key pair and other parts of the session context and then build(session_persister(short_id)). Builder model is also more attractive bc we are also expanding the session context with optional fields (#933 #920 )

Pull Request Checklist

Please confirm the following before requesting review:

• A human has reviewed every single line of this code before opening the PR (no auto-generated, unreviewed LLM/robot submissions).
• I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

[coveralls]

Pull Request Test Coverage Report for Build 17298830437

Details

• 23 of 24 (95.83%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.01%) to 85.785%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-cli/src/app/v2/mod.rs	9	10	90.0%

Totals
Change from base Build 17297590316:	0.01%
Covered Lines:	8026
Relevant Lines:	9356

---

💛 - Coveralls

[arminsabouri]

impl'd the builder id in 8503e2f

TODO: before this can be undrafted

• Remove UnInitialized
• Update FFI

[nothingmuch]

Hmm, i'm a bit ambivalent about the approach, since short IDs are not guaranteed to be collision free just very unlikely to. In a 24 hour window that probability is very low, but for a user who makes potentially thousands of historical sessions, the birthday bound can conceivably make this non-negligible (but still very unlikely) in the future, and there is no clear way to handle this. This is probably not an issue in practice for payjoin-cli, but as a reference implementation I think it's best we discourage people from doing that, not because this failure case is likely but because in the very unlikely event that it does crop up there's no clean way to handle it.

A more robust approach would be to add the short ID as a column without a unique constraint, and fetch all of the sessions that match it (which should almost always only have 0 or 1 rows), and in the very unlikely event of a collision, compare the full receiver key.

[DanGould]

Even if someone were searching through potentially thousands of historical sessions, the use case we have in mind here is for resumption. In the case of resumption couldn't you just use the most recent, or only look within a pre-defined expiration period?

[nothingmuch]

Even if someone were searching through potentially thousands of historical sessions, the use case we have in mind here is for resumption. In the case of resumption couldn't you just use the most recent, or only look within a pre-defined expiration period?

For querying yes, but if the session ID is the primary key, then that would have to be unique accounting for for completed sessions as well. Our suggestions are basically equivalent.

[DanGould]

in the very unlikely event of a collision, compare the full receiver key

Why not just use the full receiver key instead? @nothingmuch. I feel like we're still not settled on a solution here.

[arminsabouri]

Can we satify #336 by just recommending to the integrations to use a auto incrementing id? We do this in Liana and save the short id as metadata in the sessions table for fast look ups.

[nothingmuch]

Can we satify #336 by just recommending to the integrations to use a auto incrementing id? We do this in Liana and save the short id as metadata in the sessions table for fast look ups.

I think that's the preferred approach. The arguments for this are that it's very widely understood as a pattern, with sqlite or other SQL databases. More importantly application developers don't need a surrogate key to link back to their information. For example, a wallet with a contacts feature might associate payjoin sessions with specific contacts and therefore choose to store them differently. Demonstrating sequential IDs in payjoin-cli just makes this point but if we eventually also ship this code as a library that would make a material difference.

The argument in favor of just using the receiver key as a primary key is, again assuming the tradeoff is mainly about readability (if it's shipped as a library this may have portability should we choose to support more than just sqlite, debatable), this can simplify session creation somewhat, but would also require rejecting BIP 21 URIs with an RK that matches a previously seen one, but which may differ in pjos or the amount parameters, which is essentially the same kind of error condition as using RK or the short ID as merely a surrogate key.

[arminsabouri]

I agree with the above. With all that said the contents of this PR are still independently useful for two reasons.

1. Receiver builder model which can be cherrypicked into its own PR
2. Session Id as metadata so we dont have to replay each session to determine if we have a resumable session

[DanGould]

ACK 9180edd [edited: my initial commit has was for the parent since I git reset to check the diff`. fixed]

• Keep session_id as primary key with autoincrement
• Similar to Yuval's suggestion, add the receiver public key as a column without a unique constraint, and fetch all of the sessions that match it (which[, unlike ShortID,] should almost always only have 0 or 1 rows), and in the very unlikely event of a collision, compare the full receiver key.

[DanGould]

I think this is a Deserialize Error. Should that db::Error::Deserialize variant be a unit type so that it's appropriate here as well?

Or do we just need a new variant? I can see how this wouldn't be the end of the world to panic because there's nothing else to do as long as get_send_session_receiver_pk is only being used in the one place it currently is

[arminsabouri]

yeah I was kinda ambivalent about this expect. If its truly undeserializable you got bigger problems and a panic is appropriate. But we don't really do that anywhere else in db.rs. db::Error::Deserialize makes the most sense if rust is not mad at the internal type

[DanGould]

if its truly undeserializable you got bigger problems

Ack and merging

this is tiny techdebt outside of payjoin crate.

[DanGould]

This expect seems OK since it's looking for exactly one session to send. The other one seems less ok because the context does not restrict it to exactly one use.

[nothingmuch]

wouldn't it be simpler to add a variant of get_sens_session_ids that can use the expected RK in a WHERE clause?

[arminsabouri]

yes it would

[arminsabouri]

This PR was written with sled in mind and I totally forgot I could do this after I rebased.