Modify try_preserving_privacy function signature

[spacebear21]

Addresses #32. Instead of having try_preserving_privacy modify the PSBT directly as suggested in that issue, we keep the "coin selection" and "input contribution" steps separate but simplify the function interface so that they're easier to chain and reason about.

try_preserving_privacy now takes a InputPair iterator instead of a bespoke (Amount, Outpoint) iterator which is lossy and unwieldy. It also returns a single InputPair instead of just an OutPoint.

This is the same type that contribute_inputs takes as a parameter.

[DanGould]

Matching try_preserving_privacy I/O to what WantsInputs expects does make for a much cleaner integration.

It seems like we should introduce a public InputPair type for two reasons

1. We can validate InputPairs' PrevTxOuts on construction
2. Downstream UniFFI bindings don't support Tuples in the interface, so we'd have to wrap them there anyhow. More research needs to be done to find out if we can just expose components of the existing InputPair type as public if my PrevTxOut validation actually works in this flow, and if doing so is a better alternative to just using psbt::v2::Input Map element types.

[spacebear21]

Pushed a new commit which introduces a public InputPair type and constructor.

This iteration does validation on the InputPair constructor, but we can't always assume that an InternalInputPair contains previous txout information. For example, when the sender checks the proposal PSBT inputs they expect witness_utxo and non_witness_utxo to be empty. So previous_txout() still returns a Result.

I'm exploring a route where we split up the internal type in two: InternalInputPair and ValidatedInputPair. That would allow us to validate input pairs on construction, while keeping the option to not validate e.g. when getting psbt.input_pairs(). It adds pretty significant complexity though...

[DanGould]

Complexity seems like the enemy here, so if we have some perf loss to trade for ergonomics, let's trade for ergonomics. The questions to ask for almost any decision in this crate is "Does this make payjoin easier to adopt?"

Removing the PrevTxOut error, Adding InputPair make it easier, leaving InternalInputPair made it possible not to regress on the existing performance.

In that context cloning isn't the end of the world since this is taking up our cycles.

This iteration does validation on the InputPair constructor, but we can't always assume that an InternalInputPair contains previous txout information. For example, when the sender checks the proposal PSBT inputs they expect witness_utxo and non_witness_utxo to be empty. So previous_txout() still returns a Result.

I don't understand why a validated InputPair::previous_txout() needs to return a Result if the constructor returns a result of either 1) a validated pair or 2) the relevant error. It seems like that function should expect when it calls the inner previous_txout, since the input is expected to be validated

[DanGould]

I've pushed a version which implements an infallible InputPair::previous_txout as well as helper functions. It introduces some visibility warnings that needs to be addressed, and makes some TODO comments in the InputPair constructor for validation that needs to be done on the passed input in order to prepare it for fee estimation.

I think it makes more sense to implement this function in the receive module than the psbt module since it's actually being used inside that state machine, and the InternalInputType is used for the psbt module operations. This would also let us use the names psbt::InputType<'a> and pub receive::InputType, but that might be even more confusing. I'm not yet sure if that affects whether or not we want the InputPair From impls or not.

[spacebear21]

Building on top of your commit and suggestions, I added validation on AddressType and redeem_script for P2SH inputs, and moved InputPair to the receive module.

[DanGould]

In addition to removing clippy warnings on intermediate commits (select_first_candidate in receive/mod.rs now uses ok_or in stead of map_or(|...| Ok(...)), and unused inputs were removed), I made a public PsbtInputError for use in the receive module and made the now renamed psbt::InternalPsbtInputError and its internal variant errors pub(crate) so that we can wrap the public error at the bindings layer, and to follow the same pattern we use in the rest of the crate until our error variants are stable.

tACK bd15da7 if it's ok with you @spacebear21

[spacebear21]

ACK 7956acf