Skip to content

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.

@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