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

[61] Implement Proposer selector #66

Open
wants to merge 1 commit into
base: gauravahuja/60-qbft-block-validator
Choose a base branch
from

Conversation

gauravahuja
Copy link
Contributor

#61

@gauravahuja gauravahuja requested review from jframe and Filter94 March 25, 2025 09:10
val genesisBlockNumber: ULong,
)

override fun getProposerForBlock(header: BeaconBlockHeader): SafeFuture<Validator> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't use this function from Qbft as we don't have a header where this is called. We need a function that takes a ConsensusRoundIdentifier. This interface will need to be implemented for Qbft https://github.com/hyperledger/besu/blob/main/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/blockcreation/ProposerSelector.java. I've extracted the interface from the helper class in Besu as we need that when we create the MessageValidatorFactory in qbft. It will be in the next release of Besu likely next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just updated it

@gauravahuja gauravahuja force-pushed the gauravahuja/61-proposer-selector branch from 1cb4921 to b26f62d Compare March 28, 2025 07:17
@gauravahuja gauravahuja changed the base branch from main to gauravahuja/60-qbft-block-validator March 28, 2025 07:17
prevProposer = result
returnedValidators.add(result)
}
assertThat(returnedValidators).isEqualTo(validators)
Copy link
Collaborator

@Filter94 Filter94 Mar 28, 2025

Choose a reason for hiding this comment

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

My understanding is that thanks to changeEachBlock out of totalValidators calls with consecutive block number we're getting 5 different validators. But I'm not sure if we're just lucky that the order of returnedValidators matches validators or it's the required property of ProposerSelectorImpl.

I think that order enforcement makes this test more brittle than necessary.

fun `select proposer for next block`() {
val config =
ProposerSelectorImpl.Config(
changeEachBlock = true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a test for changeEachBlock = false?

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