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

BIP 77: Payjoin Version 2 — Async Payjoin #1483

Open
wants to merge 42 commits into
base: master
Choose a base branch
from

Conversation

DanGould
Copy link
Contributor

@DanGould DanGould commented Aug 12, 2023

This document proposes an asynchronous, backwards-compatible second version of the payjoin protocol described in BIP 78, enabling complete payjoin receiver functionality including payment output substitution with only an HTTP client rather than server. The former requirement for receivers to run HTTP servers is replaced with an untrusted third-party "payjoin directory" store-and-forward server accessed by polling clients which communicate using an asynchronous protocol and authenticated, encrypted payloads. It was originally proposed to the mailing list here.

The protocol design has received rounds of review elsewhere on the bitcoin-dev mailing list as well.

Feedback from that list post has been incorporated into this draft.

Proposing this as an Standards Track BIP to ensure wallets across the ecosystem can come to rough consensus on a single asynchronous payjoin standard and correctly implement it widely.

@luke-jr
Copy link
Member

luke-jr commented Dec 26, 2023

Let's call this BIP 77

@murchandamus
Copy link
Contributor

Hi @DanGould, the first comment on this PR seems to indicate that this proposal is still WIP. Is that an accurate understanding? If this PR is not yet ready to be merged, perhaps it should be changed to "Draft". If I misunderstood the status of this PR, please respond below so someone may review to assess whether this is ready for merge.

bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
@DanGould DanGould marked this pull request as draft May 3, 2024 02:50
@DanGould
Copy link
Contributor Author

Hi @DanGould, want to update here?

Since the last review I have made breaking changes, which I'll apply with an update now. I think we're at the point where each design decision I'm aware of is deliberate. I'll take it out of draft once the new update is pushed.

@DanGould
Copy link
Contributor Author

The main changes from the last review are that

  • HTTP request methods (POST, GET, PUT) control the flow of the protocol rather than further path subdiretories
  • bip21 parameters (pj subdirectory and ohttp key config) serialize as compressed public keys to shorten the URI
  • Use PSBTv0 since PSBTv2 support is still very limited
  • explicitly mention the format of the ohttp KeyConfiguration URL fragment

@DanGould DanGould marked this pull request as ready for review September 26, 2024 21:05
@jonatack jonatack mentioned this pull request Oct 5, 2024
bip-0077.mediawiki Outdated Show resolved Hide resolved
Copy link
Contributor

@1440000bytes 1440000bytes left a comment

Choose a reason for hiding this comment

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

NACK

This BIP is bad for privacy and lacks details in "attack vectors" section. Please be careful while implementing.

@jonatack
Copy link
Member

jonatack commented Oct 8, 2024

NACK

This BIP is bad for privacy

Can you provide the reasoning, please?

@1440000bytes
Copy link
Contributor

1440000bytes commented Oct 8, 2024

NACK

This BIP is bad for privacy

Can you provide the reasoning, please?

Risks with accepting donations using payjoin:

  • Probing: Already mentioned in BIP 78
  • Small donations: Best form of attack in which FBI donates some sats to support their work and know all the UTXOs in the wallet

@1440000bytes
Copy link
Contributor

Example: https://hrf.org/payjoin

@thebrandonlucas
Copy link

Risks with accepting donations using payjoin:

  • Probing: Already mentioned in BIP 78 but BIP 77 author too emotional to accept it
  • Small donations: Best form of attack in which FBI donates some sats to support their work and know all the UTXOs in the wallet

@1440000bytes Your first point has already been addressed and he has added a link to the attack vectors section on BIP-78. No one is "not accepting" anything. Are you asking that the full explanation given on probing attacks in BIP-78 be redundantly added here? What exactly is your criticism?

Your second point has already been addressed in BIP-78, here is what the link that you suggested be added says:

While we cannot prevent this type of attack entirely, we implemented the following mitigations:

- When the receiver detects an original transaction being broadcast, or if the receiver detects that the original transaction has been double spent, then they will reuse the UTXO that was exposed for the next payjoin.
- While the exposed UTXO will be reused in priority to not leak other UTXOs, there is no strong guarantee about it. This prevents the attacker from detecting with certainty the next payjoin of the merchant to another peer.

BIP-78 already explains what can be done to mitigate this, and fully acknowledges this tradeoff.
Can you explain, in terms of probing attacks, how BIP-77 would be worse for privacy than the already accepted BIP-78?

bip-0077.mediawiki Outdated Show resolved Hide resolved
@nothingmuch
Copy link

The sender could first register an additional session subdirectory for the response, and include the associated public key along with the original PSBT when posting to the receiver's subdirectory. The sender would then respond in the sender's subdirectory.

Eliminating the request/response distinction would avoid a metadata leak, namely whether or not the payjoin flow was completed, since requests and responses would be indistinguishable.

A followup suggestion would be removing explicit session initialization. Incidentally in the reference directory it appears that since post_session is stateless, calling it before other operations is indeed not enforced. GET & POST handlers do appear to validate the id path component, but if they did then the first successful request would indicate to either party that the subdirectory ID is valid (or perhaps arbitrary IDs were allowed by design?)

Timing analysis of subdirectory polling could still leak information about which payjoin requests were responded to, but aggressive clients could mitigate this by maintaining a pool of pre-allocated and randomly polled subdirectories ahead of time, and by responding to themselves in some of them with dummy payloads to generate random cover traffic.

@DanGould
Copy link
Contributor Author

DanGould commented Oct 9, 2024

@nothingmuch I'll write up your suggestions to simplify the protocol. It makes the directory implementation simpler, requires fewer protocol message types, and even reduces bandwidth. wowza

The sender could first register an additional session subdirectory ... and include the associated public key along with the original PSBT

HPKE E2EE deployed in this application in Authenticated mode (akin to Noise Framework's IK model) already requires the sender to include their session public key as associated data, so this is an pretty darn easy change to make. The change not only simplifies the protocol messaging, but as you say eliminates a metadata leak. I'm a big fan.

A followup suggestion would be removing explicit session initialization

This makes sense too. Creating separate subdirectory resources for either sender or receiver on POST then GETting updates to replace the repeated sender POST polling reduces bandwidth too, since the GET request doesn't contain that repeat POST body.

Even without aggressive client mitigation of the remaining client leaks subject to timing analysis assuming this suggestion is put in place, your new design deletes complexity and improves efficiency.

I'll update the BIP to the best of my understanding based on your comment and link it to a rust-payjoin issue to implement. This spec has already made some breaking changes since the last directory release, so it's a good time for another one since we won't break production implementations other than our own.

Edit: Is there a potential authorization issue now that the sender POSTs first? In the current design of the protocol, only the receiver might need authorization from the directory in order to initialize a session subdirectory, and having knowledge of a session public key was sufficient for the sender to use an initialized subdirectory. Now, the sender would also need to obtain authorization somehow, since the directory doesn't yet know about the session the receiver has implicitly initiated by communicating directly with the sender, out of band from the directory. We'll have to work out how this might affect the expiration mechanic as well.

The specifics of a credential issuance are left out, however
@jonatack jonatack removed the PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author label Oct 11, 2024
bip-0077.mediawiki Outdated Show resolved Hide resolved
Copy link

@spacebear21 spacebear21 left a comment

Choose a reason for hiding this comment

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

Some minor clarifications following the "original"/"proposal" payjoin PSBT terminology established in BIP78.

bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
Copy link

@spacebear21 spacebear21 left a comment

Choose a reason for hiding this comment

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

I struggled a bit to grok the new messaging flow. After reading through the implementation I think I get it now and left some corrections/clarification suggestions throughout.

bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
bip-0077.mediawiki Outdated Show resolved Hide resolved
Comment on lines +166 to +168
===Receiver's Proposal PSBT checklist===

The receiver checklist is the same as the [[https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#receivers-original-psbt-checklist| the BIP 78 receiver checklist]], except for the "Verify that the payjoin proposal did not introduce mixed input's type" step, which may be ignored.

Choose a reason for hiding this comment

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

Suggested change
===Receiver's Proposal PSBT checklist===
The receiver checklist is the same as the [[https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#receivers-original-psbt-checklist| the BIP 78 receiver checklist]], except for the "Verify that the payjoin proposal did not introduce mixed input's type" step, which may be ignored.
===Receiver's Original PSBT checklist===
The receiver checklist is the same as the [[https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#receivers-original-psbt-checklist| the BIP 78 receiver checklist]], except for the "If the sender's inputs are all from the same scriptPubKey type, the receiver must match the same type" step, which may be ignored.

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

Successfully merging this pull request may close these issues.

10 participants