Refactor output substitution

[spacebear21]

As per discussion in https://github.com/payjoin/rust-payjoin/pull/239#issuecomment-2089097964, never allow output substitution if disable_output_substitution is true.

Consider whether there should be a custom SubstituteOutputError type, similar to SelectionError.

[DanGould]

This refactor makes sense to me to prevent a consumer from shooting themselves in the foot. It throws an error if output substitution is disabled when try_substituting_output_address is called.

I have one suggestion, that we allow output substitution for any old script instead of just known address types.

If you're up for it, consider what an interface that enables transaction cut-through would look like. To replace an output with transaction cut-through outputs, a list of outputs and their amounts would need to be passed and validated such that they spend no more than the output they replace minus additional fee costs. It would be easier for downstream to handle a single API change after deliberation here rather than multiple changes. Getting transaction construction right is at the core of the utility PDK can provide. What do you think?

Output substitution is a similar interface to input contribution methods 1 2 just because it modifies the PSBt. Though I think input contribution is a bit more difficult since PSBTv1 separates Input information in both the unsigned_tx and input maps.

FYI, within the context of payjoin-cli, output substitution exists in that place because the program prints only a single bip21 with a static address at the beginning of the run. If it were used on a long-running v1 server then output substitution prevents that address from being reused. In the context v2-only payjoin-cli, it would make less sense.

It's a critical part of the interface to get right whether it ends up being used for simple output substitution, [transaction cut-through], or dropped in payjoin-cli.

[spacebear21]

Agreed on allowing any output script type.

Supporting cut-through would be amazing. Should we always require an output being replaced though? A receiver might want to just add new outputs for another purpose, and should be able to as long as they also contributed a large enough input to cover the total output amounts + fees.

If this is true, then substitute_outputs might accept a list of outputs to remove and a list of outputs to add, both of which can be empty. The resulting outputs would then be validated against the inputs. Does that make sense?

[DanGould]

I'm not sure what the best API is yet but I think a simple one would just completely replace the Original PSBT of Output(s) paying the receiver. Then it's just a single method to maintain.

    pub fn substitute_outputs(&mut self, receiver_outputs: impl IntoIterator<Script>) {
        // ...
    }

I'm trying to imagine a stable PDK 1.0 receiver interface and am taking a stab at getting your opinion on how we might get this blurry vision into focus.

An interface with complete outputs would allow us to make a state machine to guide receiver implementations through one step at a time. Substitution could be a mandatory step in the state machine with this interface as an Option; to skip substitution, just pass None.

impl WantsOutputs {
  // ...
  fn try_substitute_outputs(self, outputs: Option<impl IntoIterator<Script>> )
  -> Result<WantsInputs>
}

I haven't thought through how the receiver might pay for fees and make change for inputs in excess of what the sender is willing to pay in additional contribution out of their change output. A receiver might also need a change output that can be reduced by subsequent fees depending on the input, like in a typical UI transaction construction interface. If lots of inputs and outputs get added then the sender's additional fee contribution is likely insufficient to cover all additional costs, but the receiver has incentive to add them since the marginal cost for inputs/outputs is still less batched versus in their own transaction.

A state machine with a completely defined output list can also have a simple input contribution interface follow at the next step. Right now ProvisionalProposal lets you add and remove inputs and outputs but it's not flexible enough for a receiver to add their own fees. If all the outputs were defined, a new typestate could select inputs to at least cover their costs and prevent unnecessary input heuristic if possible. Perhaps it could make have other modes like minimizing fees (in high fee environments) or maximum consolidation (in low fee environments) easy too.

impl WantsInputs {
  // ...
  fn try_preserving_privacy(self, inputs: Option<impl IntoIterator<psbt_v2::v2::Input>) -> Result<WantsInputs>

As a side note, Using the PSBTv2 Input data type lets us combine the two separate witness/non_witness contribution methods into one.

There's a lot to think about and a lot that can be simplified in the receiver feature API compared to what we have. Whatever step this change takes should look at the long term context. We're aiming at to get to a stable, capable, and delightful version 1.0

[spacebear21]

The error nesting doesn't make a whole lot of sense.

Agreed. I haven't come up with a satisfying solution so far, that would encompass any error type thrown by generate_script, and likely won't before tomorrow's release. I can follow-up in a separate PR/release if you'd like to merge this one as is, and also investigate the other spots that overload receive::Error.