Use FnMut closures for receiver typestate checks

[benalleng]

In our liana wallet integration we are encountering some issues due to how the liana devs have setup their database, namely that their db always references a mutable self even on read calls within the db so we need to have some mutability when accessing things like an address or outpoint from the db. This was not possible when our closures were limited to non-mutable.

I considered using FnOnce but we have some map methods that prevent us from doing that as map have the possiblity of forcing the closure being called more than once which FnOnce is limited to.

N.B. I did not make all of the closures for the typestates mutable but perhaps we should think about their consistency for integration devs and to limit rust-payjoin from deciding how devs should implement these type state checks.

[coveralls]

Pull Request Test Coverage Report for Build 16447460970

Details

• 100 of 100 (100.0%) changed or added relevant lines in 6 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 85.805%

---

Totals
Change from base Build 16427048792:	0.1%
Covered Lines:	7804
Relevant Lines:	9095

---

💛 - Coveralls

[nothingmuch]

This seems correct, both conceptually and in terms of the code.

These callbacks are how we keep the payjoin crate code in some sense pure (strictly speaking it's not, even ignoring the callbacks, due to use of time::now() and OsRng...)

Or docs compel these callbacks to maintain state, and FnMut represents this more faithfully.

Since FnMut is a supertrait of Fn, this shouldn't break existing code.

However the mut arguments in v1 vs lack of that in v2 seems like a bug

[nothingmuch]

IIUC invoking an FnMut requires the reference to the function be mut too. impl is shorthand for generics, so this code seems to be monomorphized over Fn + Copy in our tests, and I believe it implicitly copies the closure reference at like 568.

I think this means that passing real FnMuts (i.e. one lacking Copy) via the v2 typestate machine would not work with the PR as written, and the fact that it works is a side effect of our test closures all being Fns?

Suggested change

	is_owned: impl FnMut(&Script) -> Result<bool, ImplementationError>,
	mut is_owned: impl FnMut(&Script) -> Result<bool, ImplementationError>,

[benalleng]

So i though that at first but when trying it I get an error about it not needing to be mutable.

error: variable does not need to be mutable
   --> payjoin/src/core/receive/v2/mod.rs:566:9
    |
566 |         mut is_owned: impl FnMut(&Script) -> Result<bool, ImplementationError>,
    |         ----^^^^^^^^
    |         |
    |         help: remove this `mut`
    |

However if I keep it as a non-mutable closure in v2 the upstream code in liana complains when it is still pointing to a non-mutable closure as it acts as a pass-through here down to the actual logic in the v1 receiver

[nothingmuch]

So i though that at first but when trying it I get an error about it not needing to be mutable.

hmm, this is confusing =/

However if I keep it as a non-mutable closure in v2 the upstream code in liana complains when it is still pointing to a non-mutable closure as it acts as a pass-through here down to the actual logic in the v1 receiver

could you add a minimal test case of passing FnMut closures to both v1 and v2?

[nothingmuch]

Suggested change

	is_known: impl FnMut(&OutPoint) -> Result<bool, ImplementationError>,
	mut is_known: impl FnMut(&OutPoint) -> Result<bool, ImplementationError>,

[nothingmuch]

Suggested change

	is_receiver_output: impl FnMut(&Script) -> Result<bool, ImplementationError>,
	mut is_receiver_output: impl FnMut(&Script) -> Result<bool, ImplementationError>,

[nothingmuch]

Hmm, I believe this is what we actually want in both v1 and v2:

Suggested change

	is_receiver_output: impl FnMut(&Script) -> Result<bool, ImplementationError>,
	is_receiver_output: &mut impl FnMut(&Script) -> Result<bool, ImplementationError>,

having a non reference parameter takes ownership of the closure anyway, so the mut modifier in the function signature makes no real difference, that's why we can remove it in the v1 code as long as we do let mut is_owned = is_owned within, since the closure itself is moved.

i believe moving is too rigid a requirement, for the same reason that FnOnce is.

[arminsabouri]

Either way we want this to be consistent right? I dont see any reason why we would move in v1 but not in v2. I don't think thats whats being suggested here but wanted to double check.

i believe moving is too rigid a requirement, for the same reason that FnOnce is.

I agree. At the same time there doesnt seem like there is a case (in Liana at least) where we would want to re-use the same closure. &mut does reduce some cloning boilerplate. i.e having to clone db connection handlers. Its possible that in some application cloning the things you need in the closure is a non starter. In general I'd prefer to be less restrictive with ownership here unless we have a good reason.

[Johnosezele]

cACK. This change makes a lot of sense. Allowing FnMut closures provides much-needed flexibility for integrations where mutable access is required for things like database lookups. The implementation looks clean and the added tests are great.

[nothingmuch]

ACK, but I would prefer if @arminsabouri also reviewed since I don't feel super confident about my suggestion to use &mut instead of consuming the closure

[arminsabouri]

I considered using FnOnce but we have some map methods that prevent us from doing that as map have the possiblity of forcing the closure being called more than once which FnOnce is limited to.

@benalleng Can you link which state transition this is?

[benalleng]

each method has a map closure that cannot be FnOnce because it can be called more than once, I am not sure about the feasiblity of adding clone for these either.

in check_inputs_no_owned it is find_map

pub fn check_inputs_not_owned(
        self,
        is_owned: impl FnOnce(&Script) -> Result<bool, ImplementationError>,
    ) -> Result<MaybeInputsSeen, ReplyableError> {
        ...
            /// find_map prevents us from using FnOnce
            .find_map(|script| match is_owned(&script) {
                Ok(false) => None,
                Ok(true) => Some(InternalPayloadError::InputOwned(script).into()),
                Err(e) => Some(ReplyableError::Implementation(e)),
            })
        {
        ...
    }

in check_inputs_not_seen_before it is try_for_each`

    pub fn check_no_inputs_seen_before(
        self,
        is_known: impl FnMut(&OutPoint) -> Result<bool, ImplementationError>,
    ) -> Result<OutputsUnknown, ReplyableError> {
        ...
        /// this try_for_each prevents us from using FnOnce
        self.psbt.input_pairs().try_for_each(|input| {
            match is_known(&input.txin.previous_output) {
                Ok(false) => Ok::<(), ReplyableError>(()),
                Ok(true) =>  {
                    log::warn!("Request contains an input we've seen before: {}. Preventing possible probing attack.", input.txin.previous_output);
                    Err(InternalPayloadError::InputSeen(input.txin.previous_output))?
                },
                Err(e) => Err(ReplyableError::Implementation(e))?,
            }
        })?;
    ...
    }

and identify_receiver_outputs it is filter_map

    pub fn identify_receiver_outputs(
        self,
        is_receiver_output: impl FnMut(&Script) -> Result<bool, ImplementationError>,
    ) -> Result<WantsOutputs, ReplyableError> {
        ...
            /// this prevents us from using FnOnce
            .filter_map(|(vout, txo)| match is_receiver_output(&txo.script_pubkey) {
                Ok(true) => Some(Ok(vout)),
                Ok(false) => None,
                Err(e) => Some(Err(e)),
            })
            .collect::<Result<Vec<_>, _>>()
            .map_err(ReplyableError::Implementation)?;

        ...
    }

[DanGould]

Is demanding interior mutability for Liana's db operations too much? @nothingmuch That just seems like best practice in cases like this for a clean interface.

[arminsabouri]

Is demanding interior mutability for Liana's db operations too much? @nothingmuch That just seems like best practice in cases like this for a clean interface.

Its just boilerplate. It would probably work fine but I don't think Fn -> FnMut sacrificing much in terms of API hygine.

[benalleng]

I added a commit to force the v1 receiver to also use a &mut ref to the closure in a5713d3. This now keeps the closures consistent across the versions and forces the remaining untouched state checks in multiparty to now also expect a mutable reference.

[arminsabouri]

utACK a5713d3
From what I gather both using FnMut and taking a mutable refrence of the closure does reduce application side boilerplate at little cost to the API, our internal code or other integrations.

Example use

[nothingmuch]

Its just boilerplate. It would probably work fine but I don't think Fn -> FnMut sacrificing much in terms of API hygine.

Definitely it's possible to demand interior mutability, but I tend to agree, if it's not required then an Fn can be given where an FnMut is required, that relaxes what our API accepts.

The other change, going from a consumed closure to a &mut is also an improvement in this regard, but I can see the case being made for accepting an &Fn instead of &mut FnMut if we really want to require interior mutability whenever state changes need to happen.

[nothingmuch]

ACK.

Unless anyone objects I will open a followup issue for simplifying check_inputs_not_owned check_no_inputs_seen_before so that it's not implicitly recording in some external state that an input was seen until the state transition is in durable storage, in principle we can handle this logic entirely within the session event log information given the ability to look at events from all sessions, but that kind of change would be somewhat complex due to being a potential performance bottleneck if every input check ends up replaying the events of all session.

That said I think we should only revisit when we're ready to do more intelligent coin selection choices as well, allowing repeated sender inputs, both in regards to the same payment request or multiple ones, so long as the choice of receiver's inputs in the response is deterministic, so that the state machine only rejects a payjoin when/if that's no longer possible.

[arminsabouri]

ACK.

Unless anyone objects I will open a followup issue for simplifying check_inputs_not_owned so that it's not implicitly recording in some external state that an input was seen until the state transition is in durable storage, in principle we can handle this logic entirely within the session event log information given the ability to look at events from all sessions, but that kind of change would be somewhat complex due to being a potential performance bottleneck if every input check ends up replaying the events of all session.

That said I think we should only revisit when we're ready to do more intelligent coin selection choices as well, allowing repeated sender inputs, both in regards to the same payment request or multiple ones, so long as the choice of receiver's inputs in the response is deterministic, so that the state machine only rejects a payjoin when/if that's no longer possible.

Ack this general suggestion.