librustzcash icon indicating copy to clipboard operation
librustzcash copied to clipboard

`zcash_client_sqlite`: Figure out how to build SQLite in a way that is compatible with Rust, or require downstreams to do so

Open str4d opened this issue 1 year ago • 6 comments

The ECC wallet team opened https://github.com/mozilla/rust-android-gradle/issues/105 because they were having issues using rust-android-gradle with the Android emulator on x86_64 machines. My subsequent investigation (https://github.com/mozilla/rust-android-gradle/issues/105#issuecomment-1481879021 and below) indicates that this is actually an incompatibility between SQLite and Rust:

  • SQLite uses long double to preserve as much precision as possible in various places, primarily ASCII-to-float and printf. This causes it to depend on compiler builtins using 80-bit or 128-bit floats where support is available.
  • Rust does not support floats larger than double, so does not implement a bunch of "standard" compiler builtins related to them.

This problem went unnoticed on Android for a long time because:

  • In NDK 22 and below, libgcc is linked for unwinding purposes, and it happens to provides these compiler builtin symbols, so there were no linker problems.
  • In NDK 23 and above, libgcc is absent, but Rust for some reason provides the needed symbols for aarch64-linux-android targets.

We currently use SQLite via the rusqlite crate's bundled flag, which automatically compiles and bundles SQLite using whatever C compiler the cc toolchain picks up. Technically we shouldn't do this here; it should be up to the user of zcash_client_sqlite to choose how it links SQLite. In any case, the current setup means that the C compiler assumes these symbols will be available, but Rust doesn't provide them. On desktop I assume we just get the symbols from the C compiler itself, but on Android they came from the NDK which no longer provides them.

We need to figure out how to set up rusqlite to build SQLite in such a way that the necessary symbols are always provided. This may mean removing the bundled feature flag and pushing the problem onto downstream users, that have the necessary context to better solve the issue.

str4d avatar Mar 23 '23 22:03 str4d

On desktop I assume we just get the symbols from the C compiler itself, but on Android they came from the NDK which no longer provides them.

On a desktop, libgcc (strictly, libgcc_s.so.1) is typically dynamically linked. It is one of the few libraries we dynamically link to in zcashd, despite statically linking most of the dependencies.

daira avatar Mar 28 '23 20:03 daira

We currently use SQLite via the rusqlite crate's bundled flag, which automatically compiles and bundles SQLite using whatever C compiler the cc toolchain picks up. Technically we shouldn't do this here; it should be up to the user of zcash_client_sqlite to choose how it links SQLite.

I agree, we shouldn't do that. The system cc could be broken in various ways, and is not pinned. Even though the SQLite build can't/shouldn't affect consensus compatibility, the app that uses zcash_client_sqlite might want to ensure a deterministic build or otherwise control build options.

On the other hand, the description of the bundled option says:

This is a good option for cases where linking to SQLite is complicated, such as Windows.

The plight of a beleaguered dev who upgrades zcash_client_sqlite and is suddenly faced with having to figure out how to link SQLite on Windows, fills me with sympathy and solidarity — so I propose that we add a non-default bundled feature of zcash_client_sqlite that enables the bundled feature of rusqlite, and document its limitations.

daira avatar Mar 30 '23 10:03 daira

Ah, the cc crate used to build the bundled SQLite will pay attention to CC and CFLAGS environment variables (which can be configured per-platform): https://github.com/rust-lang/cc-rs#external-configuration-via-environment-variables I verified that cargo clean; CC=clang cargo check works for zcash_client_sqlite, and that cargo clean; CC=this-compiler-does-not-exist cargo check fails as you'd expect.

If we figure out what CFLAGS are needed, this approach could solve both the missing compiler builtins and the nondeterministic build issues, while still delegating to rusqlite / libsqlite3-sys to build the correct version of SQLite for its bindings.

daira avatar Mar 30 '23 13:03 daira

@str4d @daira some new idea or solution for this problem? We just started to have this problem building zingo-mobile for Android...

cc: @zancas @AloeareV

juanky201271 avatar Aug 21 '23 15:08 juanky201271

We ended up solving this in the Android SDK with a build script, much the same way as others have: https://github.com/Electric-Coin-Company/zcash-android-wallet-sdk/blob/88058c63461f2808efc953af70db726b9f36f9b9/backend-lib/build.rs

str4d avatar Jan 24 '24 01:01 str4d

Reopening because even with the above fix, we might still want to enable downstreams to control whether SQLite is bundled or not.

str4d avatar Jan 25 '24 18:01 str4d