fix(tariscript): multisig ordered signatures and pubkeys #5961
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brianp
force-pushed
the
fix-tari-script-multisig
branch
from
bd50996
to
630b875
Compare
ghpbot-tari-project
added
P-acks_required
AaronFeickert
requested changes
Nov 16, 2023
AaronFeickert
requested changes
Nov 16, 2023
| } | ||
|
|
||
| for pk in pub_keys.by_ref() { | ||
| if !sig_set.contains(s) && s.verify_raw_canonical(pk, &message) { |
There was a problem hiding this comment.
The use of verify_raw_canonical means that message must safely bind the public key. In this case, it is identical across the full set of allowed public keys, which technically breaks Fiat-Shamir. In practice, it means that a forger would need to target any one of the public keys, the complexity of which depends on the number of public keys in the set.
There was a problem hiding this comment.
See #5817 for a related issue.
Require the ordering of the pubkeys and signatures to matter. If the signatures or pubkeys are out of order the likelyhood of passing decreases.
Remove the check that a key has been validated previously. With ordered sigs and keys a key will only ever be checked once so we don't need to confirm if it's already provided a valid sig. It's also confusing because Alice can provide two sigs against her pub key but she has to provide the pubkey twice. The language here made that a litle confusing.
brianp
force-pushed
the
fix-tari-script-multisig
branch
from
70e4b6d
to
8c36321
Compare
SWvheerden
approved these changes
Nov 17, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Update the multisig op code checks to care about the order of keys and signatures. This is to assist with reducing the amount of iterations and processing a node is required to perform to validate a collection.
Motivation and Context
Low/Minor audit issue to reduce processing for nodes.
Closes: #5806
How Has This Been Tested?
Test suite / CI
What process can a PR reviewer use to test or verify this change?
Look at the test additions. They make it more clear what the effects of the changes are.
Breaking Changes