feat: Add non-sequential multi-sig support

[janniks]

This PR was published to npm with the version 6.15.0
e.g. npm install @stacks/common@6.15.0 --save-exact

• Adds non-sequential multi-sig support ✨ closes Multisig transaction signing order #1487

• Fixes incorrect compression check in signing algorithm
• Fixes incorrect compression in deserialization
• Fixes incorrect field order for legacy (sequential) multisig
• Cleans up opts types/interfaces for less duplication

Related:

• 📓 SIP: non-sequential-multisig-transactions
• Core PR Feat: Signature order-independence fix for multisig tx stacks-network/stacks-core#3710

---

Adds a useNonSequentialMultiSig flag to multi-sig transaction creation methods. (Along with the internals to support it)

So far this opt is marked @experimental, since the interface of the default could change. (And we likely want to see how the authorization type plays out on mainnet, before switching fully)

Example

      const tx = await makeSTXTokenTransfer({
        recipient: 'STB44HYPYAT2BB2QE513NSP81HTMYWBJP02HPGK6',
        amount: 12_345n,
        fee: 1_000n,
        nonce: 2n,
        network: 'testnet',

        numSignatures: required,
        publicKeys: [k1, k2, k3],
        signerKeys: [k3, k1], // could sign in any order (even if serializing in between and not sharing state)

        useNonSequentialMultiSig: true, // <--- NEW OPTIONAL FLAG
      });

Comment

The separate signing test really shows there is more work needed to make this easy to use for folks building a "real" multi-sig experience -- with serializing the transaction in between signatures. This could be added as a separate issue or addressed in a breaking change (there is an opportunity to clean up the TransactionSigner as well, which is intertwined with the StacksTransaction class).

Follow up thoughts: This can probably be addressed with one mutating public method on the StacksTransaction called something like .finalize() or .finalizeMultiSig(), which ensures the transaction-auth-fields are in the intended order. (Might need the list of public-keys again, or can do an opinionated .sort first, but technically should work for legacy multi-sig as well).

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
stacksjs-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 28, 2024 6:50pm

[codecov]

Codecov Report

Attention: Patch coverage is 95.09804% with 5 lines in your changes missing coverage. Please review.

Files	Patch %	Lines
packages/transactions/src/builders.ts	88.09%	5 Missing ⚠️

📢 Thoughts on this report? Let us know!

[janniks]

Refactored these types to be more reusable. Does anybody think the change from interface to type is a problem?

[janniks]

@jbencin you'll notice this now differs from the blockchain make methods, but I think is more correct. We try to sign/append in order of public-keys, rather than first all signs and then all appends (not exactly sure why that was in the code previously, but seems to be inspired by stacks-core methods)

[jbencin]

This should be handled the same way in all multisig transaction types: We recommend that the user sort their pubkeys when creating, but we can't force it because users may have funds in accounts that don't use the recommended ordering

You could add a boolean argument, or create another version of this function, createMultiSigSpendingConditionSorted, if you want to provide sorting functionality to the user. But you can't just assume the user wants it

[janniks]

Got it. I would like to be more opinionated about it, so maybe sorting can be the default and we add a disablePublicKeySorting flag. But yeah, this should be a part of all the helpers then as well.. Will change 👍

[janniks]

Or maybe not, 🤔 technically the developer can still update and replace conditions/modes/etc. on any transaction fairly easily in Stacks.js (assuming they know what they're doing). Similar to how we don't really expose p2wsh multi-sig, we can "hide" the unsorted variant a bit more as well 🤷🏻‍♀️ trying to find a balance for users to be able to get less wrong by accident.

[jbencin]

Even with sequential multisig transactions, it was customary to sort the pubkeys before creating a transaction, but it was never enforced. So new multisig types should not handle this differently

One thing you could do, which would work for both types and would be backwards compatible, is to add an optional address parameter. Users would be encouraged to supply the address of the multisig account, and you could check if the pubkeys generate that address, first in the order given, then sort them and try in that order. If it doesn't match in either order, return an error

[janniks]

Lovely approach!!! I really like this more than another config flag!! 👏

[janniks]

Just thinking about next breaking release, I would like to change the default behavior to "sorted" to guide the ecosystem to the preferred way
.

[janniks]

Ready for re-review @jbencin 🙏

[jbencin]

Would be safer to have a switch here, and fail if privateKey.data.byteLength isn't PRIVATE_KEY_COMPRESSED_LENGTH or PRIVATE_KEY_UNCOMPRESSED_LENGTH

[jbencin]

Good work here! Any of the discussions that are still open I consider very minor and won't hold up approval for them