wasm32 support for twenty-first

[dan-da]

Adds wasm32 support. See included README files:

• README-wasm32.md --> usage instructions, aimed at users of twenty-first.
• README-wasm32-dev.md --> details of the changes to enable wasm32 support, aimed at twenty-first devs/contributors.

This PR is a series of commits. The very first commit only changes Cargo.toml and is sufficient to enable wasm32 support for cargo build meaning that crates that depend on twenty-first can use it in their wasm32 builds. So it can be considered the "bare minimum" for wasm32 support.

The remainder of the commits mainly tackle the challenge of getting our unit tests and especially proptests working in the wasm32 environment, which is substantially more complicated.

There are also a couple of commits that address failing test cases. One of these failing tests found a problem with twenty-first when executing in a 32-bit environment. This is addressed in 19a11a4, and should be carefully reviewed for correctness. This is the only change that was made to non-test library code in any of the commits, afaik.

This PR may be easiest to grok by:

1. reading the two README files.
2. reviewing each commit, in sequence from oldest to newest.

note: the README files were generated by Gemini LLM, with coaching and minor edits. Also the LLM was very helpful for navigating through the complex issues getting the tests to build and execute.

[Sword-Smith]

👀 Very interesting! Would be really cool if we could get our dependencies to all run on wasm-32.

What I've seen looks good, with minimal changes/decorations to get this to work. I'll let others weigh in though, as devops is not my forté.

[jan-ferdinand]

Thanks for taking this on. I've left a number of comments inline. Additionally:

• I can't wasm-pack build because apparently, “crate-type must be cdylib to compile to wasm32-unknown-unknown”. The suggestion is to add crate-type = ["cdylib", "rlib"] to the Cargo.toml's [lib] section. I don't know what the implications of this are. Do you?
• Running wasm-pack test --node (which works) results in one test failure, in particular, ntt_on_empty_input. Running cargo t ntt_on_empty_input results in a pass. Seems like we need to do some debugging.
• Before merging, I'd like to have a CI script that builds twenty-first for a wasm target and executes the full test suite successfully.

[jan-ferdinand]

In order to not lose the docstring on #[cfg(not(target_pointer_width = "32"))] architectures, I suggest to merge these.

Suggested change

	#[cfg(target_pointer_width = "32")]
	const NUM_DOMAINS: usize = 29; // On 32-bit, up to length 2^28 to avoid isize::MAX overflow.
	#[cfg(not(target_pointer_width = "32"))]
	const NUM_DOMAINS: usize = 32; // Default for 64-bit and other architectures.
	const NUM_DOMAINS: usize = {
	#[cfg(target_pointer_width = "16")]
	compile_error!("pointer width 16 is not supported");
	#[cfg(target_pointer_width = "32")]
	{
	29 // On 32-bit, up to length 2^28 to avoid isize::MAX overflow.
	}
	#[cfg(target_pointer_width = "64")]
	{
	32 // Default for 64-bit and other architectures.
	}
	};

[jan-ferdinand]

We can also

#[cfg(target_pointer_width = "16")]
const NUM_DOMAINS: usize = compile_error!("pointer width 16 is not supported");

/// The number of different domains over which this library can compute (i)NTT.
///
/// In particular, the maximum slice length for both [NTT][ntt] and [iNTT][intt]
/// supported by this library is 2^28. All domains of length some power of 2
/// smaller than this, plus the empty domain, are supported as well.
//
// only up to length 2^28: avoid isize::MAX overflow
#[cfg(target_pointer_width = "32")]
const NUM_DOMAINS: usize = 29;


/// The number of different domains over which this library can compute (i)NTT.
///
/// In particular, the maximum slice length for both [NTT][ntt] and [iNTT][intt]
/// supported by this library is 2^31. All domains of length some power of 2
/// smaller than this, plus the empty domain, are supported as well.
#[cfg(not(target_pointer_width = "64"))]
const NUM_DOMAINS: usize = 32;

[dan-da]

it seems the const NUM_DOMAINS: usize = gets removed in the first suggestion. Is it implied to be above the suggested code? Can conditional compilation be applied to expressions in a statement? It seems I am not fully understanding the suggestion.

The second suggestion seems more straight-forward.

[jan-ferdinand]

You're right, I botched the first suggestion. I've edited to unbotch.

Now, I'm also realizing that this const is not pub, so it's probably not particularly important anyway.

[jan-ferdinand]

Nit: my spellchecker suggests “64-bit system” over “64 bit system”
Pedant: comment width of 80

Suggested change

	/// supported by this library is 2^31 on 64 bit systems and 2^28 on 32 bit systems.
	/// All domains of length some power of 2 smaller than this, plus the empty domain,
	/// are supported as well.
	/// supported by this library is 2^31 on 64-bit systems and 2^28 on 32-bit
	/// systems. All domains of length some power of 2 smaller than this, plus the
	/// empty domain, are supported as well.

[jan-ferdinand]

Once frozenlib/test-strategy#18 is resolved, I suggest to reduce the attribute duplication by doing the following instead:

    /// A crate-specific replacement of the `#[test]` attribute for tests that
    /// should also be executed on `wasm` targets (which is almost all tests).
    ///
    /// If you specifically want to exclude a test from `wasm` targets, use the
    /// usual `#[test]` attribute instead.
    ///
    /// # Usage
    ///
    /// ```
    /// #[macro_rules_attr::apply(test)]
    /// fn foo() {
    ///     assert_eq!(4, 2 + 2);
    /// }
    /// ```
    macro_rules! test {
        ($item:item) => {
            #[test]
            #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)]
            $item
        };
    }
    pub(crate) use test;

    /// A crate-specific replacement of the `#[test_strategy::proptest]`
    /// attribute for tests that should also be executed on `wasm` targets
    /// (which is almost all tests).
    ///
    /// If you specifically want to exclude a test from `wasm` targets, use the
    /// usual `#[test_strategy::proptest]` attribute instead.
    ///
    /// # Usage
    ///
    /// ```
    /// #[macro_rules_attr::apply(proptest)]
    /// fn foo(#[strategy(0..=42)] x: i32) {
    ///     assert_eq!(2 * x, x + x);
    /// }
    /// ```
    macro_rules! proptest {
        ($item:item) => {
            #[test_strategy::proptest]
            #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)]
            $item
        };
    }
    pub(crate) use proptest;

Pros

• Less code duplication
• I think it's easier to see whether a new test has #[macro_rules_attr::apply(test)] over #[test] than it is to see whether the line #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] is missing

Cons

• macro_rules_attr becomes a new dev-dependency

[dan-da]

It seems unfortunate that the wasm ecosystem has to use a different attribute for tests. And that it can be such a headache to get them working at the Cargo.toml level. The end result (according to an LLM) seems to be that many/most crates that support building wasm32 target do not bother to make the test suite build and run for wasm32 target. In that sense, this PR is going the extra mile.

[jan-ferdinand]

I both appreciate the extra mile and deem it important. I don't really trust that things will “just work” on a completely different architecture.

[jan-ferdinand]

Could you add explainer comments why the two -C flags are here?

[dan-da]

sure. in a nutshell without these the generated debug binary is so huge that the wasm test harness barfs on it and emits huge stack traces without any decipherable error message.

[jan-ferdinand]

I think this is a great summary to ease the PR process; I don't think it should make its way into the repository. If the list of changes is relevant in the future, we can query git. If a specific thing needs to be justified, that justification should live close to the thing.

[dan-da]

If I were maintainer I would include them, for future developers reference. But its your call; I can certainly remove the file.

[jan-ferdinand]

Let's hope that your PR on the upstream crate gets merged, but I'm not holding my breath. In order to publish twenty-first on crates.io, we must not have git dependencies per their policy. There are two obvious ways forward, and maybe there are more:

• We (or you) publish your fork as a new crate to crates.io, twenty-first then depends on that crate.
• We copy the entire content of your fork into twenty-first and remove the dependency. (We should publicly expose this for downstream usage.)

I don't like either method too much. I think I slightly favor the latter approach. Perhaps you see a better option still?

[dan-da]

yeah, I don't really like either option. I just added a comment to that PR asking if its likely to be merged soon. If no reply by end of week, I suppose we should move forward with one of the two options.

[jan-ferdinand]

This is very helpful. I think we might as well merge this with the README.md. Thoughts?

[dan-da]

I think wasm32 is not relevant for a lot of people and would just clutter up the main README for them. I would suggest we keep the files separate, but add a link from the main readme.

[jan-ferdinand]

Too bad. I think it's better to keep the test and to remove the dependency on insta. To be frank, we don't really need the functionality provided by insta anyway, as we're only asserting on u64s and the occasional Digest with it. To me, it's much more important to have the snapshot tests also in wasm than to communicate that the assertion is a snapshot test.

[jan-ferdinand]

I've removed insta as a dev-dependency in 17a5683. Feel free to rebase.

[dan-da]

i will, thx.

[jan-ferdinand]

“Make sure your current working directory is /path/to/twenty-first/twenty-first before executing any of the commands below.”

[dan-da]

Thanks for taking this on. I've left a number of comments inline. Additionally:

• I can't wasm-pack build because apparently, “crate-type must be cdylib to compile to wasm32-unknown-unknown”. The suggestion is to add crate-type = ["cdylib", "rlib"] to the Cargo.toml's [lib] section. I don't know what the implications of this are. Do you?

well, I just learned:

• I've been building a Rust-internal Wasm library (rlib) with cargo build --target wasm32-unknown-unknown. This is usable by the test harness.
• You were trying to build a web-ready package with wasm-pack. That requires cdylib format, a stable ABI that a javascript runtime can load.

Apparently adding crate-type = ["cdylib", "rlib"] is the standard way to tell Cargo to produce both formats: the rlib needed for cargo test to work, and the cdylib needed for wasm-pack to build the final product.

Hopefully we can make this specific to the wasm32 build.

• Running wasm-pack test --node (which works) results in one test failure, in particular, ntt_on_empty_input. Running cargo t ntt_on_empty_input results in a pass. Seems like we need to do some debugging.

I observed same/similar. Though I was using cargo build --target wasm32-unknown-unknown.

However I also saw multiple test failures running cargo test against unmodified master branch on my machine. The exact observations were:

1. some tests would fail when run in parallel with cargo test
2. Those same tests would succeed when run individually.
3. Some or all of them would also fail when run with cargo test -- --test-threads 1.

This is novel behavior and I concluded that perhaps there is some shared state happening between tests. But since it was happening on master I put it firmly in the "not my problem" box. If anything, the wasm32 tests behaved better under the wasm test harness, because only one of them failed when run with cargo test --target wasm32-unknown-unknown.

Regardless, my position is that:

1. the very first commit introduces basic wasm support, and is sufficient to enable crates that depend on twenty-first to build wasm32 target. So if only that commit gets merged, it is already a win and all I really need for wallet purposes.
2. getting tests to work with wasm32 was already a substantial effort, outside my present area of focus. I did that work to "help out", but I am not inclined to spend much more effort on it nor to debug a single flaky test (that passes when run by itself) further. I don't think that should be a showstopper for this PR, but that's not my call. Stated differently, I don't think it is demonstrated that the (very mechanical) wasm32 changes cause the test failure; rather the test suite itself may have issues.

• Before merging, I'd like to have a CI script that builds twenty-first for a wasm target and executes the full test suite successfully.

I consider CI scripts to be outside scope of this PR. Anyway, it is not a skill I possess. I would suggest that could be done in a followup and should not block merging this PR.

[jan-ferdinand]

As per discussion with @dan-da, we're splitting this PR in two. The PR's first commit is merged into master via 1aeb2a2, the remaining commits will be merged (in due time) via #269.

Thanks, @dan-da, for getting tests to work on wasm32-unknown-unknown!