Skip to content
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

fix: ensure transferred ArrayBuffer are Transferable #6251

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,9 +1,29 @@
import fs from "node:fs";
import worker from "node:worker_threads";
import {expose} from "@chainsafe/threads/worker";
import {Transfer, TransferDescriptor} from "@chainsafe/threads";
import {Keystore} from "@chainsafe/bls-keystore";
import {DecryptKeystoreArgs, DecryptKeystoreWorkerAPI, isLocalKeystoreDefinition} from "./types.js";

/**
* @param buffer The ArrayBuffer to be returned as transferable
* @returns a buffer that can be transferred. If the provided buffer is marked as untransferable, a copy is returned
*/
function transferableArrayBuffer(buffer: ArrayBuffer): ArrayBuffer {
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-assignment
const unknownWorker = worker as any;
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-assignment
const isMarkedAsUntransferable = unknownWorker["isMarkedAsUntransferable"];
// Can be updated to direct access once minimal version of node is 21
// eslint-disable-next-line @typescript-eslint/no-unsafe-call
if (isMarkedAsUntransferable && isMarkedAsUntransferable(buffer)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the buffer always untransferable or why do we need this check? Based on nodejs/node#47604 it would previously (node 20) just silently copy the buffer while with node 21 it throws an error. The problem seems to be related to @chainsafe/bls-keystore and how the keystore buffer is created.

Upgrading @chainsafe/bls-keystore fixes the issue for me since with v3 it uses Unit8Array instead of Buffer. Will do some performance testing with new version and open PR for that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it might not be transferred at all with node20.

If the new @chainsafe/bls-keystore offers equivalent or better perf that's definitely the better option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you try #6253 on your machine? At least on linux there are no issues running unit tests on node 21

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that it works on osx with node 21

// Return a copy of the buffer so that it can be transferred
return buffer.slice(0);
} else {
return buffer;
}
}

/**
* Decrypt a single keystore, returning the secret key as a Uint8Array
*
Expand All @@ -18,7 +38,7 @@ export async function decryptKeystore(args: DecryptKeystoreArgs): Promise<Transf
const secret = await keystore.decrypt(args.password);
// Transfer the underlying ArrayBuffer back to the main thread: https://threads.js.org/usage-advanced#transferable-objects
// This small performance gain may help in cases where this is run for many keystores
return Transfer(secret, [secret.buffer]);
return Transfer(secret, [transferableArrayBuffer(secret.buffer)]);
}

expose({decryptKeystore} as unknown as DecryptKeystoreWorkerAPI);
Loading