BIP78: Allow mixed inputs and clarify a few things

Kixunil commented at 5:48 pm on December 2, 2021: none

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; I think I remember seeing Samourai doing mixed inputs too.) Thus it makes sense to enable mixed inputs. To avoid compatibility issues a grace period is suggested.

Additional clarifications were made:

Disallow reciever removing output - such would break the reference implementation and seems better than changing the reference implementation.
Forbid sender changing TXID in special case where no input is added.
Confirm that amount is not mandatory.

We discussed this with @NicolasDorier in the past. (Finally found the time! :)) This may interest wallet developers: @nopara73 @Overtorment @rage-proof @RCasatta

BIP78: Allow mixed inputs and clarify a few things

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; I think I remember seeing
Samourai doing mixed inputs too.) Thus it makes sense to enable mixed
inputs. To avoid compatibility issues a grace period is suggested.

Additional clarifications were made:
* Disallow reciever removing output - such would break the reference
  implementation and seems better than changing the reference
  implementation.
* Forbid sender changing TXID in special case where no input is added.
* Confirm that `amount` is *not* mandatory.

d4e69ca02d

luke-jr added the label Proposed BIP modification on Dec 15, 2021

luke-jr commented at 9:26 pm on December 15, 2021: member

@NicolasDorier

DanGould commented at 1:16 am on July 30, 2022: none

LGTM. Even more software is supporting mixed spends or has plans to like bdk. Avoiding UIH 1 seems more important. Matching i/o sequence numbers probably needs a deeper look too.

Disallow reciever removing output - such would break the reference implementation and seems better than changing the reference implementation

What exactly would break in the reference implementation without this change?

DanGould commented at 7:26 pm on July 14, 2023: none

To stay completely backwards compatible without requiring a new version or grace period, a sender explicitly ignoring mixed input script check also use a request uri parameter like mixin=1 to explicitly allow it. A receiver having neither mixed input support nor an input of the same type could return the error. One supporting the update could return a Payjoin Proposal PSBT with mixed inputs.

edit: Bitcoin Core does occasionally mix input types, having done so even more often than when this PR was introduced https://github.com/bitcoin/bitcoin/pull/24584

in bip-0078.mediawiki:573 in d4e69ca02d

569@@ -564,8 +570,7 @@ public async Task<PSBT> RequestPayjoin(
570                 if (actualContribution > additionalFee)
571                     throw new PayjoinSenderException("The actual contribution is not only paying fee");
572                 // Make sure the actual contribution is only paying for fee incurred by additional inputs
573-                int additionalInputsCount = proposalGlobalTx.Inputs.Count - originalGlobalTx.Inputs.Count;
574-                if (actualContribution > originalFeeRate * GetVirtualSize(inputScriptType) * additionalInputsCount)
575+                if (actualContribution > originalFeeRate * additionalSize)

DanGould commented at 7:46 pm on July 17, 2023:

@NicolasDorier: I also don’t understand int additionalInputsCount = proposalGlobalTx.Inputs.Count - originalGlobalTx.Inputs.Count; why this line got removed

additionalSize is already calculcated, so we can check the same condition with that pre-calculation. This assumes additional contribution should only pay additional fee for one input as recommended by the spec.

NicolasDorier commented at 9:57 am on April 30, 2024:

additionalSize is set to be the virtual size of the last input. This isn’t proper accounting here.

DanGould commented at 3:53 pm on May 31, 2024:

@NicolasDorier I took your feedback and think I found a solution to allow the reference implementation to correctly include mixed script inputs without much fuss. This change focuses only on mixed script inputs.

What a sender really wants to ensure is that their additionalContribution paid for at least one additional input to break the common input ownership heuristic. The easiest way to check this in the reference is as follows:

0                if (additionalInputsCount <= 0)

The code already ensures the contribution doesn’t pay for anything other than additional fees. This simple change allows us to move forward by allowing mixed script inputs in the spec and leaving a more nuanced fee calculation, if so desired, to implementations themselves. The protocol allows for fine-grained fee negotiation that seems out of scope for this particular problem.

in bip-0078.mediawiki:276 in d4e69ca02d

274@@ -268,7 +275,6 @@ The sender should check the payjoin proposal before signing it to prevent a mali
275 *** Verify the PSBT input is finalized
276 *** Verify that <code>non_witness_utxo</code> or <code>witness_utxo</code> are filled in.

DanGould commented at 7:50 pm on July 17, 2023:

This check is antithetical to PSBT IMO and may be vestigial of a specific PSBT signer in C#. It makes a pain for other signers in LND and BDK which search for places they can sign by script. Perhaps it’s out of scope of this PR but I’d appreciate feedback either way.

in bip-0078.mediawiki:127 in d4e69ca02d

123 * Shuffle the order of inputs or outputs, the additional outputs or additional inputs must be inserted at a random index.
124 * Decrease the absolute fee of the original transaction.
125 
126+The payjoin proposal SHOULD NOT:
127+* Include mixed input types until May 2022. Mixed inputs were previously completely disallowed so this gives some grace period for senders to update.
128+

DanGould commented at 7:52 pm on July 17, 2023:

@NicolasDorier: I believe we don’t need any grace period or anything if the receiver add a mixed input, then the sender might not like it and thus not use Payjoin and fallback which is fine we should just notify libraries

Applies also to lines 106-108

Kixunil commented at 10:36 am on October 27, 2023:

Not sure where you’re quoting him from, anyway, that’s why there’s SHOULD NOT, and not MUST NOT. It’s simply to increase compatibility to prevent a bunch of PayJoins failing in the meantime.

in bip-0078.mediawiki:298 in d4e69ca02d

294@@ -289,6 +295,7 @@ Note:
295 * The sender must allow the receiver to add/remove or modify the receiver's own outputs. (if payment output substitution is disabled, the receiver's outputs must not be removed or decreased in value)
296 * The sender should allow the receiver to not add any inputs. This is useful for the receiver to change the paymout output scriptPubKey type.
297 * If no input have been added, the sender's wallet implementation should accept the payjoin proposal, but not mark the transaction as an actual payjoin in the user interface.
298+* If no input have been added, the sender's wallet MUST NOT perform changes that would change transaction ID. Such changes would be accepted by the Bitcoin network in this special case but may invalidate smart contracts the receiver participates in. (E.g. Lightning Network channel opening)

DanGould commented at 7:56 pm on July 17, 2023:

@NicolasDorier : “the sender’s wallet MUST NOT perform changes that would change transaction ID” I disagree saying MUST NOT isn’t useful it can be done we can’t prevent this

I see Nicolas’s point but I think asking specs to behave a certain way might make expectations for accounting a heck of a lot easier, though I’m not sure I understand under which circumstances a receiver-signed PSBT could be modified to change the txid. @Kixunil may you please elaborate?

DanGould commented at 4:44 pm on July 31, 2023:

I agree with @NicolasDorier’s concern. If any inputs to the transaction are not segwit then the txid could be malleated. @Kixunil do channel opening txs commit to a TXID? Do they accept non-segwit inputs? If yes to both, this requirement might make sense. If they’re segwit only then the payjoin receiver can check for that before trying to open a channel and it doesn’t need to be part of BIP 78 spec.

Kixunil commented at 10:40 am on October 27, 2023:

Firstly, non-segwit is out of scope since things like LND already check for that.

If all the inputs belong to the sender (so it’s just batching) then the sender can change them. We cannot cryptographically prevent that but so we can’t prevent sending to a wrong address. I propose that changing TXID (by changing the inputs and sending to the replaced address) is treated as sending to a wrong address.

(Side note: BTC on chain really needs signing of addresses/requests just like LN invoices do.)

Note also that this effectively forbids replace-by-fee.

DanGould commented at 4:19 pm on June 10, 2024:

I think I understand. If a receiver chooses not to contribute input, then the sender could change inputs and pay to a substituted output that requires a specific txid. This should be prevented if possible.

Kixunil commented at 10:11 am on June 17, 2024:

Exactly. I wanted to discuss this with Nicolas at BTC Prague but I didn’t even manage to say hi to him. :sob: Too much stuff going on.

Now I’ve realized that it probably cannot be fully prevented easily. A wallet can remember a transaction was a payjoin but if the data is lost and the wallet is recovered from seed it doesn’t know if it’s safe to RBF. It could use the RBF flag in transaction to signal that but that creates footprint in the chain. So unless I’m missing something loptos/nolooking is effectively broken and can lead to loss of funds in this edge case. This sucks a lot since being able to initialize an LN node with just one transaction is very helpful.

ghost commented at 11:15 pm on August 2, 2023: none

Samourai doing mixed inputs too

It makes it easier to identify change address in samourai which is bad for privacy. Not sure what are the benefits of allowing mixed inputs in payjoin.

TonyGiorgio commented at 4:23 am on October 27, 2023: none

I’m unsure of the implications by allowing mixed inputs and whether that could hurt privacy if so, but to chime in a little. Mutiny is a taproot address only wallet so interacting with current payjoin server implementations that aren’t using taproot either might reduce the compatibility. Not sure how many have upgraded by now.

Kixunil commented at 10:33 am on October 27, 2023: none

@TonyGiorgio when this BIP was first created the authors incorrectly thought that there are no wallets that mix inputs. If there truly weren’t then mixing would be a clear giveaway that the transaction is PayJoin. However there actually are such wallets and thus mixing inputs doesn’t reveal it’s a PayJoin, it actually reveals it’s not a PayJoin - thus it leaks data. So that incorrect restriction has to go.

As for Mutiny or any similar wallets, mixing inputs will reveal that the wallet is definitely not Mutiny but, after removing the restriction, doesn’t reveal whether it’s a PayJoin or not. If Mutiny implements mixed inputs in the future it’ll get better.

DanGould commented at 7:33 pm on March 6, 2024: none

However there actually are such wallets and thus mixing inputs doesn’t reveal it’s a PayJoin, it actually reveals it’s not a PayJoin - thus it leaks data. So that incorrect restriction has to go.

Kixunil’s point is the main privacy consideration about this change.

This unintuitive result is the same as the consequence of lexicographical ordering. If only some wallets can produce a given transaction topology, even if that result is intended to make it uniform for the sake of privacy, it creates an identifiable fingerprint. For example, Trezor always orders outputs according to BIP-69 Lexicographical ordering, allowing analysts to conclude that any transaction not following lexicographical ordering must not have come from Trezor. This in fact REDUCES the potential ambiguity a transaction might have. See Ishaana’s article on wallet fingerprinting

murchandamus commented at 8:13 pm on April 26, 2024: contributor

It seems to me that these changes would need sign-off from the original BIP author. Pinging @NicolasDorier for review.

in bip-0078.mediawiki:107 in d4e69ca02d

102@@ -103,6 +103,9 @@ The original PSBT MUST:
103 * Not include fields unneeded for the receiver such as global xpubs or keypath information.
104 * Be broadcastable.
105 
106+The original PSBT SHOULD NOT:
107+* Include mixed input types until May 2022. Mixed inputs were previously completely disallowed so this gives some grace period for recivers to update.

NicolasDorier commented at 9:43 am on April 30, 2024:

recivers is a typo

NicolasDorier commented at 10:04 am on April 30, 2024: contributor

There is one typo recivers, and there is a dubious way to calculate that the actual contribution is only paying for fee incurred by additional inputs.

additionalSize is set arbitrarily be the virtual size of the last input added by the receiver. If the receiver added mixed inputs, then this shouldn’t be this. Either it should be properly calculated, or we should require the receiver to not use mixed inputs. I think that the simplification in implementation is well worth the additional restriction on the receiver. @DanGould you can ping me on DM or btcpay chat to get that moving.

in bip-0078.mediawiki:531 in d4e69ca02d

527@@ -520,9 +528,7 @@ public async Task<PSBT> RequestPayjoin(
528             if (proposedPSBTInput.NonWitnessUtxo == null && proposedPSBTInput.WitnessUtxo == null)
529                 throw new PayjoinSenderException("The receiver did not specify non_witness_utxo or witness_utxo for one of their inputs");
530             sequences.Add(proposedTxIn.Sequence);
531-            // Verify that the payjoin proposal did not introduced mixed inputs' type.
532-            if (inputScriptType != proposedPSBTInput.GetInputScriptPubKeyType())
533-                throw new PayjoinSenderException("Mixed input type detected in the proposal");
534+            additionalSize = GetVirtualSize(proposedPSBTInput.GetInputScriptPubKeyType())

DanGould commented at 9:28 pm on May 1, 2024:

I agree with @NicolasDorier’s that this calculation is incorrect. The additionalSize should be the sum of all inputs added to the Original PSBT, not just the last one that gets checked.

0            additionalSize += GetVirtualSize(proposedPSBTInput.GetInputScriptPubKeyType())

DanGould commented at 10:50 pm on May 1, 2024: none

@NicolasDorier I’ve DMd you on BTCPay’s mattermost as requested.

The additionalfeecontribution seems to be intended to incentivize privacy-preserving behavior. If a payjoin spent the additionalfeecontribution without contributing at least one additional input, it would be fair for the sender to be unwilling to sign. Rather than calculate the marginal cost of each additional input and output, a notoriously complex feat, just calculate the fee of the Payjoin and subtract the fee of the Original transaction plus additionalfeecontribution if the receiver contributed an input. Ensure that Payjoin maintains the sender’s specified minfeerate, otherwise the min relay fee. If there’s no receiver input, check that the receiver did not spend spend the additionalfeecontribution.

The original spec actualContribution calculation seems to ignore the complexity of having a receiver pay for fees on additional outputs if they had substituted the original payment with multiple outputs. The fee calculation outlined above covers this case as well.

Perhaps multiple-output substitution was intended to be prohibited for the sake of simplicity, or otherwise overlooked, but the sender checklist is somewhat ambiguous as to allowing it or not. Yes, the spec says singular ‘“the” payment output’, but sender implementations I’ve come across (BlueWallet, Sparrow, Payjoin-CLI from memory) allow multiple-output substitution.

One could argue everyone has privacy to gain and be preserved from multi-input transactions, and the receiver already has incentive to add them via lower marginal input cost for consolidation or multi-output transaction cut-through without additional contribution, but additionalfeecontribution is already part of the spec and this mixed input clarification is not meant to have that discussion. It’s just to allow mixed input because that’s prevents a privacy-breaking fingerprint.

NicolasDorier commented at 1:20 pm on May 2, 2024: contributor

@DanGould keep it focus for this specific PR. This is difficult enough to review. @Kixunil Can you also fix another thing?

There is references to an output variable. This should actually be originalOutput.

murchandamus added the label Pending acceptance on May 8, 2024

murchandamus added the label PR Author action required on May 8, 2024

Zavalynech approved

5twelve approved

BIP78: Allow mixed inputs and clarify a few things #1244