consensus: only call privValidator.GetPubKey once per block

[melekes]

Closes #4865

[auto-comment]

👋 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! 🚀

[codecov]

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]

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?

[melekes]

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]

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]

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]

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]

is this PR good to go into master?