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

BFT-463: Add LeaderSelectionMode::Rota #115

Merged
merged 2 commits into from
May 21, 2024
Merged

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented May 21, 2024

What ❔

Adds a LeaderSelectionMode::Rota to allow specifying leaders in subsequent views using a list of public keys. The 1st validator on the list is going to be the leader in view 0, the 2nd in view 1, the 3rd in view 2, wrapping around to start from the beginning as the views increase. There can be repeated keys on the list, and they all have to be part of the committee.

Why ❔

This is part of BFT-439 to modify the BFT tests to use the approach outlined in the Twins paper. Part of that is the ability to control who is the leader in each of the views.

With this change we should be able to generate a fixed leader schedule for the first few rounds of a protocol. The implementation wraps around so it's a total function, but in practice the tests will provide leaders for all the rounds executed in the test.

Why not modify Sticky?

I thought about whether I should add a new variant to LeaderSelectionMode or modify LeaderSelectionMode::Sticky to allow multiple values. On the Protobuf bytecode level they should be compatible, and I thought Sticky is probably used mostly in tests.

Based on the answers in the comments below, this is not the case, and it is in fact used in production to represent the centralised sequencer. The genesis it is part of is stored in Postgres as JSONB, so changes in the key name would not be backwards compatible. The protobuf_conformance workflow correctly highlighted this regression 🎉

@pompon0
Copy link
Contributor

pompon0 commented May 21, 2024

please remember to make the pr backward compatible

@aakoshh aakoshh force-pushed the bft-463-leader-schedule branch from 0e5f413 to 15621e5 Compare May 21, 2024 15:08
@aakoshh
Copy link
Contributor Author

aakoshh commented May 21, 2024

please remember to make the pr backward compatible

I tried (for binary, mentioned the potential JSON issue). Can you highlight what you consider a breaking change?

@aakoshh
Copy link
Contributor Author

aakoshh commented May 21, 2024

Are there some JSON based parsers, for config or genesis?

@aakoshh
Copy link
Contributor Author

aakoshh commented May 21, 2024

I can see the config being read from JSON in node/tools/main. All other JSON-Protobuf conversions seem to be in various tests.

@pompon0 so my question is: do you prefer to have a new variant next to Sticky because it's used in production configs, or is it a breaking change in JSON format that doesn't actually affect anything other than in-memory tests?

Happy to add a new variant, just thought I'd open the PR and ask here.

@aakoshh aakoshh force-pushed the bft-463-leader-schedule branch from 15621e5 to dd086b7 Compare May 21, 2024 16:02
@pompon0
Copy link
Contributor

pompon0 commented May 21, 2024

genesis is stored in postgresdb in jsonb format

@pompon0
Copy link
Contributor

pompon0 commented May 21, 2024

also, Sticky is currently the only variant used in zksync-era and it will be in use for foreseeable future (main node being the only proposer).

@aakoshh
Copy link
Contributor Author

aakoshh commented May 21, 2024

also, Sticky is currently the only variant used in zksync-era and it will be in use for foreseeable future (main node being the only proposer)

Thanks. I thought you would do that with a single sized committee and round-robin. nvm, that's different - confused it with the sequencer

Okay, now, what should I call this thing with multiple keys... 🤔
... got it: a rota 💡

@aakoshh aakoshh force-pushed the bft-463-leader-schedule branch 3 times, most recently from 621c78c to d9db46f Compare May 21, 2024 17:33
@aakoshh aakoshh changed the title BFT-463: Change LeaderSelectionMode::Sticky to allow multiple keys BFT-463: Add LeaderSelectionMode::Rota May 21, 2024
@aakoshh aakoshh requested review from pompon0 and brunoffranca May 21, 2024 17:41
@pompon0
Copy link
Contributor

pompon0 commented May 21, 2024

plan is to make ENs non-proposer validators

@aakoshh
Copy link
Contributor Author

aakoshh commented May 21, 2024

Hate to ask, but what's an "EN"?

@brunoffranca
Copy link
Member

External node. That's old terminology though, we can start calling them full nodes.

@aakoshh
Copy link
Contributor Author

aakoshh commented May 21, 2024

Thanks @brunoffranca
That's what I thought, except in your presentation the full nodes were connected through the gossip-network, not the consensus-network, so "non-proposing validator" was slightly confusing into which network they would belong to, and if the consensus (as validators), then why external (or full) at the same time?

Copy link
Member

@brunoffranca brunoffranca left a comment

Choose a reason for hiding this comment

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

Looks good. I would maybe rename the mode though. Round robin and Rota are synonyms. Since we are rotating on a subset of validators, maybe call it Subset?

node/libs/roles/src/validator/messages/consensus.rs Outdated Show resolved Hide resolved
@aakoshh aakoshh force-pushed the bft-463-leader-schedule branch from d9db46f to 7f0d40d Compare May 21, 2024 22:00
@aakoshh
Copy link
Contributor Author

aakoshh commented May 21, 2024

Round robin and Rota are synonyms.

I don't think they are synonyms... supervisors create the rota for workers to do, they aren't necessarily doing any round robin 🤔

From Google:
noun
1.
BRITISH
a list showing when each of a number of people has to do a particular job.

Since we are rotating on a subset of validators, maybe call it Subset?

The problem is it's not a subset, it's a list that can be arbitrarily long and contain arbitrarily repeating elements, e.g. [Alice, Bob, Alice, Alice, Alice, Charlie].

@brunoffranca
Copy link
Member

So, I think Grzegorz comment was a bit confusing. Let me clear this up.
We have 2 types of nodes: full nodes and validators. Full nodes (previously called external nodes) only participate in the gossip network. Validators participate in the gossip and validator networks.
The plan now is for us to have several validators but only one will be the leader (using the Sticky selection mode).

@aakoshh
Copy link
Contributor Author

aakoshh commented May 21, 2024

From vocabulary.com:

You can also call a rota a roster.

@brunoffranca would you prefer Roster?

@brunoffranca
Copy link
Member

brunoffranca commented May 21, 2024

Ok, fair enough. I thought it only meant "round or rotation of duties". Let's keep it then.

@aakoshh aakoshh merged commit c369606 into main May 21, 2024
5 checks passed
@aakoshh aakoshh deleted the bft-463-leader-schedule branch May 21, 2024 23:26
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