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

Fix Payjoin: Add back original PSBT input to payjoin proposal #267

Closed
wants to merge 3 commits into from

Conversation

DanGould
Copy link
Collaborator

@DanGould DanGould commented Jul 12, 2023

Unlike Bitcoin Core's walletprocesspsbt RPC, BDK's finalize_psbt only checks if the script in the PSBT input map matches the descriptor and does not check whether it has control of the OutPoint specified in the unsigned_tx's TxIn. BIP 78 spec clears script data from payjoin proposal. So the original_psbt input data needs to be added back into payjoin_psbt without overwriting receiver input.

See where we ran into the same problem with BDK-CLI and the proposed solution there.

Unlike Bitcoin Core's walletprocesspsbt RPC, BKD's finalize_psbt only checks
if the script in the PSBT input map matches the descriptor and does not
check whether it has control of the OutPoint specified in the unsigned_tx's
TxIn. So the original_psbt input data needs to be added back into
payjoin_psbt without overwriting receiver input.

BIP 78 spec clears script data from payjoin proposal.
Copy link
Member

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

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

Neat! LGTM, we can test this in our next release.

@DanGould
Copy link
Collaborator Author

DanGould commented Jul 12, 2023

I've added 2 1 more relatively small commit.

  • One bumps the library 0.8.0 (was set to =0.8.0 since 0.8.1 updates to rust bitcoin 0.30.~ oops! I yanked 0.8.1)
  • Second makes use of the fee parameter. PDK needs an upgrade to the fee calculation and estimation, but I implemented it manually here.

src/bitcoin/payment.rs Outdated Show resolved Hide resolved
@DanGould DanGould force-pushed the payjoin-fix branch 2 times, most recently from e7188b9 to de8d8b3 Compare July 12, 2023 19:23
@cryptoquick
Copy link
Member

These changes still look fine. If any fixes need to be made, we can do them in a separate PR. I'll be sure to run CI checks on this as well.

@cryptoquick
Copy link
Member

I've invited you to the project with write access so your contributions can run in CI more easily.
For now I'm running CI checks for this PR here:
https://github.com/diba-io/bitmask-core/actions/runs/5545431781

@cryptoquick
Copy link
Member

Looks like there was a linter issue. I'm closing this for #269.

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

Successfully merging this pull request may close these issues.

2 participants