One expiration error: Separate session replay & protocol operation by DanGould · Pull Request #1036 · payjoin/rust-payjoin

DanGould · 2025-09-03T16:49:53Z

Do not throw an Expired error as a ReplayError variant. An expired
session does indicate an error with replay. Rather, a replay of an
expired session that has not yet reached a terminal state will produce
an error when it follows protocol and tries to create a request. That
error will create a SessionEvent::SessionInvalid (or whatever that type
turns into after follow ups) which will later be replayed by the state
machine.

This separates the concerns of the protocol from the session replay and
more accurately reflects the protocol operation. This lets us record to
what extent the protocol actually was able to operate before expiration
since all of the events will be replayed up to the terminal expiration
event.

I noticed that the receiver's final post was not checking expiration, so
I added and tested that as well after removing the replay expiration
test.

Make SessionError(pub(super) InternalSessionError) so that the error
variant can be used in matches in unit tests.

I did use claude-4-sonnet to write the skeleton of the fn test_create_post_request_fails_when_expired( so that I didn't have to fetch all of the boilerplate. After it was done I used ctrl+k in-file llm edit with gpt-4o to "prune this" to reduce some redundancy and generated comments. Then, because the robot generated assertions against the Display impl of the error I rewrote the assert statements to match against the exact variant I expected. so that the test was as precise as possible.

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.

coveralls · 2025-09-03T16:53:33Z

Pull Request Test Coverage Report for Build 17554627187

Details

74 of 74 (100.0%) changed or added relevant lines in 3 files are covered.
9 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.007%) to 85.926%

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

Totals
Change from base Build 17504088813:	-0.007%
Covered Lines:	8224
Relevant Lines:	9571

💛 - Coveralls

spacebear21 · 2025-09-03T17:45:28Z

payjoin/src/core/receive/v2/mod.rs

+        let mut valid_proposal = payjoin_proposal_from_test_vector();
+        assert!(!matches!(
+            valid_proposal.create_post_request(&*EXAMPLE_URL),
+            Err(Error::Protocol(ProtocolError::V2(SessionError(InternalSessionError::Expired(_)))))
+        ));


Why is the "valid_proposal" expected to fail?

It's not: !matches! not matches!

~~Perhaps assert_not! is more legible?~~ <- that's a whole ass dependency or another macro, not part of the language.

Or would you prefer the condition be something else?

Oh I see, why not is_ok() or something similar?

My rationale here is that it's supposed to be NOT that one error for sure, but not necessarily OK or another specific error. is that not the right test condition?

If we it's a "valid_proposal" it implies to me it should be "ok" but I may just be bikeshedding now.

spacebear21 · 2025-09-03T17:56:11Z

payjoin/src/core/receive/v2/mod.rs

+        if SystemTime::now() > self.state.session_context.expiry {
+            return Err(InternalSessionError::Expired(self.state.session_context.expiry).into());
+        }


I'm trying to think of a good reason why a Receiver might want to post the proposal PSBT even after expiry, and I'm coming up short.

yeah I don't think there is one. At that point it's optional to post the fallback and that's it.

spacebear21

The SystemTime exclusion in mutants.toml needs to be updated to fix CI.

DanGould · 2025-09-03T19:39:05Z

Fixed the mutants exclusion. Thanks for the suggestion I did not find that on my own.

arminsabouri · 2025-09-03T20:18:29Z

payjoin/src/core/receive/v2/mod.rs

        &mut self,
        ohttp_relay: impl IntoUrl,
    ) -> Result<(Request, ohttp::ClientResponse), Error> {
+        if SystemTime::now() > self.state.session_context.expiry {


I agree that expired session is not a replay error. And it should have never been. However it still make sense and provides better UX to error out on protocol related errors as soon as we can.

Consider Liana hardware signer(s) that replay an expired session, collect signature(s) from their airgapped hw wallet, and then gets a session is expired error. This can be avoided if we propogate protocol errors during session replays.

~~I do not have a great suggestion. Perhaps we can add ProtocolError as a variant to InternalReplayError.~~

Edit: that is essentially what we do today.

I think this is simple to solve actually. When a protocol error is hit for the first time, it'll create a TerminalFailure event (SessionInvalid as of this PR) which gets persisted. When the session is replayed this is hit and that error can be displayed. I think that is current behavior. It's possible for us to check for TerminalFailure straight away in replay as an optimization.

Consider Liana hardware signer(s) that replay an expired session, collect signature(s) from their airgapped hw wallet, and then gets a session is expired error

To alleviate this specific concern another expiration check could be done before any sort of async check that might have a great deal of delay before the next expiration check

I think that is current behavior.

Yes, I believe this is the current behavior. And this is why expired session was a special carveout. All other protocol errors would have and will be encountered during state machine progression.

It is. For some reason I figured the request for a signature would initiate the closure that could check for expiration but I see how that's a problem. The replay itself needs to check for this protocol error.

Is there still a good reason to check expiry in create_post_request if we already checked on replay?

No I don't think so.
Its currently checked in 4 places:

In replay_event_log (session.rs:71)

In create_post_request (mod.rs:252)

In process_post_request (mod.rs:344)

In process_get_request (mod.rs:1024)

I believe create_post_request is redundant if a non-default expiry is long enough.

Imagine a receiver that never goes offline and replays. In that case I think at each stage you do indeed need to check the expiration. Because an arbitrary amount of time can pass between each stage.

The only place I can see this being unnecessary is in process_post_request if a good payload is returned and the state machine would have already progressed to the point that it's waiting on the counterparty to sign and broadcast.

if a non-default expiry is long enough

This assumption would then have to be enforced

Because an arbitrary amount of time can pass between each stage.

In practice this could be a likely outcome. A server could keep a session active in memory and never replay.

The only place I can see this being unnecessary is in process_post_request

I agree.

arminsabouri · 2025-09-05T18:24:10Z

payjoin/src/core/receive/v2/session.rs

    }

+    let ctx =
+        history.session_context().expect("Session context should be present after the first event");


Note: this expect makes more sense after we remove uninitlized as a session state #1014

spacebear21

This PR addresses the receiver side, I suppose we'd also want to check session expiry when replaying sender session events?

spacebear21 · 2025-09-05T19:12:41Z

payjoin/src/core/receive/v2/session.rs


+    let ctx =
+        history.session_context().expect("Session context should be present after the first event");
+    if SystemTime::now() > ctx.expiry {


I expect this needs a mutants exclusion too, see #1036 (review)

copy that. I excluded this pattern: ""replace > with >= in replay_event_log","

We'll see if that works. Thanks

spacebear21 · 2025-09-05T19:16:25Z

payjoin/src/core/receive/v2/mod.rs

+        let mut valid_proposal = payjoin_proposal_from_test_vector();
+        assert!(!matches!(
+            valid_proposal.create_post_request(&*EXAMPLE_URL),
+            Err(Error::Protocol(ProtocolError::V2(SessionError(InternalSessionError::Expired(_)))))
+        ));


If we it's a "valid_proposal" it implies to me it should be "ok" but I may just be bikeshedding now.

payjoin/src/core/receive/v2/session.rs

spacebear21 · 2025-09-05T19:20:04Z

payjoin/src/core/receive/v2/mod.rs

        &mut self,
        ohttp_relay: impl IntoUrl,
    ) -> Result<(Request, ohttp::ClientResponse), Error> {
+        if SystemTime::now() > self.state.session_context.expiry {


Is there still a good reason to check expiry in create_post_request if we already checked on replay?

arminsabouri · 2025-09-05T21:28:27Z

@spacebear21
sender side is now implemented in e7406f9

arminsabouri · 2025-09-05T21:31:49Z

I am realizing the behavior for handling an expired session is different when replaying and normal state machine progressions.

if SystemTime::now() > self.context.expiry {
            return Err(InternalSessionError::Expired(self.context.expiry).into());
        }

In most cases we return an error instead of closing teh session and pushing an error event. I don't think that should be a blocker for this release. But something to figure out before the 1.0 RC release.

Do not throw an Expired error as a ReplayError variant. An expired session does indicate an error with replay. Rather, a replay of an expired session that has not yet reached a terminal state will produce an error when it follows protocol and tries to create a request. That error will create a SessionEvent::SessionInvalid (or whatever that type turns into after follow ups) which will later be replayed by the state machine. This separates the concerns of the protocol from the session replay and more accurately reflects the protocol operation. This lets us record to what extent the protocol actually was able to operate before expiration since all of the events will be replayed up to the terminal expiration event. I noticed that the receiver's final post was not checking expiration, so I added and tested that as well after removing the replay expiration test. Make SessionError(pub(super) InternalSessionError) so that the error variant can be used in matches in unit tests.

extract_v2 was renamed create_v2_post_request.

Close the session and save a session expired error after replaying the event log.

Close the session and save a session expired error after replaying the sender event log.

spacebear21

ACK 95a2bed

I don't think that should be a blocker for this release. But something to figure out before the 1.0 RC release.

Isn't this release the 1.0 RC release?

arminsabouri · 2025-09-08T15:40:54Z

ACK 95a2bed

I don't think that should be a blocker for this release. But something to figure out before the 1.0 RC release.

Isn't this release the 1.0 RC release?

My bad. Typo. I meant to say "shouldn't be a blocker for this PR getting merged"

DanGould requested a review from arminsabouri September 3, 2025 16:50

spacebear21 reviewed Sep 3, 2025

View reviewed changes

spacebear21 requested changes Sep 3, 2025

View reviewed changes

DanGould force-pushed the one-expiry-error branch from 8ba186a to 409b1ab Compare September 3, 2025 19:37

arminsabouri mentioned this pull request Sep 3, 2025

Display session history in payjoin-cli #1039

Merged

2 tasks

DanGould requested a review from spacebear21 September 3, 2025 20:11

arminsabouri reviewed Sep 3, 2025

View reviewed changes

DanGould marked this pull request as draft September 3, 2025 23:25

arminsabouri self-assigned this Sep 5, 2025

arminsabouri force-pushed the one-expiry-error branch from 409b1ab to 61c3c16 Compare September 5, 2025 18:22

arminsabouri reviewed Sep 5, 2025

View reviewed changes

arminsabouri marked this pull request as ready for review September 5, 2025 18:24

spacebear21 reviewed Sep 5, 2025

View reviewed changes

arminsabouri force-pushed the one-expiry-error branch 2 times, most recently from 85d94f2 to e5687ea Compare September 5, 2025 21:05

arminsabouri added this to the payjoin-1.0 milestone Sep 8, 2025

arminsabouri force-pushed the one-expiry-error branch from e7406f9 to b6a4ebe Compare September 8, 2025 14:11

DanGould and others added 6 commits September 8, 2025 10:42

Rename tests to match functions they're testing

5e700f3

extract_v2 was renamed create_v2_post_request.

Check for expired sessions after replaying recv logs

e3522c2

Close the session and save a session expired error after replaying the event log.

Check for expired sessions after replaying send logs

7bdc639

Close the session and save a session expired error after replaying the sender event log.

Fix typos in mutants.toml

d9e7255

Add test coverage for replaying expired sessions

95a2bed

arminsabouri force-pushed the one-expiry-error branch from b6a4ebe to 95a2bed Compare September 8, 2025 14:43

arminsabouri requested a review from spacebear21 September 8, 2025 15:05

spacebear21 approved these changes Sep 8, 2025

View reviewed changes

arminsabouri merged commit ca35bac into payjoin:master Sep 8, 2025
10 checks passed

arminsabouri mentioned this pull request Sep 8, 2025

Expired sessions should close the session #1049

Closed

spacebear21 mentioned this pull request Sep 8, 2025

Replace SystemTime with bitcoin::absolute::Time #1047

Merged

2 tasks

arminsabouri mentioned this pull request Sep 8, 2025

Change return type of apply_unchecked_from_payload #1053

Merged

2 tasks

Conversation

DanGould commented Sep 3, 2025

Pull Request Checklist

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 17554627187

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

DanGould commented Sep 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

DanGould Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

spacebear21 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!

Choose a reason for hiding this comment

Uh oh!

arminsabouri commented Sep 5, 2025

Uh oh!

arminsabouri commented Sep 5, 2025

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

arminsabouri commented Sep 8, 2025

Uh oh!

Uh oh!

Reviewers

coveralls commented Sep 3, 2025 •

edited

Loading

DanGould Sep 3, 2025 •

edited

Loading

arminsabouri Sep 3, 2025 •

edited

Loading

DanGould Sep 8, 2025 •

edited

Loading