Use reply key for replyable errors to v2 senders by nothingmuch · Pull Request #981 · payjoin/rust-payjoin

nothingmuch · 2025-08-20T21:02:56Z

Before some relatively recent spec changes, two messages were written to the same mailbox. The error path was not updated to reflect this, so the sender will not receive it (as it is polling a different mailbox), and the receiver further relied on the directory allowing mailbox contents to be overwritable (or at least accepting the POST request).

With this change a replyable error will be sent to the reply key, assuming one was obtained from the first message. If no key can be recovered from the message then there is no point in sending a reply as the peer is not adhering to the protocol.

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.

arminsabouri

Approach ACK

coveralls · 2025-08-22T14:34:39Z

Pull Request Test Coverage Report for Build 17306832046

Details

221 of 222 (99.55%) changed or added relevant lines in 7 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.2%) to 85.967%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-cli/src/app/v2/mod.rs	2	3	66.67%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/core/receive/v2/session.rs	1	97.35%

Totals
Change from base Build 17305633599:	0.2%
Covered Lines:	8166
Relevant Lines:	9499

💛 - Coveralls

DanGould · 2025-08-22T14:36:13Z

+    /// The mailbox ID where the receiver expects the sender's Original PSBT
+    /// payjoin proposal.
+    pub(crate) fn proposal_mailbox_id(&self) -> ShortId {
+        short_id_from_pubkey(self.receiver_key.public_key())
+    }


"Original PSBT payjoin proposal" uses both names for the Original PSBT and the Payjoin Proposal. Which one is it? IF from sender, Original, if from receiver, Proposal.

https://github.com/bitcoin/bips/blob/master/bip-0077.md#sequence-diagram

yeah i hate that but in the code the typestate is UncheckedProposal so i felt like i had to put that in there to be less confusing...

i guess this should rename UncheckedProposal to UncheckedOriginalPSBT?

also do you mind if Original is renamed to OriginalPSBT for consistency with the spec? Original on its own seems harder to digest even if it's more accurate and less redundant

nothingmuch · 2025-08-22T14:36:20Z

there's a big FIXME comment re session_history.extract_err_request, but because optionality of the reply key in the session is also used to distinguish v1/v2 requests internally and lifting reply_key to the Receiver typestate structs themselves is very boilerplate heavy, i left it as is for now even though i was hoping to make some progress on #929 i gave up on that for this PR

arminsabouri

Approach/utACK
Core logic for updating error request mailbox lgmt. The rest of the refactors and naming updates all seem sane as well. Thanks 👍

arminsabouri · 2025-08-27T20:05:25Z

+        // FIXME ideally this should be more like a method of
+        // Receiver<UncheckedProposal> and subsequent states instead of the
+        // history as a whole since it doesn't make sense to call it before,
+        // reaching that state.


The most ergonomic approach imo is to have v2 receiver errors to be convertable to json replies iff they are fatal errors. That way you can send the error reply after you fail to process a typestate or send it after you replay the SEL.

hmm, that sounds right to me, but i don't see how this would specifically avoid this method of the context being callable even in states where it doesn't make sense... i guess the mailbox ID would be derived as part of that conversion? would it be captured into the fatal error?

either way do you mind opening an issue with a bit more detail than this comment? i would prefer to do it as a followup

Yes. In fact we have this issues. But your comment about the different mailbox id is new context that we haven't considered yet. #793

nothingmuch · 2025-08-28T18:05:11Z

@DanGould i've renamed as per your suggestions in DM, but having second thoughts about updating the spec since part of our rationale for "Original PSBT" is that it exactly matched BIP 78's terminology, confusing though it may be. i do think it's appropriate in the code since the state is not just the psbt but the psbt, the params and the reply key (although the OriginalPayload struct does not include the reply key as that is v2 specific).

nothingmuch · 2025-08-28T18:10:00Z

i guess ship has sailed on calling them "sender's proposal" and "receiver's proposal" lol

nothingmuch · 2025-08-28T18:14:41Z

hmm, this now introduces mutant failures but i'm not convinced they're worth addressing, if i understand Armin's suggestions then I think that would eliminate them, the only caller of session_context is extract_err_request so if we refactor that i think that method could go away so i am inclined to ignore these mutants

arminsabouri · 2025-08-28T19:38:49Z

hmm, this now introduces mutant failures but i'm not convinced they're worth addressing, if i understand Armin's suggestions then I think that would eliminate them, the only caller of session_context is extract_err_request so if we refactor that i think that method could go away so i am inclined to ignore these mutants

Makes sense. No need to write tests rn that we would have to remove soon

benalleng · 2025-08-28T19:41:44Z

If I'm not mistaken these are the same mutants covered by these tests #915 (comment), right?

arminsabouri · 2025-08-28T19:47:58Z

Please push the mutations ignore and I will review and drop a re-ack Or migrate the tests mentioned here #981 (comment)

nothingmuch · 2025-08-28T19:59:56Z

If I'm not mistaken these are the same mutants covered by these tests #915 (comment), right?

oh right i had mentally filed that into the next PR to rebase forgetting that it's based on this one... i will add them to this one thanks again for that!

Before some relatively recent spec changes, two messages were written to the same mailbox. The error path was not updated to reflect this, so the sender will not receive it (as it is polling a different mailbox), and the receiver further relied on the directory allowing mailbox contents to be overwritable (or at least accepting the POST request). With this change a replyable error to a v2 sender will be sent to the reply key, assuming one was obtained from the first message. If no key can be recovered from the message then there is no point in sending a reply as the peer is not adhering to the protocol.

Co-authored-by: Ben Allen <benalleng@gmail.com>

arminsabouri

re-ACK

arminsabouri · 2025-08-29T12:18:10Z

-            session_context: SHARED_CONTEXT.clone(),
+            original: OriginalPayload { psbt: PARSED_ORIGINAL_PSBT.clone(), params },
+            session_context: SessionContext {
+                reply_key: Some(HpkeKeyPair::gen_keypair().public_key().clone()),


SHARED_CONTEXT should have a fresh keypair. Why would we create a new one here?

pub(crate) static SHARED_CONTEXT: Lazy<SessionContext> = Lazy::new(|| SessionContext { address: Address::from_str("tb1q6d3a2w975yny0asuvd9a67ner4nks58ff0q8g4") .expect("valid address") .assume_checked(), directory: EXAMPLE_URL.clone(), mailbox: None, ohttp_keys: OhttpKeys( ohttp::KeyConfig::new(KEY_ID, KEM, Vec::from(SYMMETRIC)).expect("valid key config"), ), expiry: SystemTime::now() + Duration::from_secs(60), receiver_key: HpkeKeyPair::gen_keypair(), reply_key: None, amount: None, });

SHARED_CONTEXT is used in both uninitialized and uncheckedpayload tests, it's not correct to have a reply key set in the uninitialized state

nothingmuch requested a review from arminsabouri August 20, 2025 21:02

nothingmuch force-pushed the use-reply-key-for-replyable-errors branch from fbcd995 to 1d288a9 Compare August 20, 2025 23:41

nothingmuch commented Aug 20, 2025

View reviewed changes

Comment thread payjoin/src/core/receive/v2/mod.rs Outdated

arminsabouri reviewed Aug 21, 2025

View reviewed changes

nothingmuch force-pushed the use-reply-key-for-replyable-errors branch from 1d288a9 to 6ef1772 Compare August 22, 2025 14:30

nothingmuch marked this pull request as ready for review August 22, 2025 14:31

nothingmuch changed the title ~~[WIP] Use reply key for replyable errors~~ Use reply key for replyable errors to v2 senders Aug 22, 2025

nothingmuch force-pushed the use-reply-key-for-replyable-errors branch from 6ef1772 to 7b18d7b Compare August 22, 2025 14:34

DanGould reviewed Aug 22, 2025

View reviewed changes

nothingmuch requested a review from arminsabouri August 22, 2025 14:39

nothingmuch force-pushed the use-reply-key-for-replyable-errors branch 2 times, most recently from 51b838e to beeb471 Compare August 22, 2025 17:05

arminsabouri approved these changes Aug 27, 2025

View reviewed changes

nothingmuch force-pushed the use-reply-key-for-replyable-errors branch 2 times, most recently from d4b0295 to 373a802 Compare August 28, 2025 18:00

nothingmuch requested a review from DanGould August 28, 2025 18:03

nothingmuch force-pushed the use-reply-key-for-replyable-errors branch from 373a802 to 99b6b91 Compare August 28, 2025 18:10

arminsabouri mentioned this pull request Aug 28, 2025

Refactor Receiver Replyable Error #793

Closed

nothingmuch force-pushed the use-reply-key-for-replyable-errors branch from 99b6b91 to 36580dd Compare August 28, 2025 19:29

arminsabouri reviewed Aug 28, 2025

View reviewed changes

Comment thread payjoin/src/core/receive/v2/mod.rs Outdated

nothingmuch and others added 3 commits August 28, 2025 22:03

Rename Unchecked{Proposal,OriginalPayload}

94063ce

Rename Original to OriginalPayload

41332a8

mutation coverage for replyable errs w/ reply key

68608bf

Co-authored-by: Ben Allen <benalleng@gmail.com>

nothingmuch force-pushed the use-reply-key-for-replyable-errors branch from 36580dd to 68608bf Compare August 28, 2025 20:12

arminsabouri approved these changes Aug 29, 2025

View reviewed changes

nothingmuch mentioned this pull request Aug 29, 2025

Configure directory with config & clap #927

Merged

arminsabouri merged commit 676f2a5 into payjoin:master Aug 29, 2025
14 checks passed

nothingmuch mentioned this pull request Aug 30, 2025

Replace redis with file based mailbox storage #915

Merged

nothingmuch deleted the use-reply-key-for-replyable-errors branch September 25, 2025 20:04

Conversation

nothingmuch commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Uh oh!

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 17306832046

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nothingmuch commented Aug 22, 2025

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nothingmuch commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nothingmuch commented Aug 28, 2025

Uh oh!

nothingmuch commented Aug 28, 2025

Uh oh!

arminsabouri commented Aug 28, 2025

Uh oh!

benalleng commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arminsabouri commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nothingmuch commented Aug 28, 2025

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nothingmuch commented Aug 20, 2025 •

edited

Loading

coveralls commented Aug 22, 2025 •

edited

Loading

nothingmuch commented Aug 28, 2025 •

edited

Loading

benalleng commented Aug 28, 2025 •

edited

Loading

arminsabouri commented Aug 28, 2025 •

edited

Loading