solana-labs / solana-web3.js

Solana JavaScript SDK
https://solana-labs.github.io/solana-web3.js
MIT License
2.13k stars 853 forks source link

web3.js: replace dependencies to lower supply-chain risk, improve tree-shakeability, and reduce bundle size #1103

Closed paulmillr closed 2 months ago

paulmillr commented 2 years ago

You have quite a few dependencies in web3. All of those can be replaced by 4 deps:

"@ethersproject/sha2": "^5.5.0",
"bigint-buffer": "^1.1.5",
"bn.js": "^5.0.0",
"bs58": "^4.0.1",
"buffer": "6.0.1",
"js-sha3": "^0.8.0",
"secp256k1": "^4.0.2",
"tweetnacl": "^1.0.0"

I suggest switching to audited libraries that are faster, use less dependencies.

Supply chain attacks are very dangerous. Breaking any sub-dependency of your dependencies could leak user keys. So, the best option is to use projects that don't have sub-dependencies.


Individual tracking tasks

paulmillr commented 2 years ago

It also seems like tweetnacl has a security vulnerability, which is a separate issue.

steveluscher commented 2 years ago

Paul! Thanks for this. This is great, worthwhile work that I'll start on now. I had started down this road in solana-labs/solana#25014 - particularly eliminating @ethersproject/sha2 with a zero-dependency library – but never finished.

I'll start picking these off one by one. I have learned to expect significant challenges, such as:

I'll work through all these challenges as they come.

steveluscher commented 2 years ago

It also seems like tweetnacl has a security vulnerability…

The lockfile for @solana/web3.js specifies tweetnacl@1.0.3.

https://github.com/solana-labs/solana/blob/403b2e4841ef7301370e86a443ebe51f8ae512e0/web3.js/yarn.lock#L7345-L7347

Nevertheless, I'll update the deps for avoidance of doubt.

steveluscher commented 2 years ago

Immediately, when trying to replace tweetnacl with @noble/ed25519, I fell on my face in exactly the way I expected. All of our signing methods are synchronous, and the Noble library is async. There's simply no way to start with this:

class Transaction {
  sign(...signers: Array<Signer>): void;
}

Then replace the signing implementation

- const signature = nacl.sign.detached(signData, signer.secretKey);
+ const signature = await nobleEd25519.sign(signData, signer.secretKey);

…without having to change the signature to this:

   class Transaction {
-    sign(...signers: Array<Signer>): void;
+    sign(...signers: Array<Signer>): Promise<void>;
   }

You can always start with an async API and remove the genuinely async parts of it, but you can't go the other way without breaking the entire universe. Everyone who currently does this in @solana/web3.js:

function handleClick(): void {
  const t = new Transaction().add(/* ... */).sign(signer);
  /* ... */
}

Would have to change to this:

   function handleClick(): void {
-    const t = new Transaction().add(/* ... */).sign(signer);
+    const t = new Transaction().add(/* ... */);
+    await t.sign(signer);
     /* ... */
   }

…which in turn would force the outer method to become async…

- function handleClick(): void {
+ async function handleClick(): Promise<void> {
     const t = new Transaction().add(/* ... */);
     await t.sign(signer);
     /* ... */
  }

…which in turn would cause folks to have to completely rethink the lifecycle of their event handlers (eg. what happens if the event handler is halfway done but then the UI it's handling an event for unmounts?).

This is part of @noble/ed25519's genius; it achieves speed and small size by delegating hashing to the runtime's implementation of sha512 (ie. SubtleCrypto). This is incompatible with @solana/web3.js as it stands now for two reasons:

  1. SubtleCrypto is async. We're sync.
  2. SubtleCrypto is not available in React Native, and we need @solana/web3.js to run there.
paulmillr commented 2 years ago

It is the simplest and most reasonable way to implement no-dependency ed25519. Current consumers are fine with using it as-is. If you need synchronous api, see how noble-secp256k1 implemented both async and sync versions — it is still possible.

What happens if you'll decide to sign sol TX with hardware wallet? The procedure cannot be sync — since that can take indefinite amount of time.

paulmillr commented 2 years ago

As for

This is part of @noble/ed25519's genius; it achieves speed and small size by delegating hashing

it achieves speed by delegating hashing

False: sha512 is not a bottleneck — scalar multiplication is. Native sha512 is 2x slower than noble-hashes implementation. Noble-hashes on Apple M1 can still do 463,606 sha512 hashings of 32-byte input per second — one hashing every 2 microseconds, or ~4000 hashings per 1 120fps frame.

For comparison, noble-ed25519 sign() takes 261μs — 100x longer than sha512(). Your current tweetnacl's signing takes 8x longer than noble-hashes: 2ms per operation.

it achieves small size by delegating hashing

False: sha512 takes about 350 tree-shaken lines of code in noble-hashes. tweetnacl-fast is 2.3KLOC, while noble ed+sha512 is still 1.5KLOC.

steveluscher commented 2 years ago

It is the simplest and most reasonable way to implement no-dependency ed25519.

100% agree!

…see how noble-secp256k1 implemented both async and sync versions — it is still possible.

You're right! It's possible to inject a sync implementation with a modification like that. But then, we have to go find a userspace implementation of sha512. But then we lose all the benefits that @noble/ed255519 offers: we're slower again (because we're not using the native browser binding), and bigger (because we had to bundle a new dependency to do what SubtleCrypto does ‘for free’).

What happens if you'll decide to sign sol TX with hardware wallet?

This isn't something that @solana/web3.js is responsible for. The signing methods in web3.js are only useful where you have access to a private key. A hardware wallet never exfiltrates those keys, and it certainly doesn't run web3.js.

paulmillr commented 2 years ago

But then, we have to go find a userspace implementation of sha512

You don't have to find anything. noble-hashes is audited and it is there. You will need it in any case, to replace js-sha3, and other projects.

But then we lose all the benefits that @noble/ed255519 offers: we're slower again

secp with sha256 from noble-hashes is 15 microseconds slower than a native version. Proof:

sign x 4,974 ops/sec @ 201μs/op
signSync x 4,642 ops/sec @ 215μs/op

It is still 215μs, which is 9x faster than your current tweetnacl ed25519-sign.

and bigger (because we had to bundle a new dependency

You will need noble-hashes in any case. Or you can bundle its implementation of sha512, which is about 350 lines of code.

paulmillr commented 2 years ago

Want to reiterate: @noble/ed25519 could be easily made synchronous without noticeable performance penalty and without massive increase in bundle size. This just requires a few hours of work. If enough users commit to this, we could even do it upstream.

We have made the same thing in @noble/secp256k1 and it's up and running: see README (search for signSync).

paulmillr commented 2 years ago

@noble/ed25519 1.7.0 is out with sync support.

paulmillr commented 1 year ago

@steveluscher heads up, much smaller v2 of noble-secp, noble-ed is out. Many features have been removed, but should be fine for your case. The libraries are just 4kb gzipped now.

Another option is to use noble-curves which have improved security, are larger; but tree shaking is supported so all is cool.

steveluscher commented 1 year ago

Thanks, @paulmillr!

steveluscher commented 2 months ago

Closed by 5a20ed0649fd0fa28d4b6a96ba042c9cdb47630e.

github-actions[bot] commented 2 months ago

Because there has been no activity on this issue for 7 days since it was closed, it has been automatically locked. Please open a new issue if it requires a follow up.