[wasm] Precompile for ecmult increases wasm release build (z) for my project from 69kB to 1181kB

[junderw]

(Ignoring the fact that WASM doesn't work on master right now)

I was shocked to notice that WASM binary size grew from 69kB to 1181kB on z release build running it through wasm-opt tool.

Deleting the file additions of precomputed_ecmult_gen.c and precomputed_ecmult.c from build.rs brought the size back down to 69kB (while still being broken due to other issues)

Perhaps there could be a feature similar to lowmemory that will lower the binary size.

There is a possibility that the inability for z release profile and wasm-opt tool not being able to remove any of the data (even in lowmemory feature mode with lower window, the binary size did not change) could be a hint toward the solution to the other WASM issues.

But for now, I am making this a separate issue, which may or may not be solved by solving the other WASM issues. Just putting it out there.

[elichai]

We can probably tweek ECMULT_WINDOW_SIZE for this, can you share your workspace / configuration that gave you the 1800kB and I'll look into it this weekend?

[junderw]

Repro steps: (edit: 1181kB not 1800kB)

(Note: this requires wasm-opt in your PATH)

$ git clone \
    --branch test/bloated-wasm \
    --depth 2 \
    https://github.com/bitcoinjs/tiny-secp256k1.git \
    /tmp/tiny-secp256k1 && \
    cd /tmp/tiny-secp256k1
$ git checkout HEAD~1
$ make build-wasm
$ ls -l lib/secp256k1.wasm 

-rwxr-xr-x 1 xxx xxx 69385  Mar 22 00:42 lib/secp256k1.wasm

$ git checkout test/bloated-wasm
$ make build-wasm
$ ls -l lib/secp256k1.wasm 

-rwxr-xr-x 1 xxx xxx 1181312  Mar 22 00:42 lib/secp256k1.wasm

$ git diff HEAD~1

diff --git a/Cargo.lock b/Cargo.lock
index 183c6b1..9a0f0be 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -79,18 +79,16 @@ dependencies = [
 
 [[package]]
 name = "secp256k1"
-version = "0.21.3"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "9c42e6f1735c5f00f51e43e28d6634141f2bcad10931b2609ddd74a86d751260"
+version = "0.22.1"
+source = "git+https://github.com/junderw/rust-secp256k1?rev=e88fc67a61b79565626b25fc08d6816e23f84cf6#e88fc67a61b79565626b25fc08d6816e23f84cf6"
 dependencies = [
  "secp256k1-sys",
 ]
 
 [[package]]
 name = "secp256k1-sys"
-version = "0.4.2"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "957da2573cde917463ece3570eab4a0b3f19de6f1646cde62e6fd3868f566036"
+version = "0.5.0"
+source = "git+https://github.com/junderw/rust-secp256k1?rev=e88fc67a61b79565626b25fc08d6816e23f84cf6#e88fc67a61b79565626b25fc08d6816e23f84cf6"
 dependencies = [
  "cc",
 ]
diff --git a/tiny-secp256k1/Cargo.toml b/tiny-secp256k1/Cargo.toml
index 422f431..105d307 100644
--- a/tiny-secp256k1/Cargo.toml
+++ b/tiny-secp256k1/Cargo.toml
@@ -19,5 +19,6 @@ no_std = []
 minimal_validation = []
 
 [dependencies]
-secp256k1 = { version = "0.21.3", default-features = false, features = ["recovery"] }
+# This commit is on top of 0.22.1 (removing duplicate symbols so WASM can compile at all)
+secp256k1 = { git = "https://github.com/junderw/rust-secp256k1", rev = "e88fc67a61b79565626b25fc08d6816e23f84cf6", default-features = false, features = ["recovery"] }
 rand = { version = "0.8.4", optional = true }
diff --git a/tiny-secp256k1/src/lib.rs b/tiny-secp256k1/src/lib.rs
index 20033ca..7ccc7ed 100644
--- a/tiny-secp256k1/src/lib.rs
+++ b/tiny-secp256k1/src/lib.rs
@@ -249,7 +249,7 @@ pub fn private_add(
     validate_tweak(tweak)?;
     let mut sec = SecretKey::from_slice(private.as_slice()).map_err(|_| Error::BadPrivate)?;
     if sec.add_assign(tweak.as_slice()).is_ok() {
-        Ok(Some(sec.serialize_secret()))
+        Ok(Some(sec.secret_bytes()))
     } else {
         Ok(None)
     }
@@ -274,8 +274,8 @@ pub fn private_sub(
     let mut tweak = SecretKey::from_slice(tweak.as_slice()).map_err(|_| Error::BadPrivate)?;
     tweak.negate_assign();
 
-    if sec.add_assign(tweak.serialize_secret().as_slice()).is_ok() {
-        Ok(Some(sec.serialize_secret()))
+    if sec.add_assign(tweak.secret_bytes().as_slice()).is_ok() {
+        Ok(Some(sec.secret_bytes()))
     } else {
         Ok(None)
     }
@@ -286,7 +286,7 @@ pub fn private_sub(
 pub fn private_negate(private: &PrivkeySlice) -> InvalidInputResult<PrivkeySlice> {
     let mut sec = SecretKey::from_slice(private.as_slice()).map_err(|_| Error::BadPrivate)?;
     sec.negate_assign();
-    Ok(sec.serialize_secret())
+    Ok(sec.secret_bytes())
 }
 
 /// # Errors

[apoelstra]

Very interesting, oops! I think WASM is exactly the kind of case where we don't want precomp at all. @elichai probably we want to add a feature that just reduces the parameters to their minimum allowed values.

[sanket1729]

I am not experienced with WASM, but the application size in runtime should be similar. We are allocating the tables statically instead of dynamically. Just trying to understand why is the static binary size important for the WASM use-case.

[apoelstra]

@sanket1729 because in WASM the static binary is sent over HTTPS to the user.

[devrandom]

Well, this is strange, given that in #299 I saw only 62KB code increase.

We definitely want precompute for embedded systems, since it greatly decreases RAM usage, but we also don't want to increase the code size too much.

I'll take a closer look.

[junderw]

Well, this is strange, given that in #299 I saw only 62KB code increase.

Likely if I switched the opt-level = 0 and didn't run wasm-opt, the diff in size would be much closer.

[devrandom]

So I'm looking at the test in embedded/ in this repo. When I add opt_level = "z" to embedded/Cargo.toml and do a --release compilation, I get:

$ cargo +nightly build --target thumbv7m-none-eabi --release
$ arm-none-eabi-objdump -h target/thumbv7m-none-eabi/release/embedded | egrep '(text|rodata)'
  1 .text         0000cd68  00000400  00000400  00010400  2**3
  2 .rodata       0000a328  0000d168  0000d168  0001d168  2**4

So both sections are well under 64K and the secp buffer size is reported as 192 bytes. So looks like the embedded use case is working as expected.

I'll look at the wasm case.

[devrandom]

Ah, I see what's wrong. You need to turn on the lowmemory feature of the secp256k1 crate, otherwise it generates a full size precompute table.

[junderw]

I wonder why this wasn't an issue before v0.22.x?

Also, I was able to get the size down to 99800 by changing the ECMULT_WINDOW_SIZE under the lowmemory feature to 2 instead of 4. With 4 it was 100185 bytes.

But still that is a 44% increase from 69385 (which did not use lowmemory feature, btw)

[devrandom]

I assume it's because the precompute is now on by default.

This has the advantage of makding the secp contexts much cheaper - they now use just a couple hundred bytes of heap instead of hundreds of kilobytes.

[apoelstra]

To clarify: does the existing lowmemory feature solve this problem?

I guess we need to update the documentation on that feature, and probably rename it, now that it is about codesize rather than heap space.

[elichai]

Yeah, so enabling lowmemory is the best we can do right now.
but how bad is an extra ~30Kb here for a wasm binary? if it's really really bad then we should open an issue upstream to discuss this.
otherwise, currently upstream no longer allows generating the table in runtime, and the direction we're going to is to remove the context entirely

[apoelstra]

@elichai I'm sure it's fine to keep the table static, as long as we can shrink it for some applications.

[elichai]

@elichai I'm sure it's fine to keep the table static, as long as we can shrink it for some applications.

The smallest it will currently go is ~30Kb, do you know if it can be reduced even further in upstream?

[junderw]

To clarify: does the existing lowmemory feature solve this problem?

Reducing a 1603% increase to a 44% increase is a great improvement. But it would be nice to have the option to completely remove the precompute.

Also, since WASM still has issues that are open regarding its ability to be used, I would rather keep this issue open since the solution to the WASM issues might affect binary size again.

I guess we need to update the documentation on that feature, and probably rename it, now that it is about codesize rather than heap space.

Yes. This would help.

[junderw]

Also good to note that WASM contexts usually have an initialization phase (when you load the initiate the module) which can include running any functions that should be run once and hold some static data.

So having 0 precompute and creating a 30kB memory block is fine. Browsers can allocate 30kB easily.

But to be quite honest... it's not that big of a deal.

Binary increased by 44%. It would be nice to have the ability to compile without any precompute at all, and just compute the precompute tables on initialization and plop it in memory... but it's not a deal breaker.

I am fine with closing this issue, since it seems like something that is a libsecp256k1 concern.

If we close this, it might be helpful to create an issue for tracking on the C library repo.