Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions bip-0078.mediawiki
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recivers is a typo


The original PSBT MAY:
* Have outputs unrelated to the payment for batching purpose.

Expand All @@ -113,18 +116,23 @@ The payjoin proposal MUST:
* Only fill the <code>witnessUTXO</code> or <code>nonWitnessUTXO</code> for the additional inputs.

The payjoin proposal MAY:
* Add, remove or modify the outputs belonging to the receiver.
* Add, or replace the outputs belonging to the receiver unless output substitution is disabled.

The payjoin proposal MUST NOT:
* Shuffle the order of inputs or outputs, the additional outputs or additional inputs must be inserted at a random index.
* Decrease the absolute fee of the original transaction.

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.

Comment on lines +125 to +127
Copy link
Contributor

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

Copy link
Author

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.

===BIP21 payjoin parameters===

This proposal is defining the following new [[bip-0021.mediawiki|BIP 21 URI]] parameters:
* <code>pj=</code>: Represents an http(s) endpoint which the sender can POST the original PSBT.
* <code>pjos=0</code>: Signal to the sender that they MUST disallow [[#output-substitution|payment output substitution]]. (See [[#unsecured-payjoin|Unsecured payjoin server]])

Note: the `amount` parameter is *not* required.

===<span id="optional-params"></span>Optional parameters===

When the payjoin sender posts the original PSBT to the receiver, he can optionally specify the following HTTP query string parameters:
Expand Down Expand Up @@ -242,7 +250,6 @@ The receiver needs to do some check on the original PSBT before proceeding:

* Non-interactive receivers (like a payment processor) need to check that the original PSBT is broadcastable. <code>*</code>
* If the sender included inputs in the original PSBT owned by the receiver, the receiver must either return error <code>original-psbt-rejected</code> or make sure they do not sign those inputs in the payjoin proposal.
* If the sender's inputs are all from the same scriptPubKey type, the receiver must match the same type. If the receiver can't match the type, they must return error <code>unavailable</code>.
* Make sure that the inputs included in the original transaction have never been seen before.
** This prevent [[#probing-attack|probing attacks]].
** This prevent reentrant payjoin, where a sender attempts to use payjoin transaction as a new original transaction for a new payjoin.
Expand All @@ -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.
Copy link
Contributor

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.

** Verify that the payjoin proposal did not introduced mixed input's sequence.
** Verify that the payjoin proposal did not introduced mixed input's type.
** Verify that all of sender's inputs from the original PSBT are in the proposal.
* For each outputs in the proposal:
** Verify that no keypaths is in the PSBT output
Expand All @@ -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)
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Author

@Kixunil Kixunil Oct 27, 2023

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.

Copy link
Contributor

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.

Copy link
Author

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.


Our method of checking the fee allows the receiver and the sender to batch payments in the payjoin transaction.
It also allows the receiver to pay the fee for batching adding his own outputs.
Expand Down Expand Up @@ -344,7 +351,7 @@ On top of this the receiver can poison analysis by randomly faking a round amoun

===<span id="output-substitution"></span>Payment output substitution===

Unless disallowed by sender explicitely via `disableoutputsubstitution=true` or by the BIP21 url via query parameter the `pjos=0`, the receiver is free to decrease the amount, remove, or change the scriptPubKey output paying to himself.
Unless disallowed by sender explicitely via `disableoutputsubstitution=true` or by the BIP21 url via query parameter the `pjos=0`, the receiver is free to decrease the amount, or change the scriptPubKey output paying to himself.
Note that if payment output substitution is disallowed, the reveiver can still increase the amount of the output. (See [[#reference-impl|the reference implementation]])

For example, if the sender's scriptPubKey type is P2WPKH while the receiver's payment output in the original PSBT is P2SH, then the receiver can substitute the payment output to be P2WPKH to match the sender's scriptPubKey type.
Expand Down Expand Up @@ -475,6 +482,7 @@ public async Task<PSBT> RequestPayjoin(
if (proposalGlobalTx.LockTime != originalGlobalTx.LockTime)
throw new PayjoinSenderException($"The proposal PSBT changed the nLocktime");

var additionalSize = 0;
HashSet<Sequence> sequences = new HashSet<Sequence>();
// For each inputs in the proposal:
foreach (PSBTInput proposedPSBTInput in proposal.Inputs)
Expand Down Expand Up @@ -520,9 +528,7 @@ public async Task<PSBT> RequestPayjoin(
if (proposedPSBTInput.NonWitnessUtxo == null && proposedPSBTInput.WitnessUtxo == null)
throw new PayjoinSenderException("The receiver did not specify non_witness_utxo or witness_utxo for one of their inputs");
sequences.Add(proposedTxIn.Sequence);
// 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())
Copy link
Contributor

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.

Suggested change
additionalSize = GetVirtualSize(proposedPSBTInput.GetInputScriptPubKeyType())
additionalSize += GetVirtualSize(proposedPSBTInput.GetInputScriptPubKeyType())

}
}

Expand Down Expand Up @@ -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)
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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:

Suggested change
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.

throw new PayjoinSenderException("The actual contribution is not only paying for additional inputs");
}
else if (allowOutputSubstitution && output.OriginalTxOut.ScriptPubKey == paymentScriptPubKey)
Expand Down