Skip to content

Conversation

palas
Copy link
Contributor

@palas palas commented Sep 23, 2025

Using coerce for partialiseWord64 assumes that csize is 64 bits, and this breaks at architectures where this is not true (like in wasm32).

This is my proposed solution, but I am not sure of the context of this code, so it may not be correct.

Feel free make or suggest modifications to the solution

@palas palas requested a review from a team as a code owner September 23, 2025 18:50
@jorisdral
Copy link
Contributor

Thanks for the PR!

Hmm, right, I see what you are saying. Was this causing you problems in practice?

I think in the software stack where we're using fs-api and fs-sim, i.e., cardano-node, there is an implicit assumption that the system it runs on is 64-bit. But of course we can be extra careful that fs-api and fs-sim work on 32-bit systems as well if it it's not too complicated

@palas
Copy link
Contributor Author

palas commented Sep 25, 2025

Thanks for the PR!

Hmm, right, I see what you are saying. Was this causing you problems in practice?

I think in the software stack where we're using fs-api and fs-sim, i.e., cardano-node, there is an implicit assumption that the system it runs on is 64-bit. But of course we can be extra careful that fs-api and fs-sim work on 32-bit systems as well if it it's not too complicated

Yes, that is a fair assumption, because modern systems are all typically 64 bits. We ran into it because we are compiling to WebAssembly (here), and WebAssembly does use 32 bits (I've read that the reason why WebAssembly uses 32 bits is that it is more portable, since running 32 bits in 64 bits is easier than the other way around). And we are compiling to WebAssembly because it gives us a lot of portability, it can be run in the browser and can be used from other languages, so we aim to increase adoption.

The function I modify in this PR is the only error that appears when targeting WebAssembly at compilation time (not sure about execution time), so I think the impact should be small. We can also make changes conditional, so that they only affect the build for WebAssembly, if that is better, but I thought in this case it would be more readable like this

@jorisdral
Copy link
Contributor

I think the change looks sensible! So we can probably go ahead and merge this. @jasagredo I don't have repo rights anymore, so maybe your team should pick this up?

I wouldn't worry about run-time issues with this code. It will probably only start showing run-time problems once the Word64 values we're working with get close to the maximum value of a machine word (32 bits in your WebAssembly case). This is testing code, and in that context I would be surprised if we ever use such large values.

@jasagredo
Copy link
Contributor

Sounds good, let's get it merged and released

@jasagredo jasagredo added this pull request to the merge queue Sep 29, 2025
@jorisdral
Copy link
Contributor

(don't forget to add a changelog entry)

Merged via the queue into input-output-hk:main with commit bafaff2 Sep 29, 2025
22 checks passed
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

Successfully merging this pull request may close these issues.

3 participants