Hidden assumptions within multisig transaction signing

[jecassis]

Describe the bug
When submitting a P2SH multisig transaction to the mainnet/testnet that was signed with compressed keys, the network responds with the error: Signer hash does not equal hash of public key(s). Close reading of SIP-005 as well as the stacks-blockchain source code, indicate that there are a lot of underlying assumptions abstracted by the library, further aggravated by incorrect generalizations in the example comments.

Code to Reproduce

import {
  TransactionSigner,
  getPublicKey,
  createStacksPrivateKey,
  publicKeyToString,
  standardPrincipalCV,
  broadcastTransaction,
  makeUnsignedSTXTokenTransfer,
} from '@stacks/transactions';
import { StacksTestnet } from '@stacks/network';
import BigNum from 'bn.js';

// Private key strings with compression byte appended
const privKeyStrings = [
  '6d430bb91222408e7706c9001cfaeb91b08c2be6d5ac95779ab52c6b431950e001',
  '2a584d899fed1d24e26b524f202763c8ab30260167429f157f1c119f550fa6af01',
  'd5200dee706ee53ae98a03fba6cf4fdcc5084c30cfa9e1b3462dcdeaa3e0f1d201',
];
const privKeys = privKeyStrings.map(createStacksPrivateKey);

// Compressed public keys
const pubKeys = privKeys.map(getPublicKey);
const pubKeyStrings = pubKeys.map(publicKeyToString);

const recipient = standardPrincipalCV('SN3C9PPBTKJNWJ5HZBBWERRAF4E3W6M39AN54S92');
const amount = new BigNum(74);
const fee = new BigNum(278);
const nonce = new BigNum(0);
const memo = 'test';
const network = new StacksTestnet();

(async () => {
  const transaction = await makeUnsignedSTXTokenTransfer({
    recipient,
    amount,
    network,
    fee,
    nonce,
    memo,
    numSignatures: 2,
    publicKeys: pubKeyStrings,
  });

  const signer = new TransactionSigner(transaction);
  signer.signOrigin(privKeys[0]);
  signer.signOrigin(privKeys[2]);
  signer.appendOrigin(pubKeys[1]);

  const txId = await broadcastTransaction(transaction, network);
  console.log('Transaction ID: ' + JSON.stringify(txId, null, 2).replace(/^"|"$/, ''));
})().catch((err) => {
  console.error('Error:' + err);
  process.exit(1);
});

Expected Behavior
The code as given above will fail with the aforementioned error. The reason for it is that the keys in the fields are in the wrong order and thus the hash160 of the redeem script mismatches.

Additional Context
The solution with the current state of the verify function to this must be the following:
Even after meeting the numSignatures requirement, the signatures as well as the public keys of the participants who did not sign must end up IN THE SAME ORDER in the spending condition fields relative to the public key order used to generate the redeem script.

Also unmentioned (at least in this library) and yet expected is that the private key compression byte is used to generate the "type" in the transaction that is read by the signature verification to regenerate the public key. Each of these must match the format (i.e., compressed or uncompressed) for the corresponding public key passed to makeUnsignedSTXTokenTransfer. See SIP-005.

Workarounds

1. Set/hack checkOversign to false which would remove the guardrails but allow the following:

  signer.signOrigin(privKeys[0]);
  signer.signOrigin(privKeys[2]);
  signer.appendOrigin(pubKeys[1]);

2. Splice the fields array after signing and appending as in the code example, e.g.:

  const field1 = deserializedTx.auth.spendingCondition.fields.splice(2, 1);
  deserializedTx.auth.spendingCondition.fields.splice(1, 0, field1[0]);

3. Initializing the fields array with the public keys and placing the signatures in the correct location during signOrigin. Great care must be taken to ensure the sighash generation is consistent.

Related Issues

• Multisig transaction signing doesn't work with uncompressed keys #961

[jcnelson]

Even after meeting the numSignatures requirement, the signatures as well as the public keys of the participants who did not sign must end up IN THE SAME ORDER in the spending condition fields relative to the public key order used to generate the redeem script.

Admittedly, this was a design oversight in SIP-005 (but one we're stuck with for the time being). We're considering relaxing this in the Stacks 2.1 upgrade to make it so the signatures can be generated in any order.

[reedrosenbluth]

Also unmentioned (at least in this library) and yet expected is that the private key compression byte is used to generate the "type" in the transaction that is read by the signature verification to regenerate the public key. Each of these must match the format (i.e., compressed or uncompressed) for the corresponding public key passed to makeUnsignedSTXTokenTransfer. See SIP-005.

Better support for uncompressed keys was just added to @stacks/transactions v1.3.0 via: #963

[agraebe]

can this be closed now?

[jecassis]

No objection to closing on my end.