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

Remove the ability to select more than one for random sampling #563

Merged
merged 1 commit into from
Feb 17, 2023

Conversation

torao
Copy link
Contributor

@torao torao commented Feb 9, 2023

Description

This PR relates to #543, which removes voter selection. It removes the currently unused processing within the random sampling function and integrates it with the proposer selection. Multiple sampling was originally intended to select more than one in voter selection, but in practice, voters were selected in other ways and only used to select one proposer.

  • The processing of the RandomSamplingWithPriority() function has been moved to SelectProposer(). This removes the check, as it's able to guarantee that the totalVotingPower is the sum of the VotingPower.
  • Candidate interface that is no longer required has been removed.

I originally intended to remove SplitMix64 because I thought we could just use LastProofHash as a random number, but in reality, we need to use random numbers based on height and round in addition to LastProofHash, so SplitMix64 is still used (and therefore remains compatible with the previous behavior).

Closes: #XXX

@torao torao added the C: enhancement Classification: New feature or its request, or improvement in maintainability of code label Feb 9, 2023
@torao torao self-assigned this Feb 9, 2023
@torao torao requested review from Kynea0b and tnasu as code owners February 9, 2023 09:14
@codecov
Copy link

codecov bot commented Feb 9, 2023

Codecov Report

Merging #563 (f17c312) into main (5aca894) will decrease coverage by 0.11%.
The diff coverage is 92.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #563      +/-   ##
==========================================
- Coverage   66.17%   66.07%   -0.11%     
==========================================
  Files         278      277       -1     
  Lines       37022    36969      -53     
==========================================
- Hits        24499    24426      -73     
- Misses      10763    10775      +12     
- Partials     1760     1768       +8     
Impacted Files Coverage Δ
types/validator_set.go 91.13% <92.00%> (+0.62%) ⬆️
privval/socket_listeners.go 79.72% <0.00%> (-4.06%) ⬇️
privval/signer_listener_endpoint.go 88.88% <0.00%> (-2.39%) ⬇️
consensus/state.go 71.40% <0.00%> (-1.36%) ⬇️
p2p/conn/connection.go 79.80% <0.00%> (-0.58%) ⬇️
consensus/reactor.go 74.44% <0.00%> (-0.18%) ⬇️
light/client.go 72.64% <0.00%> (-0.15%) ⬇️
p2p/pex/pex_reactor.go 79.67% <0.00%> (ø)
proxy/multi_app_conn.go 47.66% <0.00%> (ø)
... and 2 more

zemyblue

This comment was marked as resolved.

@torao
Copy link
Contributor Author

torao commented Feb 10, 2023

@zemyblue
I think it seems that the voteSet parameter cannot be removed from this MakeCommit (it's the same code for Tendermint). Is there any reason why it should?

@zemyblue zemyblue self-requested a review February 10, 2023 05:06
@zemyblue
Copy link
Member

@zemyblue
I think it seems that the voteSet parameter cannot be removed from this MakeCommit (it's the same code for Tendermint). Is there any reason why it should?

Yes, no problem. I misunderstood and wrote it. Thank you.

@torao torao merged commit 7fcbb0f into Finschia:main Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: enhancement Classification: New feature or its request, or improvement in maintainability of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants