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

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

Open
str4d opened this issue Mar 23, 2023 · 6 comments

Comments

@str4d
Copy link
Contributor

str4d commented Mar 23, 2023

The ECC wallet team opened mozilla/rust-android-gradle#105 because they were having issues using rust-android-gradle with the Android emulator on x86_64 machines. My subsequent investigation (mozilla/rust-android-gradle#105 (comment) 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.

@daira
Copy link
Contributor

daira commented Mar 28, 2023

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
Copy link
Contributor

daira commented Mar 30, 2023

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
Copy link
Contributor

daira commented Mar 30, 2023

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.

@juanky201271
Copy link

@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

@str4d
Copy link
Contributor Author

str4d commented Jan 24, 2024

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 str4d closed this as completed Jan 24, 2024
@str4d
Copy link
Contributor Author

str4d commented Jan 25, 2024

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

@str4d str4d reopened this Jan 25, 2024
@str4d str4d removed this from the Zeboot milestone Jan 25, 2024
AArnott added a commit to nerdcash/Nerdbank.Cryptocurrencies that referenced this issue Mar 6, 2024
AArnott added a commit to nerdcash/Nerdbank.Cryptocurrencies that referenced this issue Mar 7, 2024
AArnott added a commit to nerdcash/Nerdbank.Cryptocurrencies that referenced this issue Mar 14, 2024
This workaround came from zcash/librustzcash#800 (comment)

Build android binaries with the NDK
AArnott added a commit to nerdcash/Nerdbank.Cryptocurrencies that referenced this issue Mar 15, 2024
This workaround came from zcash/librustzcash#800 (comment)

Build android binaries with the NDK
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants