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.

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

Add multisig bulletproof functions #3645

Closed
wants to merge 1 commit into from

Conversation

GeneFerneau
Copy link
Contributor

Add functions to create and verify a multisignature bulletproof. Callers must go
through multiple steps to create the full bulletproof

Each party must complete each step in the protocol, and the initiator
finalizes the multisignature bulletproof

@quentinlesceller
Copy link
Member

Looking good at first glance. Do you think adding tests for the ser/deser would be useful?

@GeneFerneau
Copy link
Contributor Author

Looking good at first glance. Do you think adding tests for the ser/deser would be useful?

I did run into issues with V5 slate changes in grin-wallet, but it was because of how serde interprets enums (took forever to figure that out).

Can take a look at what the ser/deser tests would look like for the node. Are you talking about the intermediate states? The final bulletproof looks the same AFAIU.

In the wallet, I think only the tau*, common_nonce, and partial_commit members need to be serialized. I've added those to the V5 slate format. Still working on multiparty transaction building, so maybe more is needed.

Serialization tests for multiparty bulletproofs are probably better suited in the wallet, IMHO.

@DavidBurkett
Copy link
Contributor

@GeneFerneau If I understand this correctly, the multi-party bulletproofs will be seen as invalid if we try to use the existing verify bulletproof APIs, which is why we're exposing verify_bullet_proof_multi. Is that correct? If so, then we're talking about needing a hardfork to support multiparty bulletproofs?

@GeneFerneau
Copy link
Contributor Author

@GeneFerneau If I understand this correctly, the multi-party bulletproofs will be seen as invalid if we try to use the existing verify bulletproof APIs, which is why we're exposing verify_bullet_proof_multi. Is that correct? If so, then we're talking about needing a hardfork to support multiparty bulletproofs?

As I understand it, the verify_bullet_proof_multi just allows to verify multiple bulletproofs in one call (instead of multiple calls to verify_bullet_proof). The multi-party bulletproofs are the same as single-party bulletproofs when finalized. The main difference is in how they are built.

No hardfork necessary, AFAIU

@DavidBurkett
Copy link
Contributor

Yep, you're correct. I don't think you even need to use the batch api here though. You're only verifying a single bulletproof by the looks of it.

@GeneFerneau
Copy link
Contributor Author

Definitely, I can remove it if you want. Just thought it might be useful for if/when the node wanted to do batch verify, maybe during IBD?

@DavidBurkett
Copy link
Contributor

We already use the batch api during IBD. It's called from TransactionBody::validate()

@GeneFerneau
Copy link
Contributor Author

We already use the batch api during IBD. It's called from TransactionBody::validate()

I'll remove verify_bullet_proof_multi from this PR, then. Seems easy enough to add back, if another use comes up.

Add function to create a multisignature bulletproof. Callers must go
through multiple steps to create the full bulletproof

Each party must complete each step in the protocol, and the initiator
finalizes the multisignature bulletproof
K: Keychain,
B: ProofBuild,
{
// TODO: proper support for different switch commitment schemes
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't the derive_key call on line 83 take care of the switch commitment? What more is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, and I copied this comment from the existing create function. I think it's referring to being able to specify the SwitchCommitmentType with a function argument.

I can add that if you want.

@GeneFerneau
Copy link
Contributor Author

Merged these changes into #3643

@GeneFerneau GeneFerneau closed this Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants