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

Committee-based consensus #42

Merged
merged 83 commits into from
Jul 14, 2024
Merged

Committee-based consensus #42

merged 83 commits into from
Jul 14, 2024

Conversation

MatheusFranco99
Copy link
Contributor

This SIP proposes aggregating Attestation and Sync Committee duties on a single duty. This will reduce the number of messages exchanged and processing costs.

sips/cluster_consensus.md Outdated Show resolved Hide resolved
sips/cluster_consensus.md Outdated Show resolved Hide resolved
sips/cluster_consensus.md Outdated Show resolved Hide resolved
sips/cluster_consensus.md Outdated Show resolved Hide resolved
sips/cluster_consensus.md Outdated Show resolved Hide resolved
sips/cluster_consensus.md Outdated Show resolved Hide resolved
@GalRogozinski
Copy link
Contributor

I think this requires reduce signatures with a fork to totally take out BLS out of consensus?

@MatheusFranco99
Copy link
Contributor Author

@GalRogozinski For this change, we can still use BLS in consensus.

@GalRogozinski
Copy link
Contributor

@MatheusFranco99 when you are doing operator based consensus, which validator do you choose?

What happens when validators are added or removed? You didn't spec this in the SIP...

You can find logic to solve this, but I think it makes more sense to simply switch to operator's keys... Because we should do this anyhow

@MatheusFranco99
Copy link
Contributor Author

@GalRogozinski
I think you don't need to choose any validator for that. As I see it, if you are operator A and validator X with cluster (A, B, C, D) has a duty, then you would start a consensus instance for the cluster (A, B, C, D) for the duty's slot. The consensus data can either be an AttestationData with any CommitteeIndex

type AttestationData struct {
	Slot             Slot
	Index           CommitteeIndex // Default value
	BeaconBlockRoot Root 
	Source         *Checkpoint
	Target          *Checkpoint
}

or a custom structure like:

struct {
	BeaconBlockRoot Root 
	Source         *Checkpoint
	Target          *Checkpoint
}

Another validator Y with the same cluster would use the same consensus instance and get the useful decided information about the Root and Checkpoints to build the object to be signed.

@MatheusFranco99
Copy link
Contributor Author

@GalRogozinski
Do you mean aggregating the validator registration and exit duties? Or how added/removed validators change this aggregation?
For the second question, removed validators shouldn't have the duty event and won't be using the decided data by its cluster's consensus, while added validators would use this data.

@GalRogozinski
Copy link
Contributor

GalRogozinski commented Mar 11, 2024 via email

@MatheusFranco99
Copy link
Contributor Author

@GalRogozinski What do you mean by choosing a validator?

@MatheusFranco99
Copy link
Contributor Author

If operators used BLS for consensus, they would need to choose a validator to know with which BLS key they should sign messages. On discussion, we concluded that this approach would require some agreed-upon deterministic selection. This would not be easy with the current design (due to the decoupling of consensus and duty execution logic).

However, there's a cryptography change going on by which operators should use their network keys to validate consensus messages. Relying on that would make things much simpler. Therefore, we prefer to make this change a pre-requisite for this SIP.

@moshe-blox
Copy link
Contributor

moshe-blox commented Mar 26, 2024

Stuff to think about:

  • Subnet calculation by cluster identity? And if so — we must refactor subnet scoring to tune expected message rate

@GalRogozinski @MatheusFranco99

@moshe-blox
Copy link
Contributor

Just noting that this SIP should have the representation of the BeaconState (or however you decide to call it) and a specification of how to construct it.

@moshe-blox
Copy link
Contributor

moshe-blox commented Mar 27, 2024

@GalRogozinski & me considering a different design approach that more naturally represents the hierarchy of cluster -> instance -> runners:

Untitled-2024-03-27-1654

Please let us know what you think @MatheusFranco99

If this would be too big of a change, then we might consider skipping it

@MatheusFranco99
Copy link
Contributor Author

@moshe-blox Indeed. Will add it later on. Thanks!

@MatheusFranco99
Copy link
Contributor Author

I like the design @moshe-blox .

This cluster-based consensus PR will already be huge anyway. I think this design produces an even bigger change in terms of code and tests, but I think it may be worth the cost.

@GalRogozinski
Copy link
Contributor

GalRogozinski commented Mar 30, 2024

New containers and interfaces to be added:

// Cluster is a cluster of a unique set of operators that run the same validator set
type Cluster struct {
	ConsensusRunner ConsensusRunner
	SSVRunners      []SSVRunner
	Network         Network
	Beacon          BeaconNode
	Shares           *[]types.Share
}

type ConsensusRunner interface {
	Getters

	GetSSVSigner() types.SSVSigner
	StartNewInstance(role types.BeaconRole, slot spec.Slot) error
	ProcessConsensus(msg *qbft.SignedMessage) error
	HasRunningInstance() bool
}

type SSVRunner interface {
	Getters

	GetBeaconSigner() types.BeaconSigner
	StartNewDuty(duty *types.Duty) error
	ProcessPreConsensus(signedMsg *types.SignedPartialSignatureMessage) error
	ProcessPostConsensus(signedMsg *types.SignedPartialSignatureMessage) error
}

@GalRogozinski
Copy link
Contributor

For topics the first validator can be used or we organize our topics via clusters

Copy link
Contributor

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

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

just clarify to me your calculation please...
maybe I am wrong, but I am not sure why

sips/committee_consensus.md Outdated Show resolved Hide resolved
sips/committee_consensus.md Show resolved Hide resolved
sips/committee_consensus.md Outdated Show resolved Hide resolved
sips/committee_consensus.md Outdated Show resolved Hide resolved
sips/committee_consensus.md Outdated Show resolved Hide resolved
@liorrutenberg liorrutenberg requested review from alonmuroch and removed request for alonmuroch July 14, 2024 12:27
@GalRogozinski GalRogozinski merged commit 718c52e into main Jul 14, 2024
@GalRogozinski GalRogozinski deleted the cluster-consensus branch July 14, 2024 18:02
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.

6 participants