Bump MSRV, upgrade dependencies and clippification #906
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1.63 is the latest version supported by Debian. It's also now almost a year and a half old (August 2023). We start getting way behind on our dependency updates, and sometimes even missing some security updates (nothing which affects us but still..). Asking to support a less than 2.5yo compiler shouldn't be that much to ask, but hey. 1.63 is now a better balance between support for upstream updates and support for a not recklessly new compiler.
Took care of keeping dirs pinned to avoid the dirs license shitshow. See dirs-dev/dirs-sys-rs@e169da7#r111303146 for more.
This gives us bundled SQLite 3.44.0. Notably this patches CVE-2022-35737 but it shouldn't affect us.
It now finally accepts an option as argument.
darosior
force-pushed
the
2401_bump_msrv
branch
from
314971f
to
e478e57
Compare
jp1ac4
reviewed
Jan 5, 2024
src/spend.rs
Outdated
Comment on lines
305
to
318
| let weight: u32 = tx_with_change | ||
| .weight() | ||
| .to_wu() | ||
| .checked_sub(base_weight.into()) | ||
| .expect("base_weight can't be larger") | ||
| .try_into() | ||
| .expect("tx size must always fit in u32") | ||
| .expect("tx size must always fit in u32"); | ||
| // Starting with version 0.31, rust-bitcoin now accounts for the segwit marker when | ||
| // serializing transactions with no input. See comment above. | ||
| if tx_with_change.input.is_empty() { | ||
| weight.saturating_sub(2) | ||
| } else { | ||
| weight | ||
| } |
There was a problem hiding this comment.
I think we could alternatively subtract base_tx.weight().to_wu() instead of base_weight and then the extra 2 would be cancelled out from both weight values:
diff --git a/src/spend.rs b/src/spend.rs
index a373542..fc71280 100644
--- a/src/spend.rs
+++ b/src/spend.rs
@@ -300,22 +300,15 @@ fn select_coins_for_spend(
let long_term_feerate = FeeRate::from_sat_per_vb(LONG_TERM_FEERATE_VB);
let drain_weights = DrainWeights {
output_weight: {
- let mut tx_with_change = base_tx;
+ let mut tx_with_change = base_tx.clone();
tx_with_change.output.push(change_txo);
- let weight: u32 = tx_with_change
+ tx_with_change
.weight()
.to_wu()
- .checked_sub(base_weight.into())
+ .checked_sub(base_tx.weight().to_wu())
.expect("base_weight can't be larger")
.try_into()
- .expect("tx size must always fit in u32");
- // Starting with version 0.31, rust-bitcoin now accounts for the segwit marker when
- // serializing transactions with no input. See comment above.
- if tx_with_change.input.is_empty() {
- weight.saturating_sub(2)
- } else {
- weight
- }
+ .expect("tx size must always fit in u32")
},
spend_weight: max_input_weight,
};
There was a problem hiding this comment.
Good call. Did something similar:
diff --git a/src/spend.rs b/src/spend.rs
index e8e2e10..091839f 100644
--- a/src/spend.rs
+++ b/src/spend.rs
@@ -300,22 +300,18 @@ fn select_coins_for_spend(
let long_term_feerate = FeeRate::from_sat_per_vb(LONG_TERM_FEERATE_VB);
let drain_weights = DrainWeights {
output_weight: {
+ // We don't reuse the above base_weight.since 2 WU may have been substracted from it.
+ // See comment above for details.
+ let nochange_weight = base_tx.weight().to_wu();
let mut tx_with_change = base_tx;
tx_with_change.output.push(change_txo);
- let weight: u32 = tx_with_change
+ tx_with_change
.weight()
.to_wu()
- .checked_sub(base_weight.into())
+ .checked_sub(nochange_weight)
.expect("base_weight can't be larger")
.try_into()
- .expect("tx size must always fit in u32");
- // Starting with version 0.31, rust-bitcoin now accounts for the segwit marker when
- // serializing transactions with no input. See comment above.
- if tx_with_change.input.is_empty() {
- weight.saturating_sub(2)
- } else {
- weight
- }
+ .expect("tx size must always fit in u32")
},
spend_weight: max_input_weight,
};(Using a different var name to avoid shadowing if ever refactoring this code in the future.)
The most notable change is rust-bitcoin's change in the serialization of transaction with no input. It now accounts for the segwit marker even for those. The base tx weight in coin selection had to be adapted to handle this. See https://gnusha.org/bitcoin-rust/2024-01-04.log for details.
Bump clippy in CI to latest stable.
darosior
force-pushed
the
2401_bump_msrv
branch
from
e478e57
to
0e99136
Compare
|
reACK 0e99136 -- re-applying jp1ac4's ACK after addressing review |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New years' cleanup!
Our MSRV was becoming too restrictive to keep being able to benefit from the latest releases of our dependencies. Strike a better balance between not requiring a recklessly new compiler and still being able to get upstream bug fixes.
From this update most of our dependencies to their latest point releases. Then upgrade rusqlite, jsonrpc and rust-[bitcoin, miniscript] to their latest major version. Note their is a ton of updates. I've done a minimal due diligence to check what was updated, to what version, what changed, but i can't realistically audit all the code changes this is effectively pulling. I think we need to look into
cargo-crev.Take the opportunity to apply some clippy cleanups, and bump the version we're using in CI.
See commit messages for details.