fix: ensure signatures are ordered correctly (#12165)

web3.js/src/message.js

@@ -83,6 +83,16 @@ export class Message {
     );
   }
 
+  findSignerIndex(signer: PublicKey): number {
+    const index = this.accountKeys.findIndex(accountKey => {
+      return accountKey.equals(signer);
+    });
+    if (index < 0) {
+      throw new Error(`unknown signer: ${signer.toString()}`);
+    }
+    return index;
+  }
+
   serialize(): Buffer {
     const numKeys = this.accountKeys.length;
 

web3.js/src/transaction.js

@@ -220,14 +220,11 @@ export class Transaction {
       throw new Error('Transaction feePayer required');
     }
 
-    let numReadonlySignedAccounts = 0;
-    let numReadonlyUnsignedAccounts = 0;
-
     const programIds: string[] = [];
     const accountMetas: AccountMeta[] = [];
     this.instructions.forEach(instruction => {
       instruction.keys.forEach(accountMeta => {
-        accountMetas.push(accountMeta);
+        accountMetas.push({...accountMeta});
       });
 
       const programId = instruction.programId.toString();
@@ -268,16 +265,17 @@ export class Transaction {
       }
     });
 
-    // Move payer to the front and append other unknown signers as read-only
+    // Move payer to the front and disallow unknown signers
     this.signatures.forEach((signature, signatureIndex) => {
       const isPayer = signatureIndex === 0;
       const uniqueIndex = uniqueMetas.findIndex(x => {
         return x.pubkey.equals(signature.publicKey);
       });
       if (uniqueIndex > -1) {
-        if (isPayer && uniqueIndex !== 0) {
+        if (isPayer) {
           const [payerMeta] = uniqueMetas.splice(uniqueIndex, 1);
           payerMeta.isSigner = true;
+          payerMeta.isWritable = true;
           uniqueMetas.unshift(payerMeta);
         } else {
           uniqueMetas[uniqueIndex].isSigner = true;
@@ -289,23 +287,22 @@ export class Transaction {
           isWritable: true,
         });
       } else {
-        uniqueMetas.push({
+        throw new Error(`unknown signer: ${signature.publicKey.toString()}`);
-          pubkey: signature.publicKey,
-          isSigner: true,
-          isWritable: false,
-        });
       }
     });
 
-    // Split out signing from non-signing keys and count read-only keys
+    let numRequiredSignatures = 0;
+    let numReadonlySignedAccounts = 0;
+    let numReadonlyUnsignedAccounts = 0;
+
+    // Split out signing from non-signing keys and count header values
     const signedKeys: string[] = [];
     const unsignedKeys: string[] = [];
     uniqueMetas.forEach(({pubkey, isSigner, isWritable}) => {
       if (isSigner) {
-        // Promote the first signer to writable as it is the fee payer
-        const first = signedKeys.length === 0;
         signedKeys.push(pubkey.toString());
-        if (!first && !isWritable) {
+        numRequiredSignatures += 1;
+        if (!isWritable) {
           numReadonlySignedAccounts += 1;
         }
       } else {
@@ -316,6 +313,10 @@ export class Transaction {
       }
     });
 
+    if (numRequiredSignatures !== this.signatures.length) {
+      throw new Error('missing signer(s)');
+    }
+
     const accountKeys = signedKeys.concat(unsignedKeys);
     const instructions: CompiledInstruction[] = this.instructions.map(
       instruction => {
@@ -337,7 +338,7 @@ export class Transaction {
 
     return new Message({
       header: {
-        numRequiredSignatures: signedKeys.length,
+        numRequiredSignatures,
         numReadonlySignedAccounts,
         numReadonlyUnsignedAccounts,
       },
@@ -365,7 +366,18 @@ export class Transaction {
       throw new Error('No signers');
     }
 
-    this.signatures = signers.map(publicKey => ({signature: null, publicKey}));
+    const seen = new Set();
+    this.signatures = signers
+      .filter(publicKey => {
+        const key = publicKey.toString();
+        if (seen.has(key)) {
+          return false;
+        } else {
+          seen.add(key);
+          return true;
+        }
+      })
+      .map(publicKey => ({signature: null, publicKey}));
   }
 
   /**
@@ -385,10 +397,21 @@ export class Transaction {
       throw new Error('No signers');
     }
 
-    this.signatures = signers.map(signer => ({
+    const seen = new Set();
-      signature: null,
+    this.signatures = signers
-      publicKey: signer.publicKey,
+      .filter(signer => {
-    }));
+        const key = signer.publicKey.toString();
+        if (seen.has(key)) {
+          return false;
+        } else {
+          seen.add(key);
+          return true;
+        }
+      })
+      .map(signer => ({
+        signature: null,
+        publicKey: signer.publicKey,
+      }));
 
     this.partialSign(...signers);
   }
@@ -404,7 +427,14 @@ export class Transaction {
       throw new Error('No signers');
     }
 
-    const signData = this.serializeMessage();
+    const message = this.compileMessage();
+    this.signatures.sort(function (x, y) {
+      const xIndex = message.findSignerIndex(x.publicKey);
+      const yIndex = message.findSignerIndex(y.publicKey);
+      return xIndex < yIndex ? -1 : 1;
+    });
+
+    const signData = message.serialize();
     signers.forEach(signer => {
       const signature = nacl.sign.detached(signData, signer.secretKey);
       this.addSignature(signer.publicKey, signature);
@@ -422,7 +452,7 @@ export class Transaction {
       pubkey.equals(sigpair.publicKey),
     );
     if (index < 0) {
-      throw new Error(`Unknown signer: ${pubkey.toString()}`);
+      throw new Error(`unknown signer: ${pubkey.toString()}`);
     }
 
     this.signatures[index].signature = Buffer.from(signature);

web3.js/test/bpf-loader.test.js

@@ -196,25 +196,4 @@ describe('load BPF Rust program', () => {
 
     expect(logs.length).toEqual(0);
   });
-
-  test('simulate transaction with bad signer', async () => {
-    const simulatedTransaction = new Transaction().add({
-      keys: [
-        {pubkey: payerAccount.publicKey, isSigner: true, isWritable: true},
-      ],
-      programId: program.publicKey,
-    });
-
-    const {err, logs} = (
-      await connection.simulateTransaction(simulatedTransaction, [program])
-    ).value;
-    expect(err).toEqual('SanitizeFailure');
-
-    if (logs === null) {
-      expect(logs).not.toBeNull();
-      return;
-    }
-
-    expect(logs.length).toEqual(0);
-  });
 });

web3.js/test/transaction.test.js

@@ -10,6 +10,33 @@ import {SystemProgram} from '../src/system-program';
 import {Message} from '../src/message';
 
 describe('compileMessage', () => {
+  test('accountKeys are ordered', () => {
+    const payer = new Account();
+    const account2 = new Account();
+    const account3 = new Account();
+    const recentBlockhash = new Account().publicKey.toBase58();
+    const programId = new Account().publicKey;
+    const transaction = new Transaction({recentBlockhash}).add({
+      keys: [
+        {pubkey: account3.publicKey, isSigner: true, isWritable: false},
+        {pubkey: payer.publicKey, isSigner: true, isWritable: true},
+        {pubkey: account2.publicKey, isSigner: true, isWritable: true},
+      ],
+      programId,
+    });
+
+    transaction.setSigners(
+      payer.publicKey,
+      account2.publicKey,
+      account3.publicKey,
+    );
+
+    const message = transaction.compileMessage();
+    expect(message.accountKeys[0].equals(payer.publicKey)).toBe(true);
+    expect(message.accountKeys[1].equals(account2.publicKey)).toBe(true);
+    expect(message.accountKeys[2].equals(account3.publicKey)).toBe(true);
+  });
+
   test('payer is first account meta', () => {
     const payer = new Account();
     const other = new Account();
@@ -32,6 +59,48 @@ describe('compileMessage', () => {
     expect(message.header.numReadonlyUnsignedAccounts).toEqual(1);
   });
 
+  test('validation', () => {
+    const payer = new Account();
+    const other = new Account();
+    const recentBlockhash = new Account().publicKey.toBase58();
+    const programId = new Account().publicKey;
+
+    const transaction = new Transaction();
+    expect(() => {
+      transaction.compileMessage();
+    }).toThrow('Transaction recentBlockhash required');
+
+    transaction.recentBlockhash = recentBlockhash;
+
+    expect(() => {
+      transaction.compileMessage();
+    }).toThrow('No instructions provided');
+
+    transaction.add({
+      keys: [
+        {pubkey: other.publicKey, isSigner: true, isWritable: true},
+        {pubkey: payer.publicKey, isSigner: true, isWritable: true},
+      ],
+      programId,
+    });
+
+    expect(() => {
+      transaction.compileMessage();
+    }).toThrow('Transaction feePayer required');
+
+    transaction.setSigners(payer.publicKey);
+
+    expect(() => {
+      transaction.compileMessage();
+    }).toThrow('missing signer');
+
+    transaction.setSigners(payer.publicKey, new Account().publicKey);
+
+    expect(() => {
+      transaction.compileMessage();
+    }).toThrow('unknown signer');
+  });
+
   test('payer is writable', () => {
     const payer = new Account();
     const recentBlockhash = new Account().publicKey.toBase58();
@@ -48,31 +117,6 @@ describe('compileMessage', () => {
     expect(message.header.numReadonlySignedAccounts).toEqual(0);
     expect(message.header.numReadonlyUnsignedAccounts).toEqual(1);
   });
-
-  test('signers are ordered and only writable if necessary', () => {
-    const payer = new Account();
-    const account1 = new Account();
-    const account2 = new Account();
-    const recentBlockhash = new Account().publicKey.toBase58();
-    const programId = new Account().publicKey;
-    const transaction = new Transaction({recentBlockhash}).add({
-      keys: [
-        {pubkey: payer.publicKey, isSigner: true, isWritable: false},
-        {pubkey: account1.publicKey, isSigner: false, isWritable: true},
-      ],
-      programId,
-    });
-
-    // account2 is an extra signer, not involved in any instructions
-    transaction.sign(payer, account1, account2);
-    const message = transaction.compileMessage();
-    expect(message.accountKeys[0].equals(payer.publicKey)).toBe(true);
-    expect(message.accountKeys[1].equals(account1.publicKey)).toBe(true);
-    expect(message.accountKeys[2].equals(account2.publicKey)).toBe(true);
-    expect(message.header.numRequiredSignatures).toEqual(3);
-    expect(message.header.numReadonlySignedAccounts).toEqual(1);
-    expect(message.header.numReadonlyUnsignedAccounts).toEqual(1);
-  });
 });
 
 test('partialSign', () => {
@@ -97,6 +141,70 @@ test('partialSign', () => {
   expect(partialTransaction).toEqual(transaction);
 });
 
+describe('dedupe', () => {
+  const payer = new Account();
+  const duplicate1 = payer;
+  const duplicate2 = payer;
+  const recentBlockhash = new Account().publicKey.toBase58();
+  const programId = new Account().publicKey;
+
+  test('setSigners', () => {
+    const transaction = new Transaction({recentBlockhash}).add({
+      keys: [
+        {pubkey: duplicate1.publicKey, isSigner: true, isWritable: true},
+        {pubkey: payer.publicKey, isSigner: false, isWritable: true},
+        {pubkey: duplicate2.publicKey, isSigner: true, isWritable: false},
+      ],
+      programId,
+    });
+
+    transaction.setSigners(
+      payer.publicKey,
+      duplicate1.publicKey,
+      duplicate2.publicKey,
+    );
+
+    expect(transaction.signatures.length).toEqual(1);
+    expect(transaction.signatures[0].publicKey.equals(payer.publicKey)).toBe(
+      true,
+    );
+
+    const message = transaction.compileMessage();
+    expect(message.accountKeys[0].equals(payer.publicKey)).toBe(true);
+    expect(message.header.numRequiredSignatures).toEqual(1);
+    expect(message.header.numReadonlySignedAccounts).toEqual(0);
+    expect(message.header.numReadonlyUnsignedAccounts).toEqual(1);
+
+    transaction.signatures;
+  });
+
+  test('sign', () => {
+    const transaction = new Transaction({recentBlockhash}).add({
+      keys: [
+        {pubkey: duplicate1.publicKey, isSigner: true, isWritable: true},
+        {pubkey: payer.publicKey, isSigner: false, isWritable: true},
+        {pubkey: duplicate2.publicKey, isSigner: true, isWritable: false},
+      ],
+      programId,
+    });
+
+    transaction.sign(payer, duplicate1, duplicate2);
+
+    expect(transaction.signatures.length).toEqual(1);
+    expect(transaction.signatures[0].publicKey.equals(payer.publicKey)).toBe(
+      true,
+    );
+
+    const message = transaction.compileMessage();
+    expect(message.accountKeys[0].equals(payer.publicKey)).toBe(true);
+    expect(message.header.numRequiredSignatures).toEqual(1);
+    expect(message.header.numReadonlySignedAccounts).toEqual(0);
+    expect(message.header.numReadonlyUnsignedAccounts).toEqual(1);
+
+    transaction.signatures;
+  });
+});
+
 test('transfer signatures', () => {
   const account1 = new Account();
   const account2 = new Account();