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

fix: Modify omission of change to change ValidatorSet to VoterSet for marverick #345

Merged
merged 2 commits into from
Nov 19, 2021
Merged

Conversation

Kynea0b
Copy link
Contributor

@Kynea0b Kynea0b commented Nov 17, 2021

Description

Corresponding to the omission of change from the validator set to the voter set during version upgrade work.
This commit change was not reflected.25fe25c

This PR is an additional modification of this PR #340

@codecov
Copy link

codecov bot commented Nov 17, 2021

Codecov Report

Merging #345 (badbaa6) into main (b628591) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head badbaa6 differs from pull request most recent head 2bac1b2. Consider uploading reports for the commit 2bac1b2 to get more accurate results

@@            Coverage Diff             @@
##             main     #345      +/-   ##
==========================================
+ Coverage   63.00%   63.01%   +0.01%     
==========================================
  Files         272      272              
  Lines       30286    30253      -33     
==========================================
- Hits        19081    19064      -17     
+ Misses       9475     9461      -14     
+ Partials     1730     1728       -2     
Impacted Files Coverage Δ
p2p/peer.go 62.80% <100.00%> (+1.62%) ⬆️
libs/events/events.go 93.20% <0.00%> (-4.86%) ⬇️
blockchain/v1/reactor.go 69.62% <0.00%> (-3.08%) ⬇️
crypto/bls/bls.go 48.41% <0.00%> (-2.39%) ⬇️
p2p/pex/pex_reactor.go 79.58% <0.00%> (-1.05%) ⬇️
abci/client/socket_client.go 75.85% <0.00%> (-1.03%) ⬇️
blockchain/v0/reactor.go 68.01% <0.00%> (-0.91%) ⬇️
statesync/syncer.go 78.62% <0.00%> (-0.77%) ⬇️
state/execution.go 68.09% <0.00%> (-0.66%) ⬇️
p2p/switch.go 67.09% <0.00%> (-0.52%) ⬇️
... and 13 more

@torao torao added the C: bug Classification: Something isn't working label Nov 17, 2021
@torao torao requested review from torao and tnasu November 17, 2021 08:28
torao
torao previously approved these changes Nov 17, 2021
Copy link
Contributor

@torao torao left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@tnasu tnasu left a comment

Choose a reason for hiding this comment

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

Could you also update CONTRIBUTING.md? Should it change in ./test/maverick/node?

https:/line/ostracon/blob/main/CONTRIBUTING.md#maverick

If you're changing the code in consensus package, please make sure to replicate all the changes in ./test/maverick/consensus. Maverick is a byzantine node used to assert that the validator gets punished for malicious behavior.

@Kynea0b
Copy link
Contributor Author

Kynea0b commented Nov 18, 2021

@tnasu
Thank you for your pointing out.

Could you also update CONTRIBUTING.md? Should it change in ./test/maverick/node?

CONTRIBUTING.md does not need to be changed.

If you're changing the code in consensus package, please make sure to replicate all the changes in ./test/maverick/consensus. Maverick is a byzantine node used to assert that the validator gets punished for malicious behavior.

It means that the code change in consensus package should be applied to this path ./test/maverick/consensus as well, so I think there is no problem with the previous #340 and current this PR.

@tnasu
Copy link
Member

tnasu commented Nov 19, 2021

Sorry for my misleading. I mean that I want to change the sentence to the below:

If you're changing the code in consensus or node package, please make sure to replicate all the changes in ./test/maverick/consensus and ./test/maverick/node. Maverick is a byzantine node used to assert that the validator gets punished for malicious behavior.

@Kynea0b
Copy link
Contributor Author

Kynea0b commented Nov 19, 2021

@tnasu
OK. I will fix it.

@Kynea0b Kynea0b merged commit 5f56d8c into Finschia:main Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: bug Classification: Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants