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

Activity

1. spacebear21

   added

   api

   on Mar 18, 2026
2. bc1cindy commented on Mar 18, 2026

   

   bc1cindy

   on Mar 18, 2026

   Contributor

   More actions

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

   👍React with 👍1Reacted by spacebear
3. spacebear21 commented on Mar 19, 2026

   

   spacebear21

   on Mar 19, 2026

   CollaboratorAuthor

   More actions

   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())
       }
4. arminsabouri commented on Mar 19, 2026

   

   arminsabouri

   on Mar 19, 2026

   Last edited by arminsabouri

   Contributor

   More actions

   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
5. bc1cindy commented on Mar 19, 2026

   

   bc1cindy

   on Mar 19, 2026

   Contributor

   More actions

   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-    }
   
6. chavic commented on Jul 9, 2026

   

   chavic

   on Jul 9, 2026

   Collaborator

   More actions

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

   added 3 commits that reference this issue on Jul 9, 2026

   Classify FFI persisted errors as transient/fatal

   ...

   3e3593d

   Expose transient vs fatal on PersistedError

   ...

   c7dc8d6

   Classify FFI persisted errors as transient/fatal

   ...

   7f7003d
8. spacebear21

   closed this as completedin #1724on Jul 10, 2026
9. spacebear21 commented on Jul 14, 2026

   

   spacebear21

   on Jul 14, 2026

   CollaboratorAuthor

   More actions

   This shouldn't have been closed by #1724
10. spacebear21

    reopened this on Jul 14, 2026