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

fix: broadcast message with aggregated key #579

Closed
wants to merge 2 commits into from

Conversation

RiXelanya
Copy link
Contributor

Description

I tried to make a new broadcast which includes the aggregate signature and public keys

Related issue(s)

If this Pull Request is related to an issue, mention it here.

@amirvalhalla
Copy link
Member

@RiXelanya
please follow the PR title rules. read more:
Contributing guidelines

@RiXelanya RiXelanya changed the title Hello fix: broadcast message with aggregated key Jul 6, 2023
@b00f
Copy link
Collaborator

b00f commented Jul 6, 2023

@RiXelanya

From your PR, I can see that you got a little bit confused. I believe the main reason for this confusion is the need to maintain backward compatibility. It's okay, no problem. I'll try to explain it a little more here.

sayHelloLagacy method

I suggest renaming the sayHello function to sayHelloLegacy and keeping it as it is. Please make sure to apply this change to the tests as well, so that they remain functional.

The sayHelloLegacy method helps us maintain backward compatibility, but we don't need to keep it forever. In a future release, once we confirm that the majority of the nodes are supporting the new format, we can safely delete it.

Adding comments to the code would be a good idea to provide clarity and context.

sayHello method

Now we can implement sayHello, In this method first we can get the list of all public keys from the sync.signers and at the same time we calculate the aggregated signature.

Probably should be something like this:

pubs := make([]*bls.PublicKey, 0, len(sync.signers))
sigs := make([]*bls.Signature, 0, len(sync.signers))

for _, signer := range sync.signers {
	signer.SignMsg(msg)
	pubs = append(pubkeys, msg.PublicKey)
	sigs = append(signs, msg.Signature)
}
aggSig := bls.SignatureAggregate(signs)

Now, we have the aggregated signature (only one signature) and list of all public keys.
Let's create another hello message and broadcast it.

msg.Signature = aggSig
msg.PublicKeys = pubs
msg.PublicKey = nil

Signature Verification

Now, let's move to the verification part. It is done inside the SanityCheck function.
Here, we need to support both Legacy and new format.

How to figure out? It is easy. If PulicKey set, it is legacy version, it PubliKeys set it is new version.
So we will have something like:

func (m *HelloMessage) SanityCheck() error {
	if m.Signature == nil {
		return errors.Error(errors.ErrInvalidSignature)
	}
	if m.PublicKey == nil &&  m.PublicKeys == nil {
		return errors.Error(errors.ErrInvalidPublicKey)
	}
	if m.PublicKey != nil {
             /// Legacy version
             return m.PublicKey.Verify(m.SignBytes(), m.Signature)
	} else {
	/// 1- Calculate the Aggregated Public key
	/// 2- Verify the signature
        }
}

@RiXelanya
Copy link
Contributor Author

Understood. I will start fixing it now. apologies for the late response. I was busy yesterday

@themantre
Copy link
Contributor

Fixed at #640

@themantre themantre closed this Aug 20, 2023
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.

Hello message should contains all public keys
4 participants