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

Change default tx to version 2 #1789

Merged

Conversation

benalleng
Copy link
Contributor

Description

Changes the default version number to version 2 to enchance privacy for the type of wallet used

Fixes #1676

Changelog notice

  • Changed the default transaction version number in the transaction builder.
  • Updated the test to test for the default transaction number.

Checklists

All Submissions:

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

@benalleng
Copy link
Contributor Author

Should any of the tests be updated to use a version 2 tx after setting it as a new default?

@ValuedMammal
Copy link
Contributor

Should any of the tests be updated to use a version 2 tx after setting it as a new default?

You could possibly add an assertion to one of the existing tests in tests/wallet.rs to check that transactions we create are using the new default.

@ValuedMammal ValuedMammal added this to the 1.1.0 milestone Jan 3, 2025
Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

Thanks @benalleng overall this looks good to me. My only comment is I think some additional cleanups can be done like using bitcoin::transaction::Version everywhere internally instead of this roundabout way that we're doing now. But I'm ok with pushing that to a followup since this PR effectively resolves the current issue.

let mut tx = Transaction {
version: transaction::Version::non_standard(version),
lock_time,
input: vec![],
output: vec![],
};

@benalleng
Copy link
Contributor Author

Just to confirm you had meant removing the version default in the tx_builder completely in favor of the bitcoin::transaction::Version?

crates/wallet/src/wallet/mod.rs Outdated Show resolved Hide resolved
crates/wallet/src/wallet/mod.rs Outdated Show resolved Hide resolved
@notmandatory
Copy link
Member

Overall this looks good, but should be squashed into one or at most two commits with "conventional commit" style messages. Something like:

fix(tx_builder): change default transaction version to 2

This change improves privacy since >85% of transactions on the network are version 2. Version 2 is also necessary to eventually implement BIP 326 nSequence-based anti-fee-sniping.

@benalleng benalleng force-pushed the tx_builder_default_version branch 2 times, most recently from c60ec12 to 84a9bc6 Compare January 7, 2025 01:53
This change improves privacy since >85% of transactions on the network are version 2. Version 2 is also necessary to eventually implement BIP 326 nSequence-based anti-fee-sniping.
@benalleng benalleng force-pushed the tx_builder_default_version branch from 84a9bc6 to 2219b7c Compare January 7, 2025 02:02
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 2219b7c

@benalleng
Copy link
Contributor Author

Thanks for holding my hand through this one!

@notmandatory notmandatory merged commit a5335a1 into bitcoindevkit:master Jan 10, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Wallet should use transaction version 2 when creating transactions
3 participants