Async Interface for Payjoin Receiver State Machine

[xstoicunicornx]

Summary

Currently some states (UncheckedOriginalPayload, MaybeInputsOwned, MaybeInputsSeen, OutputsUnknown, ProvisionalProposal, Monitor) require the usage of synchronous callbacks to transition to the next state in the payjoin state machine. This is inconvenient for languages which required the use of asynchronous calls for validation, for example when calling the bitcoin rpc, which use the payjoin FFI language bindings. While there are some workarounds that can be used to adequately validate the state machine transitions (except for UncheckedOriginalPayload and Monitor) when relying on asynchronous validation, these are a bit unwieldily to use. Instead, this PR implements an asynchronous interface that makes state machine transitions with asynchronous calls possible.

This is an alternate version of #1446 . See rationale for reimplementing here.

Background

I was attempting to prove out the Javascript language bindings by implementing a Node version of the payjoin-cli and opened issue #1389 after encountering some of the challenges with using asynchronous validation callbacks. It was revealed to me that this was a known pain point and there was a #816 (comment) proposed by @arminsabouri of creating new state transition methods that accept the result of a validation call. This is my attempt at implementing this solution.

Side Effects

Some additional things that were touched in this PR:

• psbt_to_sign was updated to strip out the sender signatures so that callers don't have to do this themselves
• Light refactor was done to receiver v2 to improve readability (since there is a good amount of duplicated code I wanted make duplicated code easier to maintain)
• Additional tests were added to core receive module to cover all various errors produced by the methods that use validation callbacks
• Type errors in the python and javascript FFI bindings tests were cleaned up

Any and all feedback is welcome!

AI Disclosure

Claude AI was used to write some of the tests and help with troubleshooting.

Please confirm the following before requesting review:

• I have disclosed my use of
  AI
  in the body of this PR.
• I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

[coveralls]

Coverage Report for CI Build 25603774925

Coverage increased (+0.7%) to 85.992%

Details

• Coverage increased (+0.7%) from the base build.
• Patch coverage: 56 uncovered changes across 2 files (980 of 1036 lines covered, 94.59%).
• No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
payjoin/src/core/receive/v2/mod.rs	493	450	91.28%
payjoin/src/core/receive/mod.rs	414	401	96.86%

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	14570
Covered Lines:	12529
Line Coverage:	85.99%
Coverage Strength:	409.19 hits per line

---

💛 - Coveralls

[xstoicunicornx]

I had some additional general questions, let me know if this should be posted in separate issue but if can be resolved here it would be nice to include potential fix ups in this PR:

• check_payment uses extract_tx_fee_rate_limit for the fallback tx but everywhere else uses extract_tx_unchecked_fee_rate, is this an intentional discrepancy? Seems like it has the potential to introduce errors for a fallback tx that was already deemed valid.
• Is it enforced that receiver will only add Segwit inputs? If not wouldn't using payjoin_proposal.unsigned_tx.compute_txid() as the payjoin_txid in check_payment potentially give wrong txid for inputs that have sig field?
• Should finalize_proposal validate that the signed PSBT actually has a valid signature?
• In payjoin crate the Monitor typestate's method is called check_payment but in the FFI bindings it is called monitor, was there a reason for this renaming? check_payment seems to be a more accurate and intuitive name for this method, since the state is monitoring but the method itself is only checking for the payment.

Edit: please respond to this discussion in #1548

[nothingmuch]

• check_payment uses extract_tx_fee_rate_limit for the fallback tx but everywhere else uses extract_tx_unchecked_fee_rate, is this an intentional discrepancy? Seems like it has the potential to introduce errors for a fallback tx that was already deemed valid.

Hmm, good catch.

Since this happens late in the state machine, it may make sense to do that on the fallback transaction when it is first received, but even then game theoretically that's the sender's concern not the receiver's, the receiver should be concerned with the absolute fees they pay or the feerate for their inputs/outputs in the transaction, less so with the payjoin transaction as a whole concerning fees that are too high which is what this method attempt to prevent.

what's worse, this unconditionally calls .expect() so there is no way to handle this error.

Please open an issue even if fixing in this PR so that in case this PR gets bogged down in review we don't lose track of this bugfix

• Is it enforced that receiver will only add Segwit inputs?

No

If not wouldn't using payjoin_proposal.unsigned_tx.compute_txid() as the payjoin_txid in check_payment potentially give wrong txid for inputs that have sig field?

Yes, that's documented on check_payment. Depending on the wallet/node capabilities, different ways of checking for the payment status, for instance by watching for any transaction that spends or creates specific outputs would be more robust but I think that's up to wallets.

That said, you point out a flaw in our design, which is that Receiver<Monitor> arguably should not be part of the state machine if the transaction is non-segwit, and clients that want to rely on that should be able to just broadcast the fallback transaction without contributing their inputs. I think it can be split up into Receiver<TxidMonitor> that can only be entered with segwit only txs and which supports the check_payment API as implemented, and a Receiver<SomeOtherMonitorTypeStateName> that only supports asserting the status (presumably by monitoring transaction broadcast or for specific scripts depending on the wallet type)

Please open an issue for this too, IMO that should be done in a separate PR since this is a conceptual change that is non-trivial

• Should finalize_proposal validate that the signed PSBT actually has a valid signature?

This seems unnecessary since that would be the receiver validating its own singuature, if a wallet is not able to produce valid signatures that's not the payjoin crate's responsibility to enforce

[spacebear21]

Concept ACK. I think the approach can be simplified significantly by pre-computing async results and passing those as closures to the synchronous counterpart methods as explained below.

Also, 89310d8 could land separately in its own PR to expedite that fix and simplify review here.

[spacebear21]

I think we could pre-compute the result of the async callback and pass that to the synchronous check_broadcast_suitability.
we don't have to modify the latter at all, and check_broadcast_suitability_async becomes a two-liner. something like this:

    pub async fn check_broadcast_suitability_async<F>(
        &self,
        min_fee_rate: Option<FeeRate>,
        can_broadcast: impl Fn(&bitcoin::Transaction) -> F,
    ) -> Result<(), Error>
    where
        F: Future<Output = Result<bool, ImplementationError>>,
    {
        let can_broadcast = can_broadcast(&self.psbt.clone().extract_tx_unchecked_fee_rate())
            .await
            .map_err(Error::Implementation)?;
        self.check_broadcast_suitability(min_fee_rate, |_| Ok(can_broadcast))

    }

The same pattern could apply to all other transition methods afaict.

[spacebear21]

I don't think these docstrings really need updates, IMO it would be sufficient to simply add "Asynchronous counterpart to Self::check_broadcast_suitability" above the respective _async methods.