Refactor output substitution

[spacebear21]

As per discussion in #239 (comment), 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

[DanGould]

tACK this change is working as intended but I found a couple issues that ought to be addressed in follow ups

• V2 requests are being sent with v=1 default send parameter and thus defaulting to disable_output_substitution = false. v1 requests over v2 directories are in the unsecured payment server category of BIP 78, so any v1 request has output substitution disabled. V2 requests are mistakenly being interpreted as v1 requests wrt Params.
• The error nesting doesn't make a whole lot of sense. The warning produces "[2024-06-26T00:03:44Z WARN payjoin_cli::app::v2] Failed to substitute output: Internal Server Error: Output substitution is disabled." Internal Server error is intended to be returned as a response, it probably doesn't make sense here inside the "Failed to substitute output" context. I think crate::receive::Error is being abused in many more spots other than this commit and the receive crate needs an error audit. If there's an easy way to make the error of this addition nesting make sense It'd be nice to get pretty before merge but it's not a show stopper.

I've tested this and am prepared to merge if you are.

[DanGould]

Are you sure that consumers won't want to check to avoid output substitution if they knew in advance it is disabled? Seems like the only way to check now is by actually calling try_substitute_receiver_output. Makes sense to me but I want to make this decision consciously.

[spacebear21]

Good point, I wasn't thinking about consumers other than payjoin-cli when I removed it but I think it would be useful to leave it exposed.

[DanGould]

V1 receivers are long running hosted servers and only output a single bip21 per run, it made sense to me to have them substitute outputs by default.

V2 receivers on the other hand are always session based and serve only one sender. It makes less sense to me for them to try to substitute outputs. I understand that your change is unrelated but imo v2 receivers should be changed not to substitute addresses by default in a follow up.

[spacebear21]

Removed in v2 receiver

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

[DanGould]

utACK

[DanGould]

ultranit: two lines doesn't save length in this instance so this is borderline unnecessary assignment.