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.

Sign up for GitHub

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

Jump to bottom

consensus: more log grooming #6140

Merged
merged 10 commits into from
Feb 18, 2021
Merged

consensus: more log grooming #6140

merged 10 commits into from
Feb 18, 2021

Conversation

alexanderbez
Copy link
Contributor

ref: #5912

@alexanderbez alexanderbez self-assigned this Feb 18, 2021
@codecov
Copy link

codecov bot commented Feb 18, 2021 •
edited

Codecov Report

Merging #6140 (4e76041) into master (90c290a) will increase coverage by 0.06%.
The diff coverage is 52.34%.

@@            Coverage Diff             @@
##           master    #6140      +/-   ##
==========================================
+ Coverage   60.62%   60.69%   +0.06%     
==========================================
  Files         276      276              
  Lines       25689    25703      +14     
==========================================
+ Hits        15575    15601      +26     
+ Misses       8488     8485       -3     
+ Partials     1626     1617       -9
Impacted Files Coverage Δ
consensus/state.go 66.57% <52.34%> (-0.04%) ⬇️
privval/signer_server.go 94.73% <0.00%> (-5.27%) ⬇️
types/tx.go 82.97% <0.00%> (-4.26%) ⬇️
mempool/reactor.go 65.71% <0.00%> (-1.43%) ⬇️
statesync/syncer.go 79.60% <0.00%> (-0.81%) ⬇️
p2p/switch.go 59.90% <0.00%> (-0.47%) ⬇️
p2p/conn/connection.go 78.23% <0.00%> (-0.28%) ⬇️
blockchain/v0/pool.go 75.66% <0.00%> (ø)
p2p/transport_memory.go 86.30% <0.00%> (ø)
proxy/multi_app_conn.go 48.05% <0.00%> (ø)
... and 7 more

@alexanderbez alexanderbez marked this pull request as ready for review February 18, 2021 17:31
@alexanderbez alexanderbez added S:automerge Automatically merge PR when requirements pass T:code-hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. S:backport-to-v0.34.x Tell mergify to backport the PR to v0.34.x labels Feb 18, 2021
@alexanderbez alexanderbez changed the title more log grooming consensus: more log grooming Feb 18, 2021
cmwaters
cmwaters approved these changes Feb 18, 2021
View reviewed changes
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

🧹

consensus/state.go
@@ -715,28 +739,35 @@ func (cs *State) receiveRoutine(maxSteps int) {
for {
if maxSteps > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This max steps argument is an interesting artefact of the consensus reactor. Was it used for testing or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I'm not really sure but I'm too scared to make any legitimate changes to this code 😆

consensus/state.go
"proposer", cs.Validators.GetProposer().Address,
"privValidator", cs.privValidator,
"priv_validator", cs.privValidator,
Copy link
Contributor

Choose a reason for hiding this comment

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

This was the issue from yesterday right. Do you think it might pay to instead print cs.privValidatorPubKey.Address() or simply cs.privValidatorPubKey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Let me make a follow up PR :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#6144

consensus/state.go
@@ -1955,11 +2047,10 @@ func (cs *State) addVote(
switch vote.Type {
case tmproto.PrevoteType:
prevotes := cs.Votes.Prevotes(vote.Round)
cs.Logger.Info("Added to prevote", "vote", vote, "prevotes", prevotes.StringShort())
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want each vote to come in to be info? I guess with 150 validators that can be quite overbearing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's a lot. Keep in mind we can get multiple votes from multiple validators/peers multiple times per validator/peer. It can get pretty noisy!

@mergify mergify bot merged commit 8a3637a into master Feb 18, 2021
@mergify mergify bot deleted the bez/cons-groom-logs branch February 18, 2021 18:12
mergify bot pushed a commit that referenced this pull request Feb 18, 2021
@alexanderbez
consensus: more log grooming (#6140)
1033573 
ref: #5912
(cherry picked from commit 8a3637a)

# Conflicts:
#	consensus/state.go
@mergify mergify bot mentioned this pull request Feb 18, 2021
Merged
@alexanderbez alexanderbez mentioned this pull request Feb 18, 2021
Merged
mergify bot pushed a commit that referenced this pull request Feb 18, 2021
@alexanderbez
consensus: log private validator address and not struct (#6144)
27eb10a 
ref: #6140 (comment)
alexanderbez added a commit that referenced this pull request Feb 18, 2021
@alexanderbez
consensus: log private validator address and not struct (#6144)
a17b619 
ref: #6140 (comment)
alexanderbez pushed a commit that referenced this pull request Feb 18, 2021
@mergify
consensus: more log grooming (bp #6140) (#6143)
a0f3761
@cmwaters cmwaters mentioned this pull request Feb 22, 2022
Merged
This was referenced Apr 22, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmwaters cmwaters cmwaters approved these changes

@ebuchman ebuchman Awaiting requested review from ebuchman

@melekes melekes Awaiting requested review from melekes

@tessr tessr Awaiting requested review from tessr

@tac0turtle tac0turtle Awaiting requested review from tac0turtle

Assignees

@alexanderbez alexanderbez

Labels
S:automerge Automatically merge PR when requirements pass S:backport-to-v0.34.x Tell mergify to backport the PR to v0.34.x T:code-hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@alexanderbez @cmwaters