Rethink fee estimation to ensure accuracy and prevent overspending

[DanGould]

rust-payjoin/payjoin/src/receive/mod.rs

Lines 976 to 978 in 59c3d27

	// HACK: The script pubkey in the original test vector is a nested p2sh witness
	// script, which is not correctly supported in our current weight calculations.
	// To get around this limitation, this test uses a native segwit script instead.

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 #332 (comment)

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 #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 #332 (comment)

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

[spacebear21]

I wonder if we could estimate the receiver input weights/fees by first signing the PSBT in finalize_psbt, then applying fees (which we can now do because we know the redeem script from the signature), then clearing the dummy signature and re-signing to get the final signatures.

[spacebear21]

Closed by #367