Irys-xyz / arbundles

Node.js implementation of ANS-104
https://irys.xyz
Apache License 2.0
131 stars 49 forks source link

ArconnectSigner should sign with saltLength 32 #75

Closed jim-toth closed 1 year ago

jim-toth commented 1 year ago

https://github.com/Bundlr-Network/arbundles/blob/master/src/signing/chains/arconnectSigner.ts#L31

tl;dr saltLength should be 32

This issue isn't obvious unless you try to sign "large" files. For example, attempting to sign a bundle of varying sized files (ranging from several kb to ~17mb), all data items except the 17mb were able to be verified. The 17mb file got a signature (i.e. the dummy id was replaced), but failed data item verification due to invalid signature. Changing saltLength to 32 fixes this. To reiterate, you have to test with a "large enough" file as saltLength 0 signed "small files" will have verifiable signatures.

One thing to note is that any non-zero saltLength means signing the same data twice in a row will result in different signatures, although both will be valid and verifiable. We have used saltLength 32 on art by city for ~1.5 years with no issue, creators have uploaded larger files than the example above.

Why doesn't saltLength 0 work for big files? I'm not sure, but I know that's what it is supposed to be 😄 I probably referenced arconnect or arweavejs internals to discover this.

Edit: here ya go https://github.com/ArweaveTeam/arweave-js/blob/997677dfe2ba722ac6abc3e075a8dd00313e4a6d/src/common/lib/crypto/webcrypto-driver.ts#L56

JesseTheRobot commented 1 year ago

@jim-toth thanks for the bug report! This is a weird issue for sure, I'll release a fix for this shortly.