Feat: Signature order-independence fix for multisig tx

[fess-v]

Description

UPD v2 - removed rolling hash, gating logic implemented for being ready with the Nakamoto release (3.0 epoch)

UPD - check discussion below, decided to leave the rolling hash functionality and remove signatures & public keys from hash creation, add new multisig transaction hash modes following this SIP.
This comment explains this pull request.

OLD:
After the discussion on the SIP call from May 5th, we decided to make a PR with our ideas for fixing the signing ordering issue for multisig transactions.
Forum post with the initial discussion - https://forum.stacks.org/t/implementing-a-fully-functioning-multisig-wallet-on-stacks/14889
SIP PR - stacksgov/sips#139

This PR implements one possible solution for this problem - removing the rolling hash functionality from signing and verification procedures. This seems to be a redundant security measure that adds a lot of restrictions on the signing flow.
It is still a draft though so return types for verification and signing methods as well as test cases should be revised.

If it's required anyway, there's also another solution that we can think of - to unbind the relation of transaction fields to StacksAddress::from_public_keys. It can be done for instance via an additional public keys input array (in the multisig address creation order) or a separate index variable in each signing field.

An example, where the last parameter in the Signature option is the index in the initial address creation public key array:

pub enum TransactionAuthField {
    PublicKey(StacksPublicKey),
    Signature(TransactionPublicKeyEncoding, MessageSignature, u8),
}

In the second scenario, there will still be one restriction on the signing order - other users will have to get the previous signer's signature to continue the signing process. But it won't be critical anymore and will allow to create multisig wallets on Stacks.

Applicable issues

• Reliability Improvements: order-independent tx signing and verifying #2622
• Multisig transaction signing order hirosystems/stacks.js#1487

Additional info (benefits, drawbacks, caveats)

It is a consensus-critical change, therefore should be probably included in the next big release/fork of the network.
Solving this issue will open the multisig functionality to a wide range of use cases.

Checklist - TODO

• Test coverage for new or modified code paths
• Changelog is updated
• Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
• New clarity functions have corresponding PR in clarity-benchmarking repo
• New integration test(s) added to bitcoin-tests.yml

[CLAassistant]

All committers have signed the CLA.

[fess-v]

I just added a second draft to the PR using an additional field in the MultisigSpendingCondition struct, it doesn't remove the rolling hash but adds a correct order vector which helps to verify the multisig tx
I will add more autotests + fix naming and other review comments if this approach makes sense.

[jcnelson]

This is nowhere close to being ready.

First, adding a new mode of transaction authorization requires a hard fork, with gating logic to ensure that any transaction with this new struct will be unparseable until after the hard fork takes place. It will also require rigorous tests of this gating. Neither is present in this PR.

Second, the issues linked calls for order-independent transaction signing and verifying. As in, if Alice, Bob, and Charlie participate in a 2-of-3 multisig, then the order in which any two or more of them sign ought to result in not only the same multisig address, but also the same signatures (and same intermediate sighashes). This requirement is not satisfied with this code.

Third, there is zero test coverage for the feature. Even a basic test such as the one above would have demonstrated that this code does not address the problem at hand.

[fess-v]

Hey @jcnelson, thanks a lot for your feedback!
Your second point about signatures being equal from different owners is a bit hard to imagine now, could you elaborate on that? Do you mean when Alice signs the transaction as the first owner and when Bob signs as the first owner - they should get the same signature and intermediate sighash as the result? Or you're talking about when Bob signs as the first or the second owner - he should get in both cases the same signature as the outcome? Do you have any ideas on potential implementation logic in any case?

And you're right that it's far from being ready, since I have different ways to solve the problem I'm trying to propose potential solutions and start the discussion to select the way before moving forward with autotests with the gating logic which undoubtedly should be present, but before that, we need to figure out the approach to select.

[jcnelson]

Your second point about signatures being equal from different owners is a bit hard to imagine now, could you elaborate on that? Do you mean when Alice signs the transaction as the first owner and when Bob signs as the first owner - they should get the same signature and intermediate sighash as the result? Or you're talking about when Bob signs as the first or the second owner - he should get in both cases the same signature as the outcome? Do you have any ideas on potential implementation logic in any case?

Stacks multisig addresses are calculated the exact same way the are in Bitcoin (see StacksAddress::from_public_keys()). The order in which the public keys are given matters, just as it does for Bitcoin. However, unlike Bitcoin, the way signatures are calculated in MultisigSpendingCondition forces users to sign the transaction in the same order as their public keys were hashed to produce the multisig address.

Suppose Alice, Bob, and Charlie create a 2-of-3 multisig address, with their public keys given in that order. Suppose Alice and Charlie sign. When Alice signs, her signature (but not her public key) is appended to the MultisigSpendingCondition's state. Alice then adds Bob's public key to the MultisigSpendingCondition and gives the partially-signed transaction to Charlie. Then when Charlie signs, he signs not only the transaction, but also signs Alice's signature. This is because the sighash is calculated from the current state of the MultisigSpendingCondition structure -- see TransactionSpendingCondition::make_sighash_presign(), TransactionSpendingCondition::make_sighash_postsign(), TransactionSpendingCondition::next_signature(), and TransactionSpendingCondition::next_verification().

Now, if you look at MultisigSpendingCondition::verify(), you'll see that the auth structure would contain Alice's signature, Bob's public key, and Charlie's signature. Alice's public key is recovered from her signature, Bob's is popped out of the auth structure, and Charlie's public key is recovered from his signature (which means recovering it from the intermediate sighash which commits to Alice's signature). The three public keys are then hashed to recover a multisig address. If the multisig address matches the address in the transaction, then the signature is valid.

What needs to happen is that Charlie's signature must not commit to Alice's signature, but instead to just the initial transaction sighash. While Alice's and Charlie's signatures and Bob's public keys need to be inserted in to the auth structure in the same order in order for verify() to keep working, it would now be the case that Charlie no longer needs Alice to sign first.

[fess-v]

@jcnelson Oh thanks a lot for the explanation!
that's what I tried to do with my first approach, but I thought it was rejected, and then I created this one
If you can check the description of the PR and the first commit, I was trying to remove the rolling hash (the thing that you explained in the message) from TransactionSpendingCondition::next_signature(), TransactionSpendingCondition::next_verification()
Is it the way we need to follow then? Or did I approach it wrong?

[jcnelson]

Your code allows MultisigSpendingCondition::verify() to change the order in which public keys are hashed. But it does not ensure that the recovered public keys are actually the signers' public keys. For example, there's no code for generating fields_public_keys_creation_order from the signature order.

Also, I disagree with the approach. There's no need for fields_public_keys_creation_order in the first place if signers simply don't commit to each other's signatures.

[fess-v]

@jcnelson Yeah I understood that, I don't like this approach either that's why it's the second draft
Could you check the first commit? Is it the way to go? I just deleted the whole logic flow because I thought that this way was declined, but from your explanation, it seems that it's a better way to do that
I just thought that the rolling hash thing was required and deleted the first draft

[jcnelson]

Hey, I'm sorry for being snippy earlier. I'm glad you have taken the initiative to implement this feature. It's not the most straightforward thing to do, either, so kudos to you for taking this on :)

What made me grumpy earlier was all the emails you sent to me and my colleagues to review the PR, despite it being in draft status (i.e. not ready for review). Please don't do that. We're all subscribed to the repository already; we already get emails when PRs come in and get commented on. It sounded like you really, really wanted an assessment despite it not being ready for review.

---

Now to get to your questions:

Could you check the first commit? Is it the way to go? I just deleted the whole logic flow because I thought that this way was declined, but from your explanation, it seems that it's a better way to do that
I just thought that the rolling hash thing was required and deleted the first draft

The rolling hash approach is still required, because it provides linear-time signing and verification. For example, contrast this to Bitcoin's signing and verifying algorithm, which before segwit was quadratic-time.

However, the contents of the rolling hash can be different in order to accommodate new signing protocols. In particular, you'd make it so that sighash does not contain the previous signers' signatures and public keys.

Insofar as implementation, you'll notice that we have a few different enum variants for spending conditions. An order-independent multisig spending condition would be its own enum variant, with its own ID byte. This way, the block validation logic (which is epoch-gated) can be updated to reject blocks with this new spending condition structure before the hard fork in which this feature will ship takes effect.

Within TransactionSpendingCondition, you'll need new function bodies for make_sighash_presign() and make_sighash_postsign() which permit order-independent signatures if the current enum variant calls for order-independent signatures. On top of this, the implementation will need to implement StacksMessageCodec for this variant, and will need encoding/decoding tests. In addition, you'll need to extend the tests for auth structs to cover this new variant -- both in the positive case (i.e. the signature can be produced and verified) and in the negative case (i.e. corruption to the unsigned data will lead to a signature validation error).

Once you're confident with the new TransactionSpendingCondition variant, you'll additionally need to update the block and microblock validation logic so that blocks with this new condition get rejected by the network if they appear prior to the start of the system epoch in which they're allowed. This will include patching the miner code as well, so the miner does not pick out these transactions from the mempool if they arrive before the hard fork activates.

---

Again, thank you for PR. If you have any questions or concerns in making progress on the above, please drop a comment here :)

[fess-v]

Hey @jcnelson! First of all, thanks so much for your comment, now it's very much clear where to proceed, that's exactly what I was looking for!
And sorry for bothering you with emails, just before that I haven't received any reply within a few months on this PR :(
I know everyone is busy with the Nakamoto upgrade, so I just wanted to get some implementation hints to be ready to proceed.

[fess-v]

Hey @jcnelson, @jbencin!
I had to jump on our Stacks multisig release preparations and PRs to Hiro and Stacks.js, and finally back now

Implemented the new logic based on your comments above, supported the recent SIP, added OrderIndependentMultisigHashMode based on that, followed the previous repo structure for autotests, gating logic, hash modes, etc.

Now make_sighash_presign has an iteration also for public keys, not only for signatures, so now the hash for signatures is defined by index in the initial creation array

For that, I added the order_independent_sighash_by_index function to generate hash for signing for any owner by index (still rolling hash but without owner's signature or public key)

sign_no_append and append signature methods added for both sponsored and standard transactions - because now it is possible to sign a multisig transaction and then add signatures in the correct order

Gating logic added and tested (both sponsored and standard multisig transactions), used Epoch21 for now as a stub, will change it later to the target activation epoch

I'm planning to add:

1. More tests for wrong order signatures for both new hash modes
2. 3/5 and 3/3 multisig tests (now there's only 2/3 as it was before)
3. More gating logic autotests (not only p2sh, at least 2 more autotests for p2wsh - standard and sponsored)

If you have any questions let me know, looking forward to your feedback!
Thanks

[jbencin]

Added a couple comments, more to come...

[jbencin]

The rolling hash approach is still required, because it provides linear-time signing and verification. For example, contrast this to Bitcoin's signing and verifying algorithm, which before segwit was quadratic-time

@jcnelson can you explain how a rolling hash is required for linear-time signing and verification? I would think whether you use a rolling hash or not, if you have n participants, you will have to produce and validate n signatures, making it an O(n) (linear time) process either way

[jbencin]

This looks pretty good to me. Definitely make those epoch changes Jude suggested. Also maybe add tests for some different thresholds, like 2-of-9, 5-of-5, or something

[fess-v]

@jbencin @jcnelson fixed all comments except one, commented on that one too, and added more autotests for different multisig thresholds

[codecov]

Codecov Report

Attention: Patch coverage is 86.51893% with 146 lines in your changes are missing coverage. Please review.

Project coverage is 77.31%. Comparing base (c383bdd) to head (a995f21).
Report is 31 commits behind head on next.

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #3710      +/-   ##
==========================================
- Coverage   83.15%   77.31%   -5.85%     
==========================================
  Files         470      470              
  Lines      332698   335231    +2533     
  Branches      317      317              
==========================================
- Hits       276663   259171   -17492     
- Misses      56027    76052   +20025     
  Partials        8        8              

Files	Coverage Δ
stacks-common/src/codec/mod.rs	83.92% <ø> (ø)
stackslib/src/blockstack_cli.rs	77.63% <ø> (-2.44%)	⬇️
...tackslib/src/chainstate/nakamoto/staging_blocks.rs	92.83% <ø> (ø)
stackslib/src/chainstate/stacks/auth.rs	93.50% <ø> (-4.56%)	⬇️
stackslib/src/chainstate/stacks/db/blocks.rs	85.75% <100.00%> (-4.10%)	⬇️
.../src/chainstate/stacks/tests/block_construction.rs	87.32% <100.00%> (-10.95%)	⬇️
...lib/src/chainstate/stacks/tests/chain_histories.rs	94.55% <100.00%> (-1.17%)	⬇️
stackslib/src/chainstate/stacks/transaction.rs	85.32% <ø> (-11.00%)	⬇️
stackslib/src/core/mempool.rs	85.95% <100.00%> (ø)
stackslib/src/net/api/getblock.rs	83.88% <ø> (-1.67%)	⬇️
... and 29 more

... and 167 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c383bdd...a995f21. Read the comment docs.

[jcnelson]

Well done seeing this through to the end @fess-v!

Let's get that rider SIP added to SIP-021 so this activates in the next hard fork :)

[jcnelson]

Before merging, I would like @kantai or @obycode to take a look as well.

[obycode]

This LGTM. Just two minor comments.

[obycode]

Thanks!

[kantai]

LGTM!

[will-corcoran]

Amazing work. To echo @jcnelson - kudos on your perseverance @fess-v !