Rethink fee estimation to ensure accuracy and prevent overspending

[DanGould]

https://github.com/payjoin/rust-payjoin/blob/59c3d271e278dafb0a41bcacbde8b3b0d4ec2738/payjoin/src/receive/mod.rs#L976-L978

too costly in engineering to correctly calculate p2sh witness script? Not supported by rust-bitcoin?

The test is excellent, but I wonder if improper fee estimation is going to bite us elsewhere.

_Originally posted by @DanGould in https://github.com/payjoin/rust-payjoin/pull/332#discussion_r1750617256_

Full disclosure: I haven't put too much thought into this yet because my priority was to get a working test and it seemed like a long detour.

My initial hunch was that this issue might fix itself once we implement https://github.com/payjoin/rust-payjoin/issues/353, but now I'm thinking it won't. The problem is that it's impossible to know that a p2sh script pubkey is nested segwit (or generally to know anything about the weight of the corresponding input), without knowing the actual script from final_script_sig for that input. By design, final_script_sig gets cleared beforehand in https://github.com/payjoin/rust-payjoin/blob/master/payjoin/src/receive/mod.rs#L587 for sender inputs, and the receiver has yet to sign their inputs at this point. So final_script_sig is never present when we estimate and apply receiver fees.

I don't know if there's a best practice for this. Maybe we can assume that any p2sh input is nested segwit and make fee estimates based on that? If the script turns out to be something else and min_feerate is not met the sender should reject the payjoin psbt anyway.

It looks like an issue that we need to address but is probably scope creep for this particular PR.

_Originally posted by @spacebear21 in https://github.com/payjoin/rust-payjoin/pull/332#discussion_r1755734294_

[DanGould]

Couldn't the field clearing and apply_fee order be reversed to fix this problem?

[spacebear21]

Couldn't the field clearing and apply_fee order be reversed to fix this problem?

That's not a bad idea, with some caveats:

• We'd have to ensure the weight estimator uses a sender input because receiver inputs still wouldn't be signed at this stage. Currently it just uses the first input which would randomly fail sometimes.
• For accurate fee estimation it would also require that all receiver inputs have the same type as the selected sender input. That's enforced in the current BIP78 spec and it's an assumption we already make, but there's an ongoing conversation about removing this requirement to better support batching.

[DanGould]

We should be able to estimate the sender's P2SH weight with InputWightPrediction::new, however receiver inputs may not yet have the witness element length (because they're not signed before apply_fee so that would need to be estimated in the upper limit or otherwise saved for another iteration.