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-    }