Skip to content

Look up RK instead of looping #1013

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
arminsabouri wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Look up RK instead of looping #1013

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 10 additions & 18 deletions payjoin-cli/src/app/v2/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,24 +105,16 @@ impl AppTrait for App {
}
PjParam::V2(pj_param) => {
let receiver_pubkey = pj_param.receiver_pubkey();
let sender_state =
self.db.get_send_session_ids()?.into_iter().find_map(|session_id| {
let session_receiver_pubkey = self
.db
.get_send_session_receiver_pk(&session_id)
.expect("Receiver pubkey should exist if session id exists");
if session_receiver_pubkey == *receiver_pubkey {
let sender_persister =
SenderPersister::from_id(self.db.clone(), session_id).ok()?;
let (send_session, _) = replay_sender_event_log(&sender_persister)
.map_err(|e| anyhow!("Failed to replay sender event log: {:?}", e))
.ok()?;

Some((send_session, sender_persister))
} else {
None
}
});
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)?;
Copy link
Collaborator

@nothingmuch nothingmuch Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

@arminsabouri arminsabouri Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@arminsabouri arminsabouri Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@nothingmuch nothingmuch Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@nothingmuch nothingmuch Sep 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor

@DanGould DanGould Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@nothingmuch nothingmuch Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@nothingmuch nothingmuch Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Some((send_session, sender_persister))
}
None => None,
};
Comment on lines +108 to +117
Copy link
Collaborator

@nothingmuch nothingmuch Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?;


let (sender_state, persister) = match sender_state {
Some((sender_state, persister)) => (sender_state, persister),
Expand Down
16 changes: 9 additions & 7 deletions payjoin-cli/src/db/v2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use payjoin::persist::SessionPersister;
use payjoin::receive::v2::SessionEvent as ReceiverSessionEvent;
use payjoin::send::v2::SessionEvent as SenderSessionEvent;
use payjoin::HpkePublicKey;
use rusqlite::params;
use rusqlite::{params, OptionalExtension};

use super::*;

Expand Down Expand Up @@ -219,14 +219,16 @@ impl Database {
Ok(session_ids)
}

pub(crate) fn get_send_session_receiver_pk(
pub(crate) fn get_send_session_id_with_receiver_pk(
&self,
session_id: &SessionId,
) -> Result<HpkePublicKey> {
receiver_pk: &HpkePublicKey,
) -> Result<Option<SessionId>> {
let conn = self.get_connection()?;
let mut stmt =
conn.prepare("SELECT receiver_pubkey FROM send_sessions WHERE session_id = ?1")?;
let receiver_pubkey: Vec<u8> = stmt.query_row(params![session_id.0], |row| row.get(0))?;
Ok(HpkePublicKey::from_compressed_bytes(&receiver_pubkey).expect("Valid receiver pubkey"))
conn.prepare("SELECT session_id FROM send_sessions WHERE receiver_pubkey = ?1")?;
let session_id = stmt
.query_row(params![receiver_pk.to_compressed_bytes()], |row| row.get(0))
.optional()?;
Comment on lines +228 to +231
Copy link
Collaborator

@nothingmuch nothingmuch Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Ok(session_id.map(SessionId))
}
}