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

Pass MessageID To Broadcast #381

Merged
merged 3 commits into from
Apr 16, 2024
Merged

Pass MessageID To Broadcast #381

merged 3 commits into from
Apr 16, 2024

Conversation

olegshmuelov
Copy link
Contributor

Overview

This PR is a follow up for #372.

Changes

  • Pass message ID to broadcast

@olegshmuelov olegshmuelov marked this pull request as ready for review April 11, 2024 13:56
@@ -20,7 +20,7 @@ func NewTestingNetwork(operatorID types.OperatorID, sk *rsa.PrivateKey) *Testing
}
}

func (net *TestingNetwork) Broadcast(message *types.SignedSSVMessage) error {
func (net *TestingNetwork) Broadcast(msgID types.MessageID, message *types.SignedSSVMessage) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am thinking about this further now...
you only need the ValidatorPublicKey?
So maybe it is best to simply pass recipientID []byte parameter for topic.

This is because in the very near future (cluster-based consensus), we will be using ClusterID exclusively for topic calculation (at least this is the current plan), while it may not necessarily be in the MessageID (duty dependent). So my offer will save a future change...

Your choice whether you want to change it now of course

@y0sher
Copy link
Contributor

y0sher commented Apr 11, 2024

I think Gal's point is valid though, if we can make it thinner and just pass validator pubkey then maybe it's better.

@moshe-blox
Copy link
Contributor

moshe-blox commented Apr 11, 2024

@GalRogozinski @y0sher
imo consensus should make the least amount of assumptions on how p2p works
passing only 'recipient' means consensus assumes subnets are by cluster id or validator pk
but what if p2p wants to determine subnet by role + recipient?

either way is fine for now, just noting the difference between the two

@GalRogozinski
Copy link
Contributor

@moshe-blox
recipient is a general term that can be passed with no assumption...
It could be that the messageID won't have the necessary information for the topic

@moshe-blox
Copy link
Contributor

moshe-blox commented Apr 14, 2024

@GalRogozinski how so? i thought that MessageID can contain in it either the cluster id or the validator pk (if its a block proposal for example)

sure, it makes sense if it's a []byte named recipient instead and as long as it's not spec-tested then the implementation is free to pass whatever it wants there

@GalRogozinski
Copy link
Contributor

@moshe-blox
Let say we decide to calculate all topics only with cluster_id%128, and for each validator we find the cluster it belongs to (current proposal)

then in this case if you pass only validator pk MessageID won't have enough information

@moshe-blox
Copy link
Contributor

moshe-blox commented Apr 16, 2024

@GalRogozinski it's an implementation problem no? we're going to maintain mappings between cluster ids and validators so it's shouldn't be an issue

@GalRogozinski GalRogozinski merged commit 5ad0f14 into main Apr 16, 2024
2 checks passed
@GalRogozinski GalRogozinski deleted the add-msgid-to-broadcast branch April 16, 2024 12:24
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.

5 participants