Skip to content

consensus: only call privValidator.GetPubKey once per block#5143

Merged
mergify[bot] merged 7 commits intomasterfrom
anton/4865-getpubkey-excessive-calls
Jul 30, 2020
Merged

consensus: only call privValidator.GetPubKey once per block#5143
mergify[bot] merged 7 commits intomasterfrom
anton/4865-getpubkey-excessive-calls

Conversation

@melekes
Copy link
Contributor

@melekes melekes commented Jul 22, 2020

Closes #4865

@auto-comment
Copy link

auto-comment bot commented Jul 22, 2020

👋 Thanks for creating a PR!

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Wrote tests
  • Updated CHANGELOG_PENDING.md
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments
  • Re-reviewed Files changed in the Github PR explorer
  • Applied Appropriate Labels

Thank you for your contribution to Tendermint! 🚀

@melekes melekes self-assigned this Jul 22, 2020
@melekes melekes marked this pull request as ready for review July 22, 2020 08:18
@melekes melekes requested a review from tessr as a code owner July 22, 2020 08:18
@codecov
Copy link

codecov bot commented Jul 22, 2020 •
edited
Loading

Codecov Report

Merging #5143 into master will increase coverage by 1.73%.
The diff coverage is 40.54%.

@@            Coverage Diff             @@
##           master    #5143      +/-   ##
==========================================
+ Coverage   60.83%   62.57%   +1.73%     
==========================================
  Files         203      259      +56     
  Lines       20526    27272    +6746     
==========================================
+ Hits        12488    17066    +4578     
- Misses       6961     8716    +1755     
- Partials     1077     1490     +413
Impacted Files Coverage Δ
consensus/state.go 73.37% <40.54%> (+0.58%) ⬆️
blockchain/v0/reactor.go 63.25% <0.00%> (-0.94%) ⬇️
proxy/multi_app_conn.go 58.02% <0.00%> (ø)
privval/signer_listener_endpoint.go 78.26% <0.00%> (ø)
privval/retry_signer_client.go 0.00% <0.00%> (ø)
light/client.go 71.45% <0.00%> (ø)
p2p/pex/addrbook.go 72.48% <0.00%> (ø)
p2p/pex/pex_reactor.go 80.95% <0.00%> (ø)
node/node.go 58.66% <0.00%> (ø)
light/trust_options.go 100.00% <0.00%> (ø)
... and 55 more

erikgrinaker
erikgrinaker reviewed Jul 22, 2020
View reviewed changes
consensus/state.go
Comment on lines +279 to +281
if err := cs.updatePrivValidatorPubKey(); err != nil {
cs.Logger.Error("Can't get private validator pubkey", "err", err)
}
Copy link
Contributor

@erikgrinaker erikgrinaker Jul 22, 2020

Choose a reason for hiding this comment

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

Should this be a hard error condition? If we can't fetch the validator's public key on startup, is there even any point in continuing with consensus?

Copy link
Contributor Author

@melekes melekes Jul 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

the very first time we try to get pubkey is in node.go

tendermint/node/node.go

Line 664 in 48426c5

pubKey, err := privValidator.GetPubKey()

there we exit upon failure

it's fine to continue here => validator will just not sign / vote on this block (or more depending on downtime)

tac0turtle
tac0turtle reviewed Jul 22, 2020
View reviewed changes
Copy link
Contributor

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

There is another key cache in the metrics function could this be replaced with this change? Also is there any chance we could test this and see the improvement?

@melekes
Copy link
Contributor Author

melekes commented Jul 22, 2020

There is another key cache in the metrics function could this be replaced with this change?

I can't use privValidatorPubKey because privValidator might be nil, so we need this "cache" in any case.

Also is there any chance we could test this and see the improvement?

you mean write a test or test by hands?

@tac0turtle
Copy link
Contributor

tac0turtle commented Jul 22, 2020

you mean write a test or test by hands?

test by hand. I dont think we have e2e tests that could do this, do we?

@tac0turtle
Copy link
Contributor

tac0turtle commented Jul 30, 2020

is this PR good to go into master?

@erikgrinaker erikgrinaker added the S:automerge Automatically merge PR when requirements pass label Jul 30, 2020
@mergify mergify bot merged commit 0d8d721 into master Jul 30, 2020
@mergify mergify bot deleted the anton/4865-getpubkey-excessive-calls branch July 30, 2020 09:44
@melekes melekes mentioned this pull request Nov 10, 2020
Closed
melekes added a commit that referenced this pull request Nov 10, 2020
@melekes
consensus: only call privValidator.GetPubKey once per block (#5143)
f67a306 
Closes #4865
melekes added a commit that referenced this pull request Nov 10, 2020
@melekes
consensus: only call privValidator.GetPubKey once per block (#5143)
0cf98f3 
Closes #4865
melekes added a commit that referenced this pull request Nov 16, 2020
@melekes
consensus: only call privValidator.GetPubKey once per block (#5143)
b1de0c2 
Closes #4865
melekes added a commit that referenced this pull request Nov 16, 2020
@melekes
consensus: only call privValidator.GetPubKey once per block (#5143)
5ba30e6 
Closes #4865
@EnderCrypto EnderCrypto mentioned this pull request Dec 25, 2020
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tessr tessr Awaiting requested review from tessr

2 more reviewers

@tac0turtle tac0turtle tac0turtle left review comments

@erikgrinaker erikgrinaker erikgrinaker approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@melekes melekes

Labels

S:automerge Automatically merge PR when requirements pass

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Excessive calls to GetPubkey() on external signer

3 participants

@melekes @tac0turtle @erikgrinaker