-
Notifications
You must be signed in to change notification settings - Fork 331
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
Change default tx to version 2 #1789
Conversation
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 |
There was a problem hiding this 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.
bdk/crates/wallet/src/wallet/mod.rs
Lines 1390 to 1395 in 4a3675f
let mut tx = Transaction { | |
version: transaction::Version::non_standard(version), | |
lock_time, | |
input: vec![], | |
output: vec![], | |
}; |
Just to confirm you had meant removing the version default in the tx_builder completely in favor of the bitcoin::transaction::Version? |
Overall this looks good, but should be squashed into one or at most two commits with "conventional commit" style messages. Something like:
|
c60ec12
to
84a9bc6
Compare
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.
84a9bc6
to
2219b7c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 2219b7c
Thanks for holding my hand through this one! |
Description
Changes the default version number to version 2 to enchance privacy for the type of wallet used
Fixes #1676
Changelog notice
Checklists
All Submissions:
cargo fmt
andcargo clippy
before committing