Skip to content

RuntimeError: memory access out of bounds #330

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

Closed
solidsnakedev opened this issue May 3, 2024 · 9 comments
Closed

RuntimeError: memory access out of bounds #330

solidsnakedev opened this issue May 3, 2024 · 9 comments

Comments

@solidsnakedev
Copy link

I'm running into some issues when running some tests

RuntimeError: memory access out of bounds
@lucid-evolution/lucid:test:  ❯ null.<anonymous> wasm:/wasm/00a747c2:1:670612
@lucid-evolution/lucid:test:  ❯ null.<anonymous> wasm:/wasm/00a747c2:1:2248216
@lucid-evolution/lucid:test:  ❯ null.<anonymous> wasm:/wasm/00a747c2:1:2240349
@lucid-evolution/lucid:test:  ❯ null.<anonymous> wasm:/wasm/00a747c2:1:1542408
@lucid-evolution/lucid:test:  ❯ null.<anonymous> wasm:/wasm/00a747c2:1:2201648
@lucid-evolution/lucid:test:  ❯ null.<anonymous> wasm:/wasm/00a747c2:1:2231342
@lucid-evolution/lucid:test:  ❯ ../../node_modules/.pnpm/@[email protected]/node_modules/@dcspark/cardano-multiplatform-lib-nodejs/cardano_multiplatform_lib.js:11219:70

I think the issues has to do with rust releasing memory,

This CML error points to line 11219 which is
const DatumOptionFinalization = new FinalizationRegistry(ptr => wasm.__wbg_datumoption_free(ptr));

I've also had to manually free CML.TransactionBuilder when is not longer needed, along with CML.TransactionBuilderConfig

@rooooooooob
Copy link
Contributor

Do you have any tight loops that run many, many times that create anything in CML? We've enabled weakrefs in wasm/js which are supposed to be freed automatically when they go out of scope but I think JS gets behind on this sometimes so anywhere that is creating lots of the same struct you might want to explicitly call free().

When I tested this I had the tx builder create millions of transactions in a loop and it'd get out of memory after I think 500k if I had it in a regular loop, but if I put some pauses in it it'd still eventually run out of memory but usually not until after many millions. The more pause I gave it the less likely it was to do that but it'd still tend to happen when I was doing it with like 50k iterations before pausing.

And are you sure nothing in JS is maintaining references to any of these wasm structs? That would stop it from being automatically GC'd too. I'm not sure if it's smart enough to detect things that reference each other and nothing else or if it just counts refs but it could be the latter.

@solidsnakedev
Copy link
Author

I have implemented a retry loop in case the transaction fails, but it is currently configured for only four attempts, with exponential delays of 20 seconds.

Now, I have made the following updates:
Updated CML to version 5.3.
Upgraded vitest to version 1.6.0.
Updated Node.js to version 21.6.2.

and I get this error...

@lucid-evolution/lucid:test: Error: recursive use of an object detected which would lead to unsafe aliasing in rust
@lucid-evolution/lucid:test:  ❯ module.exports.__wbindgen_throw ../../node_modules/.pnpm/@[email protected]/node_modules/@dcspark/cardano-multiplatform-lib-nodejs/cardano_multiplatform_lib.js:37518:11
@lucid-evolution/lucid:test:  ❯ null.<anonymous> wasm:/wasm/00a747c2:1:2501206
@lucid-evolution/lucid:test:  ❯ null.<anonymous> wasm:/wasm/00a747c2:1:2501220
@lucid-evolution/lucid:test:  ❯ null.<anonymous> wasm:/wasm/00a747c2:1:2212507
@lucid-evolution/lucid:test:  ❯ null.<anonymous> wasm:/wasm/00a747c2:1:2034126
@lucid-evolution/lucid:test:  ❯ null.<anonymous> wasm:/wasm/00a747c2:1:2231337
@lucid-evolution/lucid:test:  ❯ ../../node_modules/.pnpm/@[email protected]/node_modules/@dcspark/cardano-multiplatform-lib-nodejs/cardano_multiplatform_lib.js:11219:70
@lucid-evolution/lucid:test: 
@lucid-evolution/lucid:test: This error originated in "test/hello.test.ts" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
@lucid-evolution/lucid:test: The latest test that might've caused the error is "CollectFunds". It might mean one of the following:
@lucid-evolution/lucid:test: - The error was thrown, while Vitest was running this test.
@lucid-evolution/lucid:test: - If the error occurred after the test had been completed, this was the last documented test before it was thrown.

I think this error relates to this? rustwasm/wasm-bindgen#1578
Error: recursive use of an object detected which would lead to unsafe aliasing in rust

@solidsnakedev
Copy link
Author

Is it possible to return more information when an error occurs, like using something such as https://github.com/rustwasm/console_error_panic_hook , @rooooooooob ?

@solidsnakedev
Copy link
Author

I'm also wondering if CML is using the latest https://github.com/rustwasm/wasm-bindgen/releases/tag/0.2.92 , @rooooooooob ?

@solidsnakedev
Copy link
Author

I think the problem has to do with DatumOption, for some reason when I free() DatumOption it returns this error Error: null pointer passed to rust'

However when I remove free() , this error keeps coming back
image

@solidsnakedev
Copy link
Author

@rooooooooob @SebastienGllmt I managed to fix the error by manipulating the file. node_modules/.pnpm/@[email protected]/node_modules/@dcspark/cardano-multiplatform-lib-nodejs/cardano_multiplatform_lib.js

I commented out these 2 lines
DatumOptionFinalization.register(obj, obj.ptr, obj);
DatumOptionFinalization.unregister(this);

My question is, can we disable them on demand?

image

@SebastienGllmt
Copy link
Contributor

That code is generated by wasm-pack tooling. Unless you create a script to modify the CML outputs, you would have to go make a PR to the underlying wasm pack library

@solidsnakedev
Copy link
Author

I've noticed that when I replaced TransactionOutput.new(...) with TransactionOutputBuilder.new(), the error no longer occurred.
I'll continue testing, and perhaps TransactionOutput.new should be deprecated or completely replaced by TransactionOutputBuilder.

@solidsnakedev
Copy link
Author

I think I can close this, I've been testing the TransactionOutputBuilder.new() without any issues at all

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

3 participants