Filesystem tweaks

[mainnet-pat]

• Eliminate synchronous fs calls
• Allow for fs in browser

[woodser]

Actually instead of adding and using the mutex dependency, the full wallet is already queuing tasks single threaded using this.module.queueTask.

I think we'd want to use this same mechanism, to ensure that the save is also queued behind scheduled wallet operations.

[mainnet-pat]

Right, maybe this is why it was sporadically erroring for me.

It is best if you will do that as you are more familiar with queueing

[mainnet-pat]

Want me to remove the mutex part?

[woodser]

Want me to remove the mutex part?

Yes please.

It is best if you will do that as you are more familiar with queueing

This:

await this.mutex.runExclusive(async () => {
  ...
});

Should be perfectly replaceable with this:

await this.module.queueTask(async () => {
 ...
});

[mainnet-pat]

this.module is not defined in MoneroWalletFullProxy. what should be used there for save call?

[woodser]

The proxy will call into the non-proxied class, which will handle the queuing, so it's not necessary in the proxy.

[mainnet-pat]

No, currently these are direct calls in the proxy:

  async moveTo(path) {
    return MoneroWalletFull.moveTo(path, this);
  }

  async save() {
    return MoneroWalletFull.save(this);
  }

[woodser]

Ah, you're right. In that case, we can avoid adding the mutex dependency by using the existing ThreadPool class, which is used by queueTask() under the covers, so makes sense to use it for the same purpose:

const pool = new ThreadPool(1);
await pool.submit(async () => {
 ...
});

[mainnet-pat]

This should be it, right?

[woodser]

The pool needs to be a shared instance to get the desired sequential execution.

I was thinking we could simply add LibraryUtils.queueTask() which uses a single pool instance, so it's available as a static utility globally, which the full wallet calls.

[mainnet-pat]

what do you think of the above?

[mainnet-pat]

Done

[mainnet-pat]

I have published my fork of indexeddb-fs as the original was lacking the unlink anyway, restored to use rename without unlinking as my fork overwrites the target. The latest commit makes use of @mainnet-pat/indexeddb-fs and makes it a dev dependency, preventing project using the monero-ts from downloading it

[woodser]

Hey I'm finally looking into all this. Apologies for the delay; it's been a ride publishing Haveno's code.

So I was looking at these filesystem changes in more detail, and now I'm wondering if we can use memfs instead.

Memfs is already used to run tests in the browser: https://github.com/woodser/monero-ts/blob/039d11dd5198cd5bd6c74815c3263f896e3f797e/src/test/utils/TestUtils.ts#L75

It's Node.js compatible so should work for everyone?

Additionally, I'm considering moving save and moveTo to be just another instance method on MoneroWalletFull, instead of a static utility accessed by both the wallet and its proxy wrapper. This would solve the issue where you needed to add LibraryUtils.queueTask, since this.module.queueTask can be called instead.

However, doing this will mean the filesystem cannot be passed as an overridable parameter, because the filesystem parameter cannot be passed over the web worker proxy. So essentially memfs will be hardcoded for browser operations.

Do you think that would be acceptable or is it essential that the FS be overridable?

[woodser]

Actually, maybe the filesystem should not be accessed within web workers, since each web worker has its own filesystem, unless using shared storage APIs.

But we could still use memfs instead of a modified indexeddb-fs if I'm not mistaken.

[mainnet-pat]

Hey, memfs hints with its name that it is an in-memory filesystem. Does it have persistency between browser window refreshes?

[mainnet-pat]

monujo.cash wallet does make use of my modified indexeddb-fs to store the wallet sync data, and does it good. I am not making any preferences, any fs-compatible lib based on promises shall work

[woodser]

Does it have persistency between browser window refreshes?

Nope, it's purely in memory. Clients would be expected to persist to storage APIs manually or by passing a custom a filesystem.

I have this branch in progress with the latest filesystem updates: https://github.com/woodser/monero-ts/pull/225

I've tested it with webpack and Next.js, trying to get vite working.

[mainnet-pat]

Right, so memfs seems to be more like a shim for the library's needs. What shall end users use to persist their wallet state then?

[mainnet-pat]

Sorry to say, but you are solving the issue without solving it

[woodser]

I didn't think the purpose of this issue was to persist to storage APIs, but instead to fix the compatibility for doing so. #225 switches to async filesystem operations based on the promises API, removes html5-fs, calls unlink before rename, and fixes the issue with concurrent saves.

Maybe another change to support persisting directly to storage APIs can/should be a follow-up PR.

[mainnet-pat]

I think this should be stated explicitly somewhere in readme, that users do need to choose their fs/promises-compatible browser implementation of fs and configure their webpack/rollupt to use it

[mainnet-pat]

I didn't think the purpose of this issue was to persist to storage APIs, but instead to fix the compatibility for doing so. #225 switches to async filesystem operations based on the promises API, removes html5-fs, calls unlink before rename, and fixes the issue with concurrent saves.

Maybe another change to support persisting directly to storage APIs can/should be a follow-up PR.

Indeed my work was to generalize the fs handling and replace the synchronous node:fs calls with asynchronous fs-like library calls for a user to drop-in

[mainnet-pat]

tbh I do not root for my fork of indexeddb-fs, I do not like it personally, it just works for me where I need it. if there'd be better choices, I'd consider an alternative

[woodser]

With the async filesystem changes, and using unlink before rename, indexeddb-fs should be a drop in replacement without any modifications, hopefully I have that right?

[woodser]

225 is ready to be merged as far as I know. Please let me know if you see any further changes needed.

[mainnet-pat]

I have figured out that fs-compatible rename overwrites the destination file, so in my current version I have just

      // replace old wallet files with new
      await wallet.fs.rename(pathNew + ".keys", path + ".keys");
      await wallet.fs.rename(pathNew, path);
      await wallet.fs.rename(pathNew + ".address.txt", path + ".address.txt");

instead of

      // delete original wallet files
      if (await LibraryUtils.exists(wallet.fs, path + ".address.txt")) await wallet.fs.unlink(path + ".address.txt");
      if (await LibraryUtils.exists(wallet.fs, path)) await wallet.fs.unlink(path);
      if (await LibraryUtils.exists(wallet.fs, path + ".keys")) await wallet.fs.unlink(path + ".keys");

      // rename new wallet files to original
      await wallet.fs.rename(pathNew + ".keys", path + ".keys");
      await wallet.fs.rename(pathNew, path);
      await wallet.fs.rename(pathNew + ".address.txt", path + ".address.txt");

[mainnet-pat]

It tuned out that memfs has an excellent fs-compatible wrapper over OPFS. I have wrapped it into a small drop-in replacement package which can be put into next.js config file:

config.resolve.alias.fs = "@mainnet-pat/opfs";

will be dropping indexeddb-fs in favor for opfs

[woodser]

It tuned out that memfs has an excellent fs-compatible wrapper over OPFS. I have wrapped it into a small drop-in replacement package which can be put into next.js config file:

config.resolve.alias.fs = "@mainnet-pat/opfs";

will be dropping indexeddb-fs in favor for opfs

Good to know this option also exists for clients.

[woodser]

These changes have been incorporated in https://github.com/woodser/monero-ts/pull/225 and released in v0.10.0. Thanks very much for your help! :)