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(tx_builder)!: make TxBuilder Send safe, remove Clone trait #1737

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

notmandatory
Copy link
Member

Description

Inspired by discord chat with @stevenroose as a way to make the TxBuilder Send safe.

See his original patch on 1.0.0-beta.5: https://gist.github.com/stevenroose/f7736dfedfaa64bbdbb0da5875df28fc

Notes to the reviewers

I had to remove the Clone trait on TxBuilder but it was only being used in tests.

Changelog notice

  • TxBuilder is now Send safe and does not implement the Clone trait

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

@notmandatory notmandatory added module-wallet api A breaking API change labels Nov 21, 2024
@notmandatory notmandatory self-assigned this Nov 21, 2024
@notmandatory notmandatory added this to the 1.0.0-beta milestone Nov 21, 2024
@notmandatory
Copy link
Member Author

@ValuedMammal you were right, I didn't need the Arc::new(Box::new()) for the test with custom_bip69_ordering. I put it back as it was originally and all still working. This is ready to review.

@stevenroose
Copy link
Contributor

stevenroose commented Nov 22, 2024

Do we know if there are users using the Clone implementation of TxBuilder? I can see how it might be useful to have for some people..

Otherwise ack 663fb13, changes look good.

Upon further thinking, I think any use of the Clone function can be replaced by a small function re-creating the same builder, I guess.

@notmandatory
Copy link
Member Author

Upon further thinking, I think any use of the Clone function can be replaced by a small function re-creating the same builder, I guess.

We should be fine removing Clone since I don't think anyone is using it and if anyone is there is the work-around to re-creating the builder with the same params. Plus I have gotten feedback from other projects who want TxBuilder to be thread safe so that seems the more valuable thing to do right now.

Long term the goal is to decouple TxBuilder from Wallet and at that point it will be much easier to make TxBuilder cloneable.

@notmandatory
Copy link
Member Author

notmandatory commented Nov 23, 2024

I moved this back to in-progress, I need to verify bitcoindevkit/bdk-ffi#611 doesn't need TxBuilder to impl Clone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change module-wallet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants