Be specific about error handling for methods that don't return state transitions

[spacebear21]

Inspired by discussion in: lightningdevkit/ldk-node#746 (comment)

create_poll_request and create_post_request failures are all fatal and should always result in terminating the session and broadcasting the fallback.

try_preserving_privacy and contribute_inputs are conditionally retryable (can't succeed with the same input set, but may succeed with a different input set) and the appropriate reaction is an implementation consideration.

None of these return state transition types with "fatal" vs. "transient" error distinction, so the proper handling is not obvious to implementers.

[bc1cindy]

hey @spacebear21 I'll work on it, ok?

[spacebear21]

Just a heads up @bc1cindy this issue isn't as trivial as it might seem. It might require new typestates or re-thinking the create_*_request/process_response pattern entirely. Additionally, it's unclear whether the create_*_request should even be failable at all assuming we've already vetted the key materials and urls. The ohttp library has weird function signatures like this one, which has no real reason to return a Result and is forcing us to propagate up the function chain:

    /// Get the encapsulated KEM secret.
    #[allow(clippy::unnecessary_wraps)]
    pub fn enc(&self) -> Res<Vec<u8>> {
        Ok(self.enc.clone())
    }

[arminsabouri]

It is a bit off to me that crypto related things (ohttp encap) could fail if the key material and urls are already vetted. Things like nonce overflow make sense from a library prespective but not for pdk. We could just .expect() that

if self.0.overflowed {
            // If the sequence counter overflowed, we've been used for far too long. Shut down.
            Err(HpkeError::MessageLimitReached)
        } 

Other things in the ohttp / hpke crate seem to be using results incorrectly
e.g in bitcoin-ohttp

 /// Get the encapsulated KEM secret.
    #[allow(clippy::unnecessary_wraps)]
    pub fn enc(&self) -> Res<Vec<u8>> {
        Ok(self.enc.clone())
    }

So i would like to do a overview of what the actual fatal errors are and which ones are just non-idiomatic code from downstream libraries

[bc1cindy]

got it! I was still trying to understand better the issue. looking at the code, OhttpEncapsulation in create_poll_request seems unreachable in practice, sinceOhttpKeys is only constructible with a valid key config, so by the time create_poll_request is called the keys are already vetted and this error really might be not necessary

payjoin/src/core/ohttp.rs-    /// Decode an OHTTP KeyConfig
payjoin/src/core/ohttp.rs-    pub fn decode(bytes: &[u8]) -> Result<Self, ohttp::Error> {
payjoin/src/core/ohttp.rs-        ohttp::KeyConfig::decode(bytes).map(Self)
payjoin/src/core/ohttp.rs-    }

[chavic]

Some implementation experience on the "conditionally retryable" pair, plus a concrete proposal in #1717.

The BTCPay Server payjoin plugin drives try_preserving_privacy / contribute_inputs with a candidate-snapshot loop: snapshot spendable receiver coins, let selection pick, and on failure remove the offending candidate and retry with the reduced set until success or exhaustion. Because the errors are opaque, the loop has to treat every failure as maybe-retryable-with-different-inputs, and burns attempts on failures like ValueTooLow where removing candidates can never help. #1717 proposes kind() accessors returning #[non_exhaustive] kind enums (mirrored across the FFI) so callers get a matchable classification without freezing the error variants.

One data point on "the appropriate reaction is an implementation consideration" for the exhaustion case: we initially kept the session alive to retry contribution on later polls, and reverted it. A session whose sender has likely given up lingers until expiry while its reservation-based selection keeps grabbing freshly confirmed UTXOs, starving other concurrent payjoins — our concurrency tests caught exactly this. We now conclude the session on exhaustion, the async analog of a v1 endpoint answering "payjoin unavailable": the sender falls back and the original transaction still pays. Might be worth capturing in whatever guidance comes out of this issue.

[spacebear21]

This shouldn't have been closed by #1724