Accomodate Updated BIP 78 Spec #505
Conversation
BIP 78 (Payjoin v1) was updated to remain in compliance with BIP 174 (PSBT v0). See: bitcoin/bips#1396 See also: payjoin#480
Pull Request Test Coverage Report for Build 12932827292Details
💛 - Coveralls |
There was a problem hiding this comment.
the BIP says "Our recommendation for maxadditionalfeecontribution= is originalPSBTFeeRate * 110." and our actual use is let recommended_additional_fee = min_fee_rate * input_weight; where input_weight is 110 only where mixed inputs appear.
AFAICT our implementation uses "cheapest default if mixed input types" which is 58 for P2TR, not 110. Actually it's not clear to me why the BIP recommends 110, it seems to be a compromise for covering most common input types except P2PKH?
I did not remove the expected_input_weight function for a blanket 110 recommendation, which I believe is in line with actual incentives to use a matching input. but I could go either way.
I agree with leaving our implementation as is. It enables savings for the sender if they use a cheaper input type, and we went to great lengths to get precise input weight calculations so they are well-tested. The only drawback is expected_input_weight could fail if the input is not recognized, but I think that's fine because the receiver needs to be able to calculate sender input weights in additional_input_weight anyway.
| @@ -128,41 +128,6 @@ mod integration { | |||
| ); | |||
| Ok(()) | |||
| } | |||
|
|
|||
| #[test] | |||
| fn disallow_mixed_input_scripts() -> Result<(), BoxError> { | |||
There was a problem hiding this comment.
There is a v2_to_v2_mixed_input_script_types test that checks for allowing mixed input script types. Maybe we should move it here as allow_mixed_input_scripts since it's now shared logic and v1 tests are shorter to write.
DanGould
force-pushed
the
update-bip78
branch
from
1b4478b to
8f627c6
Compare
|
I knew there was some send-side check I was missing. Fixed that and moved the test too. Thanks for the thorough review (as always, King behavior). |
DanGould
force-pushed
the
update-bip78
branch
from
8f627c6 to
ab12170
Compare
BIP 78 was updated to allow mixed inputs. > Disallowing mixed inputs was based on incorrect assumption that no > wallet supports mixed inputs and thus mixed inputs imply PayJoin. > However there are at least three wallets supporting mixed inputs. > (Confirmed: Bitcoin Core, LND, Coinomi) Thus it makes sense to enable > mixed inputs to avoid a payjoin-specific fingerptint. To avoid > compatibility issues a grace period is suggested. See: bitcoin/bips#1605 See also: payjoin#480
See #480
InternalInputContributionErrorhas only one error variant as a result of this, which means it's a smell to clean up in #403One thing I note is that the BIP says "Our recommendation for
maxadditionalfeecontribution=isoriginalPSBTFeeRate * 110." and our actual use islet recommended_additional_fee = min_fee_rate * input_weight;where input_weight is 110 only where mixed inputs appear. I did not remove theexpected_input_weightfunction for a blanket 110 recommendation, which I believe is in line with actual incentives to use a matching input. but I could go either way.