-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BIP78: Allow mixed inputs and clarify a few things #1244
Conversation
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.
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.
What exactly would break in the reference implementation without this change? |
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 edit: Bitcoin Core does occasionally mix input types, having done so even more often than when this PR was introduced bitcoin/bitcoin#24584 |
@@ -564,8 +570,7 @@ public async Task<PSBT> RequestPayjoin( | |||
if (actualContribution > additionalFee) | |||
throw new PayjoinSenderException("The actual contribution is not only paying fee"); | |||
// Make sure the actual contribution is only paying for fee incurred by additional inputs | |||
int additionalInputsCount = proposalGlobalTx.Inputs.Count - originalGlobalTx.Inputs.Count; | |||
if (actualContribution > originalFeeRate * GetVirtualSize(inputScriptType) * additionalInputsCount) | |||
if (actualContribution > originalFeeRate * additionalSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
additionalSize
is set to be the virtual size of the last input. This isn't proper accounting here.
@@ -268,7 +275,6 @@ The sender should check the payjoin proposal before signing it to prevent a mali | |||
*** Verify the PSBT input is finalized | |||
*** Verify that <code>non_witness_utxo</code> or <code>witness_utxo</code> are filled in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
The payjoin proposal SHOULD NOT: | ||
* Include mixed input types until May 2022. Mixed inputs were previously completely disallowed so this gives some grace period for senders to update. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -289,6 +295,7 @@ Note: | |||
* 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) | |||
* The sender should allow the receiver to not add any inputs. This is useful for the receiver to change the paymout output scriptPubKey type. | |||
* 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. | |||
* 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. I wanted to discuss this with Nicolas at BTC Prague but I didn't even manage to say hi to him. 😭 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.
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. |
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. |
@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. |
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 |
It seems to me that these changes would need sign-off from the original BIP author. Pinging @NicolasDorier for review. |
@@ -103,6 +103,9 @@ The original PSBT MUST: | |||
* Not include fields unneeded for the receiver such as global xpubs or keypath information. | |||
* Be broadcastable. | |||
|
|||
The original PSBT SHOULD NOT: | |||
* Include mixed input types until May 2022. Mixed inputs were previously completely disallowed so this gives some grace period for recivers to update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recivers
is a typo
There is one typo
@DanGould you can ping me on DM or btcpay chat to get that moving. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
// Verify that the payjoin proposal did not introduced mixed inputs' type. | ||
if (inputScriptType != proposedPSBTInput.GetInputScriptPubKeyType()) | ||
throw new PayjoinSenderException("Mixed input type detected in the proposal"); | ||
additionalSize = GetVirtualSize(proposedPSBTInput.GetInputScriptPubKeyType()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
additionalSize = GetVirtualSize(proposedPSBTInput.GetInputScriptPubKeyType()) | |
additionalSize += GetVirtualSize(proposedPSBTInput.GetInputScriptPubKeyType()) |
@@ -564,8 +570,7 @@ public async Task<PSBT> RequestPayjoin( | |||
if (actualContribution > additionalFee) | |||
throw new PayjoinSenderException("The actual contribution is not only paying fee"); | |||
// Make sure the actual contribution is only paying for fee incurred by additional inputs | |||
int additionalInputsCount = proposalGlobalTx.Inputs.Count - originalGlobalTx.Inputs.Count; | |||
if (actualContribution > originalFeeRate * GetVirtualSize(inputScriptType) * additionalInputsCount) | |||
if (actualContribution > originalFeeRate * additionalSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
if (actualContribution > originalFeeRate * additionalSize) | |
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.
Completed by the merge of #1605. |
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:
implementation and seems better than changing the reference
implementation.
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