Bump `rusqlite` to `0.31` #403

rustaceanrob · 2024-11-12T21:12:54Z

hashbrown 0.14.1 relaxes the MSRV to 1.63.0 according to this changelog, and, indeed, I compiled and tested locally on the 1.63 toolchain. I remove that here, as well as add the necessary pins for rusqlite 0.31.

Motivation for this would be to allow users to depend on both BDK and ldk-node with rusqlite backends for each. Of course ldk-node uses bdk_wallet, but users may also bring in the dependency themselves for more advanced usage.

rustaceanrob · 2024-11-13T01:17:04Z

Oh I see a similar PR for 0.32.1. Here I try to adhere to MSRV and have verified it builds on 1.63 for Linux locally, plus CI for 1.63 passes on Mac and Linux. Not sure what is happening with Windows, seems to be an unrelated build issue. Hopefully other users may also take advantage of this if they are willing to roll back from 0.32 to 0.31.

tnull · 2024-11-14T08:50:58Z

Motivation for this would be to allow users to depend on both BDK and ldk-node with rusqlite backends for each.

Mhh, please note that we don't like to arbitrarily bump dependencies if there is no good reason for it. Generally, given that LDK Node already wraps BDK, I don't think users should run a SQLite-backed BDK wallet in parallel. So the quoted reason isn't a super great motivation. However, you mentioned offline that you're working on kyoto integration which also runs a rusqlite backend that needs to be in-sync with BDK's version, so really why this bump is relevant to you is this transitive dependency I assume?

Hopefully other users may also take advantage of this if they are willing to roll back from 0.32 to 0.31.

I doubt they'd be will to do this.

@dpc Would you benefit from bumping rusqlite to 0.31 at all, or would it somehow even be worse for you?

tnull · 2024-11-14T08:47:46Z

.github/workflows/rust.yml

          cargo update -p proptest --precise "1.2.0" --verbose # proptest 1.3.0 requires rustc 1.64.0
          cargo update -p regex --precise "1.9.6" --verbose # regex 1.10.0 requires rustc 1.65.0
          cargo update -p home --precise "0.5.5" --verbose # home v0.5.9 requires rustc 1.70 or newer
          cargo update -p tokio --precise "1.38.1" --verbose # tokio v1.39.0 requires rustc 1.70 or newer
          cargo update -p tokio-util --precise "0.7.11" --verbose # tokio-util v0.7.12 requires rustc 1.70 or newer
          cargo update -p idna_adapter --precise "1.1.0" --verbose # url 2.5.3 switched to idna 1.0.3 and ICU4X, which requires rustc 1.67 or newer. Here we opt to keep using unicode-rs by pinning idna_adapter as described here: https://docs.rs/crate/idna_adapter/1.2.0
          cargo update -p indexmap --precise "2.5.0" --verbose # indexmap 2.6.0 upgraded to hashbrown 0.15, which unfortunately bumped their MSRV to rustc 1.65 with the 0.15.1 release
+          cargo update -p cc --precise "1.0.105" --verbose # cc 1.0.106 bumps MSRV to 1.67
+          cargo update -p time --precise "0.3.20" --verbose # time 0.3.21 bumps MSRV to 1.65
+          cargo update -p zstd-sys --precise "2.0.8+zstd.1.5.5" --verbose # no changelogs available


I don't think we want to start pinning compression algorithms if it can be avoided (and previously always found a way to avoid bumping zstd in particular). Could you explain why it suddenly is necessary?

I am unsure at the moment. I will look into this

Embarrassing mistake. zstd-sys is brought in from electrsd and is not related. This was negligence on my part copying pins from another CI.

Embarrassing mistake. zstd-sys is brought in from electrsd and is not related.

I assume the same could be true for cc and time?

This was negligence on my part copying pins from another CI.

No worries, nbd really!

time is an optional dependency of rusqlite and you are right ldk-node does not use it.

cc is a build dependency of libsqlite3-sys which broke MSRV on a patch version. I was unsure if this was necessary or not and I will try without it.

edit: yes cc appeared unnecessary as well

Alright, one last nit regarding the pinning: mind moving dropping the hashlink pin to a separate commit and explaining why we can now do so in the commit message?

Updated with those changes

rustaceanrob · 2024-11-14T17:05:02Z

However, you mentioned offline that you're working on kyoto integration which also runs a rusqlite backend that needs to be in-sync with BDK's version, so really why this bump is relevant to you is this transitive dependency I assume?

Yes. For the record, having kyoto, bdk_wallet, and ldk-node would put all crates on rusqlite 0.31, which would enable the default backend for a kyoto integration as opposed to a custom or copy-and-paste backend.

rustaceanrob · 2024-11-14T19:44:16Z

I updated and removed the zstd-sys pin, see comment in thread. If possible the CI should be re-ran to ensure the build passes 1.63, although I am seeing from the logs it successfully built on Mac and Linux. There was some failure in the integration tests on stable that was not build related.

dpc · 2024-11-14T22:21:41Z

@dpc Would you benefit from bumping rusqlite to 0.31 at all, or would it somehow even be worse for you?

At fedimint we use arti for Tor support, and under the hood in one of the internal deps it uses sqlite for storing some stuff, and that currently links with rusqlite that links with libsqlite3-sys/0.30.1 .

Edit: Actually, never mind. I tried that, turns out that there's no way to have conditional sys dependencies because of cargo bug/limitation.

In general I was hoping that projects that use sqlite could add conditional compilation for multiple versions, so that larger applications that need to combine multiple dependencies using sqlite had some chances of finding a version that works for all of them. If that sounds like an acceptable lift, I might try to create a PoC PR, to see how much extra boilerplate and burden it would be.

tnull · 2024-11-15T08:28:17Z

The flaky CI test is unrelated/preexisting, you might need to kick CI once more to have this pass.

TheBlueMatt · 2024-11-15T12:27:57Z

Really this is a bug in sqlite-sys. If they want to use export a heap of C symbols they need to rename them on a per-version basis. We could work around them being broken, but ideally we wouldn't have to cause it looks like we're far from the only library that's gonna cause this.

tnull

Alright, I think mod the tiny nit above this LGTM.

IIUC this would at least temporarily improve things for users of BDK/kyoto, while not making the situation worse for Fedimint (correct me if I'm wrong @dpc),

dpc · 2024-11-18T18:17:04Z

👍 All things being equal, newer is better.

`hashlink` `0.9.1` is a member of `rusqlite` `0.31.0`, which uses `hashbrown`. Previously `hashbrown` broke MSRV on a patch version (0.14.4), but `hashlink` uses the next minor version of `hashbrown` (0.15.1) which does adhere MSRV

tnull

LGTM

rustaceanrob force-pushed the rusqlite-11-12 branch from b6bf6ba to 065f128 Compare November 13, 2024 01:18

tnull reviewed Nov 14, 2024

View reviewed changes

rustaceanrob force-pushed the rusqlite-11-12 branch from 065f128 to a69e258 Compare November 14, 2024 19:30

dpc mentioned this pull request Nov 15, 2024

chore: update rusqlite 0.28.0 -> 0.32.1 #400

Closed

rustaceanrob force-pushed the rusqlite-11-12 branch 2 times, most recently from 73d9e6c to 8057467 Compare November 15, 2024 17:43

tnull reviewed Nov 18, 2024

View reviewed changes

Bump rusqlite to 0.31

2b1e02c

Remove pin on hashlink

f1e702d

`hashlink` `0.9.1` is a member of `rusqlite` `0.31.0`, which uses `hashbrown`. Previously `hashbrown` broke MSRV on a patch version (0.14.4), but `hashlink` uses the next minor version of `hashbrown` (0.15.1) which does adhere MSRV

rustaceanrob force-pushed the rusqlite-11-12 branch from 8057467 to f1e702d Compare November 18, 2024 18:49

tnull approved these changes Nov 18, 2024

View reviewed changes

tnull merged commit aaccde7 into lightningdevkit:main Nov 18, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump `rusqlite` to `0.31` #403

Bump `rusqlite` to `0.31` #403

rustaceanrob commented Nov 12, 2024 •

edited

Loading

rustaceanrob commented Nov 13, 2024 •

edited

Loading

tnull commented Nov 14, 2024

tnull Nov 14, 2024 •

edited

Loading

rustaceanrob Nov 14, 2024

rustaceanrob Nov 14, 2024

tnull Nov 15, 2024

rustaceanrob Nov 15, 2024 •

edited

Loading

tnull Nov 18, 2024

rustaceanrob Nov 18, 2024

rustaceanrob commented Nov 14, 2024 •

edited

Loading

rustaceanrob commented Nov 14, 2024 •

edited

Loading

dpc commented Nov 14, 2024 •

edited

Loading

tnull commented Nov 15, 2024

TheBlueMatt commented Nov 15, 2024

tnull left a comment

dpc commented Nov 18, 2024

tnull left a comment

Bump rusqlite to 0.31 #403

Bump rusqlite to 0.31 #403

Conversation

rustaceanrob commented Nov 12, 2024 • edited Loading

rustaceanrob commented Nov 13, 2024 • edited Loading

tnull commented Nov 14, 2024

tnull Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

rustaceanrob Nov 14, 2024

Choose a reason for hiding this comment

rustaceanrob Nov 14, 2024

Choose a reason for hiding this comment

tnull Nov 15, 2024

Choose a reason for hiding this comment

rustaceanrob Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

tnull Nov 18, 2024

Choose a reason for hiding this comment

rustaceanrob Nov 18, 2024

Choose a reason for hiding this comment

rustaceanrob commented Nov 14, 2024 • edited Loading

rustaceanrob commented Nov 14, 2024 • edited Loading

dpc commented Nov 14, 2024 • edited Loading

tnull commented Nov 15, 2024

TheBlueMatt commented Nov 15, 2024

tnull left a comment

Choose a reason for hiding this comment

dpc commented Nov 18, 2024

tnull left a comment

Choose a reason for hiding this comment

Bump `rusqlite` to `0.31` #403

Bump `rusqlite` to `0.31` #403

rustaceanrob commented Nov 12, 2024 •

edited

Loading

rustaceanrob commented Nov 13, 2024 •

edited

Loading

tnull Nov 14, 2024 •

edited

Loading

rustaceanrob Nov 15, 2024 •

edited

Loading

rustaceanrob commented Nov 14, 2024 •

edited

Loading

rustaceanrob commented Nov 14, 2024 •

edited

Loading

dpc commented Nov 14, 2024 •

edited

Loading