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 voter election process #543

Merged
merged 8 commits into from
Jan 22, 2023

Conversation

ulbqb
Copy link
Member

@ulbqb ulbqb commented Jan 17, 2023

Abstract

This PR remove the voter election process from Ostracon.

Background

Ostracon need some compatibility with Tendermint for IBC and cosmos-sdk. It it necessary to change possible parts for compatibility. Currently the voter election process is executed in Ostracon. Since voter selection can be moved to the lbm-sdk layer, it want to be moved to the lbm-sdk layer without putting it in Ostracon.

Goal

The goal is to fix to be compatible with Tendermint about voter election process. This means that the voter election process is removed from ostracon and it is done by only abci app. This goal need the following changes.

  • Remove the voter election process
  • Remove the proto field related to voter election
    • Removevoter_params, voter, voters, last_voters, voters_hash, voter_set, voting_weight that does not exist in Tendermint.
    • voter_params is the parameters used by voter election so this is not needed.
    • voters is the nodes elected from validators so this is not needed.
    • voting_weight is the value recalculated as voting power in voters so this is not needed.
  • Remove RPCs related to voter
    • Voters, ValidatorsWithVoters
  • FIx openapi.yaml
  • Fix documents

Policy

The changes is based on Tendermint v0.34.19.
This PR contains fixes other than documentation fixes. Fixing the documents is in #544.

Results

The voter election process was removed from ostracon. However, voter selection can still be implemented with the abci app. This PR makes it impossible to change the validator for each round. Updating validators is perform in EndBlock. See tendermint docs for details.

@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Merging #543 (6d004af) into main (6010b48) will decrease coverage by 0.08%.
The diff coverage is 74.80%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #543      +/-   ##
==========================================
- Coverage   66.15%   66.08%   -0.08%     
==========================================
  Files         279      278       -1     
  Lines       37593    36981     -612     
==========================================
- Hits        24871    24440     -431     
+ Misses      10947    10787     -160     
+ Partials     1775     1754      -21     
Impacted Files Coverage Δ
blockchain/v2/processor_context.go 67.44% <0.00%> (ø)
cmd/ostracon/commands/init.go 84.21% <ø> (-0.28%) ⬇️
config/toml.go 74.19% <ø> (ø)
consensus/replay_file.go 0.00% <0.00%> (ø)
consensus/types/round_state.go 0.00% <0.00%> (ø)
light/errors.go 75.00% <ø> (ø)
rpc/client/mock/client.go 19.29% <ø> (+0.65%) ⬆️
rpc/core/consensus.go 32.35% <ø> (-27.03%) ⬇️
rpc/core/routes.go 0.00% <ø> (ø)
rpc/core/types/responses.go 35.71% <ø> (ø)
... and 48 more

@ulbqb ulbqb force-pushed the fix/remove_voter_election branch 2 times, most recently from ed62360 to 2f3828a Compare January 17, 2023 05:51
@ulbqb ulbqb added the C: enhancement Classification: New feature or its request, or improvement in maintainability of code label Jan 17, 2023
@ulbqb ulbqb changed the title Remove voter election process Remove the voter election process Jan 17, 2023
@ulbqb ulbqb force-pushed the fix/remove_voter_election branch 2 times, most recently from 8eb65e3 to 66de98b Compare January 17, 2023 07:28
@ulbqb ulbqb force-pushed the fix/remove_voter_election branch from 66de98b to 1fce309 Compare January 17, 2023 08:01
@ulbqb ulbqb marked this pull request as ready for review January 17, 2023 08:29
@ulbqb ulbqb requested review from Kynea0b, torao and tnasu as code owners January 17, 2023 08:29
This reverts commit 3cab0a5.
consensus/reactor_test.go Show resolved Hide resolved
test/e2e/tests/validator_test.go Show resolved Hide resolved
@torao
Copy link
Contributor

torao commented Jan 18, 2023

I can then accept this PR if the details of the backgrounds and objectives of this modification are written.

statesync/stateprovider_test.go Outdated Show resolved Hide resolved
proto/ostracon/types/evidence.proto Outdated Show resolved Hide resolved
proto/ostracon/abci/types.proto Outdated Show resolved Hide resolved
@ulbqb ulbqb requested a review from tnasu January 19, 2023 02:45
@ulbqb
Copy link
Member Author

ulbqb commented Jan 19, 2023

@torao
I updated PR description. Please comment if anything is missing.

@ulbqb ulbqb requested a review from torao January 19, 2023 04:14
@torao
Copy link
Contributor

torao commented Jan 19, 2023

@ulbqb
Does LINE Blockchain no longer support Voter elections? If so, how about the reward payout would? Or if not, how to implement voter election? This should be important information for product design.

@zemyblue
Copy link
Member

@ulbqb
Does LINE Blockchain no longer support Voter elections? If so, how about the reward payout would? Or if not, how to implement voter election? This should be important information for product design.

@torao , we need to move this voter selection function to lbm-sdk layer later.

Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

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

Currently the voter election process is executed in Ostracon. However, if Ostracon's spec follows Tendermint's spec for this process, Ostracon should not have vote election process. Because Tendermint is not responsible for voter election, it is done by abci app.

@ulbqb , I think it is not a problem of description.
Ostracon need some compatibility with Tendermint for IBC and cosmos-sdk. So I think this PR is necessary because it is necessary to change possible parts for compatibility, and since voter selection can be moved to the lbm-sdk layer, it is moved to the lbm-sdk layer without putting it in Ostracon.

@ulbqb
Copy link
Member Author

ulbqb commented Jan 20, 2023

@ulbqb
Does LINE Blockchain no longer support Voter elections? If so, how about the reward payout would? Or if not, how to implement voter election? This should be important information for product design.

@torao Voter selection and reward logic can be freely implemented with abci app(lbm-sdk). Updating validators is perform in EndBlock. See tendermint docs for details.

Copy link
Contributor

@Kynea0b Kynea0b left a comment

Choose a reason for hiding this comment

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

LGTM (Just in case, this PR will be merged after someone else's review.)

@Kynea0b Kynea0b self-requested a review January 20, 2023 09:12
@torao
Copy link
Contributor

torao commented Jan 20, 2023

@zemyblue @ulbqb All right. We have an implicit agreement in our private discussion about how to conduct voter elections, but this PR only contains bits and pieces of information, which is why I asked the question.

To state our conclusion for those interested in LINE Blockchain, we plan to move the behaviour known as voter election from ostracon to the lbm-sdk. This is because in Tendermint, the selection of Validators is the responsibility of the Cosmos-SDK and we have chosen to follow that approach. In this PR, we'll remove the voter election from Ostracon and implement it in a future update to the lbm-sdk.

@egonspace
Copy link
Contributor

Hi guys~
Wow, this is amazing!
If so, do you give up functions such as selecting proposer according to probability? I don't think the sdk layer can be involved in selecting the proposer.

@ulbqb
Copy link
Member Author

ulbqb commented Feb 9, 2023

@egonspace
Hello.

If so, do you give up functions such as selecting proposer according to probability?

No. This PR removed only the voter election from ostracon. The proposer election is still run by ostracon.

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.

6 participants