Receiver fallback typestate#1558
Conversation
f4454b7 to
f175e01
Compare
f175e01 to
56641ee
Compare
Coverage Report for CI Build 26266054947Coverage increased (+0.04%) to 85.184%Details
Uncovered Changes
Coverage Regressions21 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
|
I read the writeups and concept ACK. I wonder if |
In the latest iteration of this PR the typestate is called |
e6828cc to
21722b1
Compare
3993880 to
174281c
Compare
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct PendingFallback { | ||
| fallback_tx: bitcoin::Transaction, | ||
| outcome: SessionOutcome, |
There was a problem hiding this comment.
xstoicunicornx
left a comment
There was a problem hiding this comment.
Only went through the payjoin core changes so far but felt it was a good stopping point for some questions/comments.
| (PersistActions::Save(event), Ok(Some(next_state))), | ||
| MaybeTerminalSuccessOutcome::Terminate(event) => | ||
| (PersistActions::SaveAndClose(event), Ok(None)), | ||
| MaybeTerminalSuccessOutcome::FatalAdvance(event, _next_state, error) => |
There was a problem hiding this comment.
If 'next_state' isn't being used why is it being provided?
| match (process_post_res(res, ohttp_context), pending) { | ||
| (Ok(_), Some(pending_fallback)) => | ||
| MaybeTerminalSuccessTransition::advance(event, pending_fallback), | ||
| (Ok(_), None) => MaybeTerminalSuccessTransition::terminate(event), | ||
| (Err(e), _) if !e.is_fatal() => | ||
| MaybeTerminalSuccessTransition::transient(protocol_error(e)), | ||
| (Err(e), Some(pending_fallback)) => MaybeTerminalSuccessTransition::fatal_advance( | ||
| event, | ||
| pending_fallback, | ||
| protocol_error(e), | ||
| ), | ||
| (Err(e), None) => | ||
| MaybeTerminalSuccessTransition::fatal_terminate(event, protocol_error(e)), | ||
| } |
There was a problem hiding this comment.
This was the main piece I wanted to ask about/check my understanding on.
It seems like there are two types of paths here:
- Return a transition where persistence will only result in persistence error
- Return a transition where persistence may unwrap a protocol error
Is this a correct understanding? If so I am not sure how I feel about that discrepancy. It seems like it would require fatal_advance and transient error paths to preserve the prior receiver state so that the session can be closed via .cancel().
The current version of this method established this pattern, but I am not even sure how the current version would close the session if it was a transient error? Would it have to keep the previous receiver state just in case so that it can rerun create_error_request and process_error_response? Also Receiver<Initialized>::process_response seems to follow a similar pattern, but at least there it consistently only returns transitions that, when persisted, only produce persister errors (though it does make me have more questions on why we handle the other transition states at all when they don't get persisted).
Is there a reason we have to try to handle the process_post_req error (Receiver<Initialized>::process_response doesn't seem to)? Would it be acceptable to just map the errors to the appropriate next state (either PendingFallback or Closed)? Maybe if transient error really is a NoOp within the persister it should return current state?
Apologies for the rambling...
Edit: Looks like MaybeFatalTransition also follows this sort of pattern so maybe this is just me finally learning how the transitions work. Though MaybeFatalTransition does return an error_state wrapped within the error, not sure if it actually meant to be used? I assume yes since its trying to advance to HasReplyableError state?
| } | ||
|
|
||
| #[test] | ||
| fn process_error_response_fatal_with_fallback_enters_pending_fallback() -> Result<(), BoxError> |
There was a problem hiding this comment.
I don't think this actually enters PendingFallback since save() returns and error and not the PendingFallback state?
There was a problem hiding this comment.
I guess this is where the above comment https://github.com/payjoin/rust-payjoin/pull/1558/changes#r3311885155 becomes relevant. Either we choose to wire this _next_state allowing a pending_fallback state to be seen or we drop that and this test just needs a rename.
Based on the current logic in process_error_response I am assuming it was meant to be fully wired up.
| let expected_tx = PARSED_ORIGINAL_PSBT.clone().extract_tx_unchecked_fee_rate(); | ||
| let replyable_error = mock_err(); | ||
|
|
||
| let test_cases = vec![ |
There was a problem hiding this comment.
Might be good to add a test vector for:
SessionEvent::ProtocolFailedresults inReceiveSession::PendingFallback- closing
ReceiveSession::PendingFallbackresults inReceiveSession::Closed
There was a problem hiding this comment.
added additional vectors to cover these in 8f642a4
Introduce a sealed::FallbackTx trait with a non-Option fallback_tx() method, implemented inside the sealed module for the in-protocol receiver states whose contract includes a confirmed broadcastable fallback. Expose access through HasFallbackTx, a public marker trait that has no methods of its own and is implemented for any type satisfying sealed::FallbackTx via a blanket impl. External crates can bound on HasFallbackTx but cannot implement it, and the method itself is only callable from inside the receive::v2 module where the sealed trait is in scope. - UncheckedOriginalPayload is deliberately excluded; it holds the sender's Original PSBT but has not yet run check_broadcast_suitability, so the PSBT is not yet verified as broadcastable. - HasReplyableError is also excluded; it will gain an optional fallback field in a later commit and continues to model the absent-fallback case at runtime. To avoid naming conflicts in intermediate commits, the existing `fallback_tx() -> Option<Transaction>` implementation is renamed to `maybe_fallback_tx`. It is removed entirely in a later commit.
PendingFallback represents a receiver session that was cancelled or hit a fatal protocol error, and has a fallback transaction available to broadcast. While the session sits in PendingFallback the implementer holds an obligation to broadcast, discard, or otherwise handle the fallback transaction (e.g. save it to wallet DB for later broadcasting). This state is preserved across restarts and session replays until the implemeter calls `close()`, indicating that the handoff of the fallback transaction is complete and no longer a payjoin concern.
HasReplyableError represents a receiver session that hit a replyable error before reaching PendingFallback. The struct must model the runtime fact that some sources can hand it a verified broadcastable fallback and others cannot. Encoding the field as Option<Transaction> keeps that distinction at the type level without weakening the HasFallback trait contract.
Introduce MaybeTerminalTransition for the no-error fork (used by cancel) and MaybeTerminalSuccessTransition for the error-bearing fork (used by process_error_response). Both expose advance and terminate constructors that map to Save and SaveAndClose actions respectively. The success variant returns Option<NextState>; the error variants preserve the caller's distinction between transient, fatal-advance, and fatal-terminate.
The receiver side of v2 had a single blanket cancel implementation that always terminated the session and handed the wallet an Option<Transaction>. Fatal protocol errors emitted Closed(Failure) directly. Both shapes lost the wallet's obligation to broadcast the original transaction across a restart whenever a fallback existed. Replace the blanket cancel with typestate-aware impls: - impl<S: HasFallback> Receiver<S>::cancel advances to PendingFallback - Receiver<Initialized>::cancel and Receiver<UncheckedOriginalPayload> ::cancel terminate with Closed(Cancel); neither holds a verified fallback - Receiver<HasReplyableError>::cancel forks on the optional fallback: Some advances to PendingFallback, None terminates with Closed(Cancel)
Drop dead code.
174281c to
a1865a1
Compare
This adds test vectors for - SessionEvent::ProtocolFailed results in ReceiveSession::PendingFallback - ReceiveSession::PendingFallback results in RecieveSession::Closed
Supersedes #1542.
Receiver counterpart to #1557 . It's a much bigger PR because a) the receiver state machine is more complicated than the sender's, and b) it also implements the "protocol failure" fallback path (i.e. not a manually-initiated cancel, but an irrecoverable error that would also warrant broadcasting the fallback tx).
stateDiagram-v2 [*] --> Initialized Initialized --> UncheckedOriginalPayload: RetrievedOriginalPayload UncheckedOriginalPayload --> MaybeInputsOwned: CheckedBroadcastSuitability state "9 HasFallbackTx in-protocol states" as InProtocol { MaybeInputsOwned --> MaybeInputsSeen: CheckedInputsNotOwned MaybeInputsSeen --> OutputsUnknown: CheckedNoInputsSeenBefore OutputsUnknown --> WantsOutputs: IdentifiedReceiverOutputs WantsOutputs --> WantsInputs: CommittedOutputs WantsInputs --> WantsFeeRange: CommittedInputs WantsFeeRange --> ProvisionalProposal: AppliedFeeRange ProvisionalProposal --> PayjoinProposal: FinalizedProposal PayjoinProposal --> Monitor: PostedPayjoinProposal } UncheckedOriginalPayload --> HasReplyableError: GotReplyableError, fallback_tx is None (broadcast unverified) InProtocol --> HasReplyableError: GotReplyableError, fallback_tx is Some InProtocol --> PendingFallback: Cancelled (cancel from any HasFallbackTx state) PayjoinProposal --> PendingFallback: ProtocolFailed (fatal process_response) HasReplyableError --> PendingFallback: Cancelled or ProtocolFailed, when fallback_tx is Some Initialized --> Closed: Closed(Cancel) on cancel, or Closed(Failure) on fatal process_response UncheckedOriginalPayload --> Closed: Closed(Cancel) on cancel HasReplyableError --> Closed: Closed(Cancel) on cancel, or Closed(Failure) on process_error_response, when fallback_tx is None Monitor --> Closed: check_payment observes Success, FallbackBroadcasted, or PayjoinProposalSent PendingFallback --> Closed: close() emits Closed(Cancel) after Cancelled entry, or Closed(Failure) after ProtocolFailed entry Closed --> [*] note right of PendingFallback Holds the wallet's obligation to broadcast or explicitly discard the original tx. Stays active until close() so it survives resume cycles. end notePlanned with Claude Opus 4.7, implemented by Codex 5.5
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.