Ambiguous state when persisting `MaybeTerminalSuccessTransition`

[xstoicunicornx]

Which crate is this feature request for (if any)?

payjoin

Question

What is the recommended method for distinguishing between a MaybeTerminalSuccessTransition::FatalTerminate and a MaybeTerminalSuccessTransition::Transient that is persisted? Specifically, after persisting the transition how are callers expected to determine whether the error is for a transient error that should be retried or a fatal error that has closed the session?

For example you can see that currently in payjoin-cli it is currently only handling the MaybeTerminalSuccessTransition::FatalAdvance transition, leaving the other two ambiguously unhandled.

rust-payjoin/payjoin-cli/src/app/v2/mod.rs

Lines 1040 to 1053 in f60825c

	if let Err(e) = session.process_error_response(&err_bytes, err_ctx).save(persister) {
	if let Some(api_err) = e.api_error_ref() {
	tracing::warn!("Failed to confirm error response delivery: {api_err}");
	}
	match e.error_state() {
	Some(_) => {
	let id = persister.session_id();
	println!(
	"Session {id} failed. Run `payjoin-cli cancel {id}` to cancel and broadcast the fallback transaction."
	);
	}
	None => return Err(anyhow!("Failed to process error response")),
	}
	}

[benalleng]

Do you think we should add a basic is_transient() -> bool method that can tell you to retry or do you think it would be better to have error_state() return an enum that is able to distinguish the cases? Or maybe just return ApiError itself publicly

[xstoicunicornx]

Personally I would lean towards having something like ApiError::TransientWithState that mimics the ApiError::FatalWithState. That way the state machine doesn't require caching of the last state. Then the callers can parse whether the returned error state is still HasReplyableError or has moved on to PendingFallback.

[benalleng]

it just feels like antipattern for Ok() to all return next state, and for fatal advance to return next state, but for transient error caller is expected to know to use last state
i think setting expectation that persisting will always return the current/next state, apart from when the session is closed, is probably a good expectation to set?

Ok, I think I like transient not returning an error in the persister and instead maybe using an enum for the success outcomes in the persister to cover advancing to the next state / closing or returning the current state

[chavic]

Opened #1716 as a concrete minimal proposal from the bindings-consumer side: is_transient() / is_fatal() on PersistedError, plus carrying the kind through payjoin-ffi's ReceiverPersistedError — which currently collapses it a second time, so whatever shape lands in core, the FFI conversion needs to carry it or downstream languages stay stuck. It's deliberately the smallest signal available today and doesn't preclude TransientWithState or the persister-outcome-enum direction discussed above; details and a field report from the BTCPay Server payjoin plugin are in the PR.

[spacebear21]

I agree that returning the current state inside the Transient variant makes sense for ergonomics, and this should be a 1.0 change IMO.

[xstoicunicornx]

Pulling on this thread a bit more just to get other's thoughts - what about the other transient errors? For example for MaybeFatalOrSuccessTransition and MaybeFatalTransition, should these transient errors also return the current state to enforce the pattern that persisting should always return the current state (apart from when the session is closed)?

[spacebear21]

what about the other transient errors?

Yes, I think whatever approach we end up with we'd want to treat all transient errors the same way.

[spacebear21]

@xstoicunicornx curious how you found this problem? Were you sifting through the code or did you actually run into an issue writing/using an implementation? If the latter, how do the proposed solutions so far compare in terms of ergonomics for addressing the issue you encountered?

[xstoicunicornx]

I was reviewing the receiver cancel/pending fallback PR and was scrutinizing HasReplyableError. Went to payjoin-cli to understand how transient was to be used and realized that is doesn't handle the transient case at all. Then I couldn't find a good way of even implementing a fix to handle the transient case.

In terms of ergonomics I don't really have much of an opinion, should be ergonomic enough? I do think it is clearer and more consistent, which I think is important.