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

unhandled exception introduced in v4.0.9 #74

Closed
shamilovtim opened this issue Dec 7, 2023 · 7 comments · Fixed by #75
Closed

unhandled exception introduced in v4.0.9 #74

shamilovtim opened this issue Dec 7, 2023 · 7 comments · Fixed by #75

Comments

@shamilovtim
Copy link

shamilovtim commented Dec 7, 2023

Prior to v4.0.9 if plain Arrays were passed into uint8arrays's own concat it would convert those Arrays into a Uint8Array using the asUint8Array helper.

In v4.0.9 a change was introduced such that:

return asUint8Array(globalThis.Buffer.concat(arrays, length))

The problem is that globalThis.Buffer.concat(arrays, length) is passing plain arrays into Buffer.concat before asUint8Array ever has a chance to convert them, which throws an unhandled exception:

Error: [TypeError: "list" argument must be an Array of Buffers]

The fix would either be to revert the change in v4.0.9 or to convert each non-buffer into a Uint8Array first prior to calling globalThis.Buffer.concat(arrays, length). I assume any performance gains will need to be re-evaluated.

@achingbrain
Copy link
Owner

A fair point, I should probably revert the change and re-release as a major, sorry for the disruption.

@shamilovtim
Copy link
Author

@achingbrain before you do publish again as a major I should point out how the plain arrays are getting in. the stack trace is below. I think they're entering through multihashes.encode(). does ion sdk need to be migrated to multiformats?

Buffer.concat()
uint8Arrays.concat.concat() 
multihashes.encode() // here I think
@decentralized-identity/ion-sdk.Multihash.hash 

@achingbrain
Copy link
Owner

It's already gone out as v5.

I think the problem is here - IIRC the varint.encode function returns a number[] if you don't pass a buffer in for it to write into.

@achingbrain
Copy link
Owner

achingbrain commented Dec 7, 2023

Can you upgrade to multiformats instead of using multihashes?

It supports the same suite of multihash prefixes but has the advantage of actually being maintained...

The multihash section of the readme shows how to use it: https://www.npmjs.com/package/multiformats#multihash-hashers

@shamilovtim
Copy link
Author

Can you upgrade to multiformats instead of using multihashes?

It supports the same suite of multihash prefixes but has the advantage of actually being maintained...

The multihash section of the readme shows how to use it: https://www.npmjs.com/package/multiformats#multihash-hashers

Happy to do it but I don't think I have write access in ion sdk. I made a ticket. I think @thehenrytsai or @csuwildcat could do it

@shamilovtim
Copy link
Author

@achingbrain I went ahead and tried to migrate that package but I'm seeing a diff in the hashes produced in the test suite.

diff --git a/lib/Multihash.ts b/lib/Multihash.ts
index 6fbea78..8563850 100644
--- a/lib/Multihash.ts
+++ b/lib/Multihash.ts
@@ -1,10 +1,9 @@
-import * as multihashes from 'multihashes';
 import Encoder from './Encoder.js';
 import ErrorCode from './ErrorCode.js';
-import { HashCode } from 'multihashes';
 import IonError from './IonError.js';
 import IonSdkConfig from './IonSdkConfig.js';
 import JsonCanonicalizer from './JsonCanonicalizer.js';
+import { decode } from 'multiformats/hashes/digest';
 import { sha256 } from 'multiformats/hashes/sha2';

 /**
@@ -17,9 +16,18 @@ export default class Multihash {
    */
   public static async hash (content: Uint8Array, hashAlgorithmInMultihashCode: number): Promise<Uint8Array> {
     const conventionalHash = await this.hashAsNonMultihashBytes(content, hashAlgorithmInMultihashCode);
-    const multihash = multihashes.encode(conventionalHash, hashAlgorithmInMultihashCode as HashCode);
-
-    return multihash;
+
+    let bytes: Uint8Array;
+    switch (hashAlgorithmInMultihashCode) {
+      case 18: // SHA256
+        let hasher = await sha256.digest(conventionalHash);
+        bytes = hasher.bytes;
+        break;
+      default:
+        throw new Error(`Unsupported hash algorithm code: ${hashAlgorithmInMultihashCode}`);
+    }
+
+    return bytes;
   }

   /**
@@ -81,13 +89,13 @@ export default class Multihash {
    * Checks if the given encoded hash is a multihash computed using the configured hashing algorithm.
    */
   public static validateEncodedHashComputedUsingSupportedHashAlgorithm (
-    encodedMultihash: string,
+    encodedMultihash: string, // didSuffix
     inputContextForErrorLogging: string
   ) {
     let multihash;
     const multihashBytes = Encoder.decodeAsBytes(encodedMultihash, inputContextForErrorLogging);
     try {
-      multihash = multihashes.decode(multihashBytes);
+      multihash = decode(multihashBytes);
     } catch {
       throw new IonError(
         ErrorCode.MultihashStringNotAMultihash,

Diffs produced by the test suite:

1) IonDid createLongFormDid() vector test - should create a long-form DID correctly.
  Message:
    Expected 'did:ion:EiBBU-mEzHZPNzcFb0csMRpGwlz8AKkXJkVX5IlKRdPruQ:eyJkZWx0YSI6eyJwYXRjaGVzIjpbeyJhY3Rpb24iOiJyZXBsYWNlIiwiZG9jdW1lbnQiOnsicHVibGljS2V5cyI6W3siaWQiOiJwdWJsaWNLZXlNb2RlbDFJZCIsInB1YmxpY0tleUp3ayI6eyJjcnYiOiJzZWNwMjU2azEiLCJrdHkiOiJFQyIsIngiOiJ0WFNLQl9ydWJYUzdzQ2pYcXVwVkpFelRjVzNNc2ptRXZxMVlwWG45NlpnIiwieSI6ImRPaWNYcWJqRnhvR0otSzAtR0oxa0hZSnFpY19EX09NdVV3a1E3T2w2bmsifSwicHVycG9zZXMiOlsiYXV0aGVudGljYXRpb24iLCJrZXlBZ3JlZW1lbnQiXSwidHlwZSI6IkVjZHNhU2VjcDI1NmsxVmVyaWZpY2F0aW9uS2V5MjAxOSJ9XSwic2VydmljZXMiOlt7ImlkIjoic2VydmljZTFJZCIsInNlcnZpY2VFbmRwb2ludCI6Imh0dHA6Ly93d3cuc2VydmljZTEuY29tIiwidHlwZSI6InNlcnZpY2UxVHlwZSJ9XX19XSwidXBkYXRlQ29tbWl0bWVudCI6IkVpQnRLZ3FoSU02ZkkzSmtxU3BoRjUxZEFpaFF3UXVURHhidDFhQjd4WkU2aHcifSwic3VmZml4RGF0YSI6eyJkZWx0YUhhc2giOiJFaUJwZGo0SGFGYThYSXlSTzF3NEg1ancyekNXTkpDRm1ZdUFpaTlXMnVGbUxRIiwicmVjb3ZlcnlDb21taXRtZW50IjoiRWlCWV92aVdRTmJDWGZyZ1NfRTZZUU0zR1MxWjJqTmdmOHJ2ZnFMcWRpd05ZQSJ9fQ' to equal 'did:ion:EiDyOQbbZAa3aiRzeCkV7LOx3SERjjH93EXoIM3UoN4oWg:eyJkZWx0YSI6eyJwYXRjaGVzIjpbeyJhY3Rpb24iOiJyZXBsYWNlIiwiZG9jdW1lbnQiOnsicHVibGljS2V5cyI6W3siaWQiOiJwdWJsaWNLZXlNb2RlbDFJZCIsInB1YmxpY0tleUp3ayI6eyJjcnYiOiJzZWNwMjU2azEiLCJrdHkiOiJFQyIsIngiOiJ0WFNLQl9ydWJYUzdzQ2pYcXVwVkpFelRjVzNNc2ptRXZxMVlwWG45NlpnIiwieSI6ImRPaWNYcWJqRnhvR0otSzAtR0oxa0hZSnFpY19EX09NdVV3a1E3T2w2bmsifSwicHVycG9zZXMiOlsiYXV0aGVudGljYXRpb24iLCJrZXlBZ3JlZW1lbnQiXSwidHlwZSI6IkVjZHNhU2VjcDI1NmsxVmVyaWZpY2F0aW9uS2V5MjAxOSJ9XSwic2VydmljZXMiOlt7ImlkIjoic2VydmljZTFJZCIsInNlcnZpY2VFbmRwb2ludCI6Imh0dHA6Ly93d3cuc2VydmljZTEuY29tIiwidHlwZSI6InNlcnZpY2UxVHlwZSJ9XX19XSwidXBkYXRlQ29tbWl0bWVudCI6IkVpREtJa3dxTzY5SVBHM3BPbEhrZGI4Nm5ZdDBhTnhTSFp1MnItYmhFem5qZEEifSwic3VmZml4RGF0YSI6eyJkZWx0YUhhc2giOiJFaUNmRFdSbllsY0Q5RUdBM2RfNVoxQUh1LWlZcU1iSjluZmlxZHo1UzhWRGJnIiwicmVjb3ZlcnlDb21taXRtZW50IjoiRWlCZk9aZE10VTZPQnc4UGs4NzlRdFotMkotOUZiYmpTWnlvYUFfYnFENHpoQSJ9fQ'.
  Stack:
        at <Jasmine>
        at file:///Users/admin/Documents/Development/block/ion-sdk/dist/tests/IonDid.spec.js:35:33
        at Generator.next (<anonymous>)
        at fulfilled (file:///Users/admin/Documents/Development/block/ion-sdk/dist/tests/IonDid.spec.js:4:58)

2) IonRequest createCreateRequest should generate a create request with desired arguments
  Message:
    Expected 'EiBtKgqhIM6fI3JkqSphF51dAihQwQuTDxbt1aB7xZE6hw' to equal 'EiDKIkwqO69IPG3pOlHkdb86nYt0aNxSHZu2r-bhEznjdA'.
  Stack:
        at <Jasmine>
        at file:///Users/admin/Documents/Development/block/ion-sdk/dist/tests/IonRequest.spec.js:35:51
        at Generator.next (<anonymous>)
        at fulfilled (file:///Users/admin/Documents/Development/block/ion-sdk/dist/tests/IonRequest.spec.js:4:58)

3) IonRequest createUpdateRequest should generate an update request with the given arguments
  Message:
    Expected 'EiBfOZdMtU6OBw8Pk879QtZ-2J-9FbbjSZyoaA_bqD4zhA' to equal 'EiAJ-97Is59is6FKAProwDo870nmwCeP8n5nRRFwPpUZVQ'.
  Stack:
        at <Jasmine>
        at file:///Users/admin/Documents/Development/block/ion-sdk/dist/tests/IonRequest.spec.js:60:40
        at Generator.next (<anonymous>)
        at fulfilled (file:///Users/admin/Documents/Development/block/ion-sdk/dist/tests/IonRequest.spec.js:4:58)

4) IonRequest createRecoverRequest should generate a recover request with given arguments
  Message:
    Expected 'EiBfOZdMtU6OBw8Pk879QtZ-2J-9FbbjSZyoaA_bqD4zhA' to equal 'EiAJ-97Is59is6FKAProwDo870nmwCeP8n5nRRFwPpUZVQ'.
  Stack:
        at <Jasmine>
        at file:///Users/admin/Documents/Development/block/ion-sdk/dist/tests/IonRequest.spec.js:95:40
        at Generator.next (<anonymous>)
        at fulfilled (file:///Users/admin/Documents/Development/block/ion-sdk/dist/tests/IonRequest.spec.js:4:58)

5) IonRequest createDeactivateRequest should generate a deactivate request with the given arguments
  Message:
    Expected 'EiBfOZdMtU6OBw8Pk879QtZ-2J-9FbbjSZyoaA_bqD4zhA' to equal 'EiAJ-97Is59is6FKAProwDo870nmwCeP8n5nRRFwPpUZVQ'.
  Stack:
        at <Jasmine>
        at file:///Users/admin/Documents/Development/block/ion-sdk/dist/tests/IonRequest.spec.js:111:40
        at Generator.next (<anonymous>)
        at fulfilled (file:///Users/admin/Documents/Development/block/ion-sdk/dist/tests/IonRequest.spec.js:4:58)

@achingbrain
Copy link
Owner

achingbrain commented Dec 8, 2023

It looks like you are double-hashing - multihashes.encode just prefixes the digest with the length/code, sha256.digest actually hashes the incoming data and .bytes is the hash prefixed with the length/code.

So instead of:

const conventionalHash = await this.hashAsNonMultihashBytes(content, hashAlgorithmInMultihashCode);
const multihash = multihashes.encode(conventionalHash, hashAlgorithmInMultihashCode as HashCode);

return multihash;

You'd just do:

let multihash
switch (hashAlgorithmInMultihashCode) {
  case sha256.code:
    multihash = await sha256.digest(content);
    return multihash.bytes;
  default:
    throw new Error(`Unsupported hash algorithm code: ${hashAlgorithmInMultihashCode}`);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants