Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix!: update commitment signature #4733

Closed
wants to merge 3 commits into from
Closed

fix!: update commitment signature #4733

wants to merge 3 commits into from

Conversation

AaronFeickert
Copy link
Collaborator

@AaronFeickert AaronFeickert commented Sep 23, 2022 •
edited by stringhandler
Loading

Switches from CommitmentSignature to CommitmentAndPublicKeySignature for transaction authorization. Work in progress.

BREAKING CHANGE: Commitment signature proof has changed

@CjS77 CjS77 added the CR-insufficient_context Your PRs commit messages don't provide enough context to justify accepting the change. label Sep 23, 2022
@AaronFeickert AaronFeickert changed the title WIP: fix: update commitment signature fix: update commitment signature Sep 23, 2022
@AaronFeickert AaronFeickert force-pushed the update-commitment-sig branch 2 times, most recently from bc8f81b to 6ef2407 Compare October 5, 2022 21:22
@AaronFeickert AaronFeickert force-pushed the update-commitment-sig branch 2 times, most recently from 5eff467 to 4a2f70b Compare October 12, 2022 16:32
stringhandler
stringhandler reviewed Oct 19, 2022
View reviewed changes
applications/tari_app_grpc/proto/transaction.proto
@@ -69,7 +69,7 @@ message TransactionInput {
// The script input data, if any
bytes input_data = 5;
// A signature with k_s, signing the script, input data, and mined height
ComSignature script_signature = 7;
ComAndPubSignature script_signature = 7;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should still be ComSig

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the description in RFC-0201 is still accurate, the intent is to assert knowledge of both the commitment opening and script private key, which necessitates the use of CommitmentAndPublicKeySignature even though the process isn't a multi-party operation. Otherwise you run into the same issue as with the transaction output use case, where you only assert the signer knows the sum of the commitment mask and script private key, and would need to ensure this doesn't result in any vulnerabilities elsewhere in the protocol.

Copy link
Collaborator

@SWvheerden SWvheerden Nov 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assumption with RFC-0201 with the input and outputs are that the because the signer knows the aggregate keys, they know the individual as well.

And we know even with multiparty signers that key cancelation is not a problem as they need to sign with the commitment blinding factor in the excess (so k needs to be known) and they need to calculate the correct script offset(again so k needs to be known)

I feel that a comsignature is 100% fine for both, but I am busy implementing this as a ComAndPubSignature for now.

@CjS77 CjS77 added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Oct 19, 2022
@stringhandler stringhandler changed the title fix: update commitment signature fix!: update commitment signature Oct 27, 2022
@stringhandler stringhandler added the W-consensus_breaking Warn - A change requiring a hard fork to be activated label Oct 27, 2022
@SWvheerden SWvheerden mentioned this pull request Nov 23, 2022
Merged
stringhandler pushed a commit that referenced this pull request Nov 23, 2022
@SWvheerden
feat!: update commitment signature (#4943)
00e98f9 
Description
---
Replaces PR #4733
This updates  CommitmentSignature to CommitmentAndPublicKeySignature for transaction authorization.
Includes a new Gen block

Motivation and Context
---
See issue: #4734

How Has This Been Tested?
---
Unit tests

BREAKING CHANGE: Commitment signature proof has changed
@stringhandler
Copy link
Collaborator

Closed in favour of #4943

@AaronFeickert AaronFeickert deleted the update-commitment-sig branch November 23, 2022 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stringhandler stringhandler stringhandler left review comments

@SWvheerden SWvheerden SWvheerden left review comments

Assignees
No one assigned
Labels
CR-insufficient_context Your PRs commit messages don't provide enough context to justify accepting the change. P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged W-consensus_breaking Warn - A change requiring a hard fork to be activated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AaronFeickert @stringhandler @SWvheerden @CjS77