Repository: algorand/js-algorand-sdk
Pull request #675 (2022-10-21 07:29:55)

fix: mergeMultisigTransactions logic error
This bug will manifest when the length of input array is > 2. Because all the mocha tests try to merge only two transactions it wasn't caught.

Essentially you have a for loop that iterates over all the transactions and at every iteration the `newSubsigs` array which should comprise all the pk and signatures from all the txs is overwritten with the the one from the reference case which is `multisigTxnBlobs[0]` + the ones from the current iteration of the loop. Thus when exiting the for loop the `newSubsigs` array is made of the signatures available in the reference tx + the last tx. All the other transactions signatures get overwritten.

It it is possible to fix this bug with a one-liner, right at the beginning of the map it would be enough to replace `current` with
```js
const current = newSubsigs[index];
```

However this whole function in general is a little clunky so I couldn't help refactoring slightly the `map` statement to avoid nested if statement and such. The for loop too can be instantiated at index 1 since index 0 is the base case calculated above. If you don't like style changes the one-liner above is enough to do the job.

AlgoDoggo: @barnjamin could I have a bug-fix label so cicd passes plz?
jasonpaulos: Hi @AlgoDoggo, thanks for making this PR. I only have one request: can you add a new test that merges > 2 multisigs, since you said that was missing from our test suite?

I need to take a closer look at the code changes to be absolutely sure they work as intended, and a new test would help with that effort (in addition to being a regression test).
scholtz: Please fix mergeMultisigTransactions .. it took us also quite some time that documented functionality does not work.
AlgoDoggo: Hey, I've refactored a test to merge three tx instead of two. You can run it solo against the old and new logic to see the diff.
AlgoDoggo: At some point we'll need to run typescript in strict mode, or at least with strictNullChecks enabled. A lot of values that can be undefined are just assumed to exist now by the sdk. It's bound to cause runtime errors eventually.
AlgoDoggo: @jasonpaulos Looks like cicd is failing at the `npm ci` step, chromedriver won't install. I'd try rerunning the workflows but I do not have permissions for this.
AlgoDoggo: ah nvm, the dev branch had moved.

Patch:
diff --git a/src/multisig.ts b/src/multisig.ts
index 245ac67..297c9a7 100644
--- a/src/multisig.ts
+++ b/src/multisig.ts
@@ -248,8 +248,8 @@ export function mergeMultisigTransactions(multisigTxnBlobs: Uint8Array[]) {
     address.fromMultisigPreImg(refPreImage)
   );
 
-  let newSubsigs = refSigTx.msig.subsig;
-  for (let i = 0; i < multisigTxnBlobs.length; i++) {
+  const newSubsigs = refSigTx.msig.subsig.map((sig) => ({ ...sig }));
+  for (let i = 1; i < multisigTxnBlobs.length; i++) {
     const unisig = encoding.decode(
       multisigTxnBlobs[i]
     ) as EncodedSignedTransaction;
@@ -281,32 +281,21 @@ export function mergeMultisigTransactions(multisigTxnBlobs: Uint8Array[]) {
     }
 
     // now, we can merge
-    newSubsigs = unisig.msig.subsig.map((uniSubsig, index) => {
-      const current = refSigTx.msig.subsig[index];
-      if (current.s) {
-        // we convert the Uint8Arrays uniSubsig.s and current.s to Buffers here because (as
-        // of Dec 2020) React overrides the buffer package with an older version that does
-        // not support Uint8Arrays in the comparison function. See this thread for more
-        // info: https://github.com/algorand/js-algorand-sdk/issues/252
-        if (
-          uniSubsig.s &&
-          Buffer.compare(Buffer.from(uniSubsig.s), Buffer.from(current.s)) !== 0
-        ) {
-          // mismatch
-          throw new Error(MULTISIG_MERGE_SIG_MISMATCH_ERROR_MSG);
-        }
-        return {
-          pk: current.pk,
-          s: current.s,
-        };
+    unisig.msig.subsig.forEach((uniSubsig, index) => {
+      if (!uniSubsig.s) return;
+      const current = newSubsigs[index];
+      // we convert the Uint8Arrays uniSubsig.s and current.s to Buffers here because (as
+      // of Dec 2020) React overrides the buffer package with an older version that does
+      // not support Uint8Arrays in the comparison function. See this thread for more
+      // info: https://github.com/algorand/js-algorand-sdk/issues/252
+      if (
+        current.s &&
+        Buffer.compare(Buffer.from(uniSubsig.s), Buffer.from(current.s)) !== 0
+      ) {
+        // mismatch
+        throw new Error(MULTISIG_MERGE_SIG_MISMATCH_ERROR_MSG);
       }
-      if (uniSubsig.s) {
-        return {
-          pk: current.pk,
-          s: uniSubsig.s,
-        };
-      }
-      return current;
+      current.s = uniSubsig.s;
     });
   }
   const msig: EncodedMultisig = {


Test patch:
diff --git a/tests/6.Multisig.ts b/tests/6.Multisig.ts
index 7dc7596..1efc9a0 100644
--- a/tests/6.Multisig.ts
+++ b/tests/6.Multisig.ts
@@ -410,6 +410,45 @@ describe('Multisig Functionality', () => {
       assert.deepStrictEqual(Buffer.from(finMsigBlobTwo), allThreeBlob);
     });
 
+    it('should merge several transactions', () => {
+      // prettier-ignore
+      const blobSignedByFirst = Buffer.from([ 130, 164, 109, 115, 105, 103, 131, 166, 115, 117, 98, 115, 105, 103, 147, 130, 162, 112, 107, 196, 32, 27, 126, 192, 176, 75, 234, 97, 183, 150, 144, 151, 230, 203, 244, 7, 225, 8, 167, 5, 53, 29, 11, 201, 138, 190, 177, 34, 9, 168, 171, 129, 120, 161, 115, 196, 64, 113, 61, 44, 215, 188, 9, 110, 249, 243, 107, 227, 105, 200, 124, 12, 209, 21, 155, 67, 225, 240, 42, 107, 19, 212, 242, 236, 251, 14, 157, 232, 202, 93, 76, 125, 237, 173, 175, 178, 41, 145, 52, 43, 74, 132, 202, 20, 132, 237, 142, 122, 248, 31, 104, 105, 253, 168, 172, 70, 227, 115, 249, 227, 13, 129, 162, 112, 107, 196, 32, 9, 99, 50, 9, 83, 115, 137, 240, 117, 103, 17, 119, 57, 145, 199, 208, 62, 27, 115, 200, 196, 245, 43, 246, 175, 240, 26, 162, 92, 249, 194, 113, 129, 162, 112, 107, 196, 32, 231, 240, 248, 77, 6, 129, 29, 249, 243, 28, 141, 135, 139, 17, 85, 244, 103, 29, 81, 161, 133, 194, 0, 144, 134, 103, 244, 73, 88, 112, 104, 161, 163, 116, 104, 114, 2, 161, 118, 1, 163, 116, 120, 110, 140, 163, 102, 101, 101, 206, 0, 3, 200, 192, 162, 102, 118, 206, 0, 14, 249, 218, 163, 103, 101, 110, 172, 100, 101, 118, 110, 101, 116, 45, 118, 51, 56, 46, 48, 162, 103, 104, 196, 32, 254, 179, 108, 57, 16, 20, 57, 0, 195, 218, 85, 66, 202, 24, 54, 176, 15, 210, 248, 25, 89, 18, 87, 205, 35, 246, 4, 47, 152, 200, 54, 157, 162, 108, 118, 206, 0, 14, 253, 194, 166, 115, 101, 108, 107, 101, 121, 196, 32, 50, 18, 43, 43, 214, 61, 220, 83, 49, 150, 23, 165, 170, 83, 196, 177, 194, 111, 227, 220, 202, 242, 141, 54, 34, 181, 105, 119, 161, 64, 92, 134, 163, 115, 110, 100, 196, 32, 141, 146, 180, 137, 144, 1, 115, 160, 77, 250, 67, 89, 163, 102, 106, 106, 252, 234, 44, 66, 160, 93, 217, 193, 247, 62, 235, 165, 71, 128, 55, 233, 164, 116, 121, 112, 101, 166, 107, 101, 121, 114, 101, 103, 167, 118, 111, 116, 101, 102, 115, 116, 206, 0, 13, 187, 160, 166, 118, 111, 116, 101, 107, 100, 205, 39, 16, 167, 118, 111, 116, 101, 107, 101, 121, 196, 32, 112, 27, 215, 251, 145, 43, 7, 179, 8, 17, 255, 40, 29, 159, 238, 149, 99, 229, 128, 46, 32, 38, 137, 35, 25, 37, 143, 119, 250, 147, 30, 136, 167, 118, 111, 116, 101, 108, 115, 116, 206, 0, 15, 66, 64, ]);
+      // prettier-ignore
+      const blobSignedBySecond = Buffer.from([ 130, 164, 109, 115, 105, 103, 131, 166, 115, 117, 98, 115, 105, 103, 147, 129, 162, 112, 107, 196, 32, 27, 126, 192, 176, 75, 234, 97, 183, 150, 144, 151, 230, 203, 244, 7, 225, 8, 167, 5, 53, 29, 11, 201, 138, 190, 177, 34, 9, 168, 171, 129, 120, 130, 162, 112, 107, 196, 32, 9, 99, 50, 9, 83, 115, 137, 240, 117, 103, 17, 119, 57, 145, 199, 208, 62, 27, 115, 200, 196, 245, 43, 246, 175, 240, 26, 162, 92, 249, 194, 113, 161, 115, 196, 64, 227, 199, 17, 26, 50, 149, 36, 250, 241, 222, 56, 188, 127, 140, 131, 144, 167, 224, 18, 230, 61, 37, 113, 136, 156, 116, 104, 237, 140, 138, 121, 215, 140, 159, 38, 64, 106, 111, 177, 108, 51, 233, 152, 250, 233, 207, 138, 116, 61, 55, 89, 204, 6, 164, 2, 114, 128, 230, 199, 130, 136, 25, 207, 1, 129, 162, 112, 107, 196, 32, 231, 240, 248, 77, 6, 129, 29, 249, 243, 28, 141, 135, 139, 17, 85, 244, 103, 29, 81, 161, 133, 194, 0, 144, 134, 103, 244, 73, 88, 112, 104, 161, 163, 116, 104, 114, 2, 161, 118, 1, 163, 116, 120, 110, 140, 163, 102, 101, 101, 206, 0, 3, 200, 192, 162, 102, 118, 206, 0, 14, 249, 218, 163, 103, 101, 110, 172, 100, 101, 118, 110, 101, 116, 45, 118, 51, 56, 46, 48, 162, 103, 104, 196, 32, 254, 179, 108, 57, 16, 20, 57, 0, 195, 218, 85, 66, 202, 24, 54, 176, 15, 210, 248, 25, 89, 18, 87, 205, 35, 246, 4, 47, 152, 200, 54, 157, 162, 108, 118, 206, 0, 14, 253, 194, 166, 115, 101, 108, 107, 101, 121, 196, 32, 50, 18, 43, 43, 214, 61, 220, 83, 49, 150, 23, 165, 170, 83, 196, 177, 194, 111, 227, 220, 202, 242, 141, 54, 34, 181, 105, 119, 161, 64, 92, 134, 163, 115, 110, 100, 196, 32, 141, 146, 180, 137, 144, 1, 115, 160, 77, 250, 67, 89, 163, 102, 106, 106, 252, 234, 44, 66, 160, 93, 217, 193, 247, 62, 235, 165, 71, 128, 55, 233, 164, 116, 121, 112, 101, 166, 107, 101, 121, 114, 101, 103, 167, 118, 111, 116, 101, 102, 115, 116, 206, 0, 13, 187, 160, 166, 118, 111, 116, 101, 107, 100, 205, 39, 16, 167, 118, 111, 116, 101, 107, 101, 121, 196, 32, 112, 27, 215, 251, 145, 43, 7, 179, 8, 17, 255, 40, 29, 159, 238, 149, 99, 229, 128, 46, 32, 38, 137, 35, 25, 37, 143, 119, 250, 147, 30, 136, 167, 118, 111, 116, 101, 108, 115, 116, 206, 0, 15, 66, 64, ]);
+      // prettier-ignore
+      const blobSignedByThird = Buffer.from([ 130, 164, 109, 115, 105, 103, 131, 166, 115, 117, 98, 115, 105, 103, 147, 129, 162, 112, 107, 196, 32, 27, 126, 192, 176, 75, 234, 97, 183, 150, 144, 151, 230, 203, 244, 7, 225, 8, 167, 5, 53, 29, 11, 201, 138, 190, 177, 34, 9, 168, 171, 129, 120, 129, 162, 112, 107, 196, 32, 9, 99, 50, 9, 83, 115, 137, 240, 117, 103, 17, 119, 57, 145, 199, 208, 62, 27, 115, 200, 196, 245, 43, 246, 175, 240, 26, 162, 92, 249, 194, 113, 130, 162, 112, 107, 196, 32, 231, 240, 248, 77, 6, 129, 29, 249, 243, 28, 141, 135, 139, 17, 85, 244, 103, 29, 81, 161, 133, 194, 0, 144, 134, 103, 244, 73, 88, 112, 104, 161, 161, 115, 196, 64, 125, 39, 23, 127, 48, 75, 104, 78, 54, 53, 177, 125, 33, 29, 42, 49, 173, 205, 5, 9, 225, 99, 169, 16, 177, 95, 186, 134, 218, 129, 179, 15, 219, 90, 103, 54, 188, 25, 90, 235, 144, 137, 45, 35, 66, 53, 45, 183, 232, 27, 20, 50, 91, 219, 194, 187, 55, 146, 113, 126, 233, 186, 161, 15, 163, 116, 104, 114, 2, 161, 118, 1, 163, 116, 120, 110, 140, 163, 102, 101, 101, 206, 0, 3, 200, 192, 162, 102, 118, 206, 0, 14, 249, 218, 163, 103, 101, 110, 172, 100, 101, 118, 110, 101, 116, 45, 118, 51, 56, 46, 48, 162, 103, 104, 196, 32, 254, 179, 108, 57, 16, 20, 57, 0, 195, 218, 85, 66, 202, 24, 54, 176, 15, 210, 248, 25, 89, 18, 87, 205, 35, 246, 4, 47, 152, 200, 54, 157, 162, 108, 118, 206, 0, 14, 253, 194, 166, 115, 101, 108, 107, 101, 121, 196, 32, 50, 18, 43, 43, 214, 61, 220, 83, 49, 150, 23, 165, 170, 83, 196, 177, 194, 111, 227, 220, 202, 242, 141, 54, 34, 181, 105, 119, 161, 64, 92, 134, 163, 115, 110, 100, 196, 32, 141, 146, 180, 137, 144, 1, 115, 160, 77, 250, 67, 89, 163, 102, 106, 106, 252, 234, 44, 66, 160, 93, 217, 193, 247, 62, 235, 165, 71, 128, 55, 233, 164, 116, 121, 112, 101, 166, 107, 101, 121, 114, 101, 103, 167, 118, 111, 116, 101, 102, 115, 116, 206, 0, 13, 187, 160, 166, 118, 111, 116, 101, 107, 100, 205, 39, 16, 167, 118, 111, 116, 101, 107, 101, 121, 196, 32, 112, 27, 215, 251, 145, 43, 7, 179, 8, 17, 255, 40, 29, 159, 238, 149, 99, 229, 128, 46, 32, 38, 137, 35, 25, 37, 143, 119, 250, 147, 30, 136, 167, 118, 111, 116, 101, 108, 115, 116, 206, 0, 15, 66, 64, ]);
+      // prettier-ignore
+      const blobSignedByAllThree = Buffer.from([130, 164, 109, 115, 105, 103, 131, 166, 115, 117, 98, 115, 105, 103, 147, 130, 162, 112, 107, 196, 32, 27, 126, 192, 176, 75, 234, 97, 183, 150, 144, 151, 230, 203, 244, 7, 225, 8, 167, 5, 53, 29, 11, 201, 138, 190, 177, 34, 9, 168, 171, 129, 120, 161, 115, 196, 64, 113, 61, 44, 215, 188, 9, 110, 249, 243, 107, 227, 105, 200, 124, 12, 209, 21, 155, 67, 225, 240, 42, 107, 19, 212, 242, 236, 251, 14, 157, 232, 202, 93, 76, 125, 237, 173, 175, 178, 41, 145, 52, 43, 74, 132, 202, 20, 132, 237, 142, 122, 248, 31, 104, 105, 253, 168, 172, 70, 227, 115, 249, 227, 13, 130, 162, 112, 107, 196, 32, 9, 99, 50, 9, 83, 115, 137, 240, 117, 103, 17, 119, 57, 145, 199, 208, 62, 27, 115, 200, 196, 245, 43, 246, 175, 240, 26, 162, 92, 249, 194, 113, 161, 115, 196, 64, 227, 199, 17, 26, 50, 149, 36, 250, 241, 222, 56, 188, 127, 140, 131, 144, 167, 224, 18, 230, 61, 37, 113, 136, 156, 116, 104, 237, 140, 138, 121, 215, 140, 159, 38, 64, 106, 111, 177, 108, 51, 233, 152, 250, 233, 207, 138, 116, 61, 55, 89, 204, 6, 164, 2, 114, 128, 230, 199, 130, 136, 25, 207, 1, 130, 162, 112, 107, 196, 32, 231, 240, 248, 77, 6, 129, 29, 249, 243, 28, 141, 135, 139, 17, 85, 244, 103, 29, 81, 161, 133, 194, 0, 144, 134, 103, 244, 73, 88, 112, 104, 161, 161, 115, 196, 64, 125, 39, 23, 127, 48, 75, 104, 78, 54, 53, 177, 125, 33, 29, 42, 49, 173, 205, 5, 9, 225, 99, 169, 16, 177, 95, 186, 134, 218, 129, 179, 15, 219, 90, 103, 54, 188, 25, 90, 235, 144, 137, 45, 35, 66, 53, 45, 183, 232, 27, 20, 50, 91, 219, 194, 187, 55, 146, 113, 126, 233, 186, 161, 15, 163, 116, 104, 114, 2, 161, 118, 1, 163, 116, 120, 110, 140, 163, 102, 101, 101, 206, 0, 3, 200, 192, 162, 102, 118, 206, 0, 14, 249, 218, 163, 103, 101, 110, 172, 100, 101, 118, 110, 101, 116, 45, 118, 51, 56, 46, 48, 162, 103, 104, 196, 32, 254, 179, 108, 57, 16, 20, 57, 0, 195, 218, 85, 66, 202, 24, 54, 176, 15, 210, 248, 25, 89, 18, 87, 205, 35, 246, 4, 47, 152, 200, 54, 157, 162, 108, 118, 206, 0, 14, 253, 194, 166, 115, 101, 108, 107, 101, 121, 196, 32, 50, 18, 43, 43, 214, 61, 220, 83, 49, 150, 23, 165, 170, 83, 196, 177, 194, 111, 227, 220, 202, 242, 141, 54, 34, 181, 105, 119, 161, 64, 92, 134, 163, 115, 110, 100, 196, 32, 141, 146, 180, 137, 144, 1, 115, 160, 77, 250, 67, 89, 163, 102, 106, 106, 252, 234, 44, 66, 160, 93, 217, 193, 247, 62, 235, 165, 71, 128, 55, 233, 164, 116, 121, 112, 101, 166, 107, 101, 121, 114, 101, 103, 167, 118, 111, 116, 101, 102, 115, 116, 206, 0, 13, 187, 160, 166, 118, 111, 116, 101, 107, 100, 205, 39, 16, 167, 118, 111, 116, 101, 107, 101, 121, 196, 32, 112, 27, 215, 251, 145, 43, 7, 179, 8, 17, 255, 40, 29, 159, 238, 149, 99, 229, 128, 46, 32, 38, 137, 35, 25, 37, 143, 119, 250, 147, 30, 136, 167, 118, 111, 116, 101, 108, 115, 116, 206, 0, 15, 66, 64]);
+
+      const finMsigBlob = algosdk.mergeMultisigTransactions([
+        new Uint8Array(blobSignedByFirst),
+        new Uint8Array(blobSignedBySecond),
+        new Uint8Array(blobSignedByThird),
+      ]);
+      const finMsigBlobTwo = algosdk.mergeMultisigTransactions([
+        new Uint8Array(blobSignedByThird),
+        new Uint8Array(blobSignedByFirst),
+        new Uint8Array(blobSignedBySecond),
+      ]);
+
+      // let's open those to verify all sigs are there
+      let { msig } = algosdk.decodeSignedTransaction(finMsigBlob);
+      assert(
+        msig && msig.subsig.every((sig) => ArrayBuffer.isView(sig.s)),
+        'missing signatures'
+      );
+
+      ({ msig } = algosdk.decodeSignedTransaction(finMsigBlobTwo));
+      assert(
+        msig && msig.subsig.every((sig) => ArrayBuffer.isView(sig.s)),
+        'missing signatures'
+      );
+
+      // let's check the merged transactions against our reference blob
+      assert.deepStrictEqual(Buffer.from(finMsigBlob), blobSignedByAllThree);
+      assert.deepStrictEqual(Buffer.from(finMsigBlobTwo), blobSignedByAllThree);
+    });
+
     it('should correctly handle a different sender', () => {
       const oneAndThreeBlob = Buffer.from(
         'g6Rtc2lng6ZzdWJzaWeTgqJwa8QgG37AsEvqYbeWkJfmy/QH4QinBTUdC8mKvrEiCairgXihc8RAWLcyn6nB68yo/PUHXB21wyTy+PhHgMQNOIXPTGB96faZP2xqpQ8IFlIR2LPotlX68ylK8MCl82SUfMR4FYyzAIGicGvEIAljMglTc4nwdWcRdzmRx9A+G3PIxPUr9q/wGqJc+cJxgqJwa8Qg5/D4TQaBHfnzHI2HixFV9GcdUaGFwgCQhmf0SVhwaKGhc8RAQ3t8lAipybLbWaRa3EDKl0mLGwUpYzl0iBCHSflhJM6zboJiT10FnWtXqW0dUBzpj9yeqtSuSJyKb8Ml3YJkCqN0aHICoXYBpHNnbnLEII2StImQAXOgTfpDWaNmamr86ixCoF3Zwfc+66VHgDfpo3R4boujYW10zQPopWNsb3NlxCBA6TSSiCVky86cWaabZ1Qmiemhw6Kp6ltlpuikQh/8V6NmZWXNA+iiZnbN8xWjZ2VurGRldm5ldC12MzguMKJnaMQg/rNsORAUOQDD2lVCyhg2sA/S+BlZElfNI/YEL5jINp2ibHbN9v2kbm90ZcQIRSYiABhShvujcmN2xCB7bOJP61uswLFk4pwiLFf19j3Dh9Q5BIJYQRxf4Q98AqNzbmTEICHMyArhQUJN+4MirQrVopinURoL2GOodR8RZxvZFZl5pHR5cGWjcGF5',