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 Redux #1605

Merged
merged 4 commits into from
Jan 14, 2025

Conversation

DanGould
Copy link
Contributor

@DanGould DanGould commented May 31, 2024

This picks up and supersedes #1244 that

  • separates concerns into individual commits
  • corrects fee verification for a proposal having mixed script inputs while ensuring additional fee indeed paid for input

I know @Kixunil is extremely busy to update these days, so I took the liberty to organize the changes we've discussed according to your wants here. @NicolasDorier I hope these changes satisfy your desire for them to be small and problem-focused while maintaining the spirit of the original.


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.

@DanGould DanGould force-pushed the bip78-mixed-inputs-ok branch 5 times, most recently from da32bf2 to a682fe6 Compare May 31, 2024 16:19
@murchandamus
Copy link
Contributor

It seems to me that you are proposing a number of changes to BIP 78 and the biggest point of friction seems to be to get sign-off from that BIP’s champion. Have you considered alternatively proposing your own variant of BIP 78 with the changes you would like to see and building BIP 77 on top of that?

@DanGould
Copy link
Contributor Author

Are you suggesting I compile the changes into an addendum document, or something else?

@murchandamus
Copy link
Contributor

Sorry, I thought this was a PR with further changes, in addition to your prior attempt to modify BIP 78. At second glance I realize now that this is a second variant with a reduced scope to the prior. Nevermind, please ignore and carry on.

@murchandamus murchandamus added Proposed BIP modification Pending acceptance This BIP modification requires sign-off by the champion of the BIP being modified labels Jun 3, 2024
Copy link
Contributor

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

I'm slowly getting some time now but feel free to go ahead with this. I'd still like to cover the edge case I mentioned because it affects some software in the wild but that can be done in a separate PR.

And thanks @DanGould for picking this up!

bip-0078.mediawiki Outdated Show resolved Hide resolved
@DanGould DanGould force-pushed the bip78-mixed-inputs-ok branch from a682fe6 to ea55024 Compare June 10, 2024 16:11
@DanGould
Copy link
Contributor Author

I'd still like to cover the edge case I mentioned because it affects some software in the wild but that can be done in a separate PR.

To be crystal clear, you're talking about transaction malleability with regard to output substitution. I think that's outside the scope of this pr but also worthy of amendment. That discussion is: https://github.com/bitcoin/bips/pull/1244/files#r1633516491

@NicolasDorier
Copy link
Contributor

NicolasDorier commented Jun 25, 2024

It means that only a single input can be covered by the contribution and the rest should be at the cost of the receiver.
Not a big deal, as I welcome that it makes things a little bit simpler while still breaking the heuristic.

  1. GetVirtualSize can be removed as it is not used anymore.
  2. Can you rename if (output.OriginalTxOut == feeOutput) to if (originalOutput.OriginalTxOut == feeOutput)

@NicolasDorier
Copy link
Contributor

NicolasDorier commented Jun 25, 2024

Ok @DanGould can you pickup this commit please: NicolasDorier@1cc129e

This basically just hardcode the maximum amount to pay for any input.

@jonatack jonatack added the PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author label Jul 1, 2024
DanGould and others added 3 commits July 8, 2024 13:57
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.

Co-authored-by: Martin Habovstiak <[email protected]>
The original text is ambiguous to allowing transaction cut-through
or not. Transaction cut-through enables savings by posting multiple
transaction intents through a single 2-party payjoin and is used
in practice in payjoins today. Let's explicitly allow it in the text.

Co-authored-by: Martin Habovstiak <[email protected]>
It's an optional parameter in BIP 21 Bitcoin URIs, but it doesn't hurt
to make it explicit.

Co-authored-by: Martin Habovstiak <[email protected]>
@DanGould DanGould force-pushed the bip78-mixed-inputs-ok branch from ea55024 to bd01a26 Compare July 8, 2024 17:57
@murchandamus murchandamus removed the PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author label Jul 17, 2024
@murchandamus
Copy link
Contributor

@NicolasDorier: It looks like the commit you requested was added. Could you please review this PR and state whether you support or reject these changes?

@NicolasDorier
Copy link
Contributor

@murchandamus yes this is ready to merge

@jonatack jonatack removed the Pending acceptance This BIP modification requires sign-off by the champion of the BIP being modified label Jan 14, 2025
@jonatack jonatack merged commit 58ffd93 into bitcoin:master Jan 14, 2025
4 checks passed
@jonatack
Copy link
Member

Closes #1244.

* Add, or replace the outputs belonging to the receiver unless output substitution is disabled.
The payjoin proposal SHOULD NOT:
* Include mixed input types until September 2024. Mixed inputs were previously completely disallowed so this gives some grace period for senders to update.
Copy link
Member

Choose a reason for hiding this comment

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

@DanGould As a follow-up, do you think these dates here and on line 110 above ought to be updated to provide a post-merge grace period?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A client may still opt to disallow mixed inputs (or any payjoin proposal) by policy rather than by protocol like this one, so I don't think it's so critical that requires an explicit grace period after all, nor does the change in #1396. I think we can leave the grace period as-is and tackle the implementations one by one.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I also see that you are reaching out to implementations at the moment 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants