Remove pubkey from DecisionMaker

[apfitzge]

Problem

• DecisionMaker currently takes in a Pubkey which should be the ID of the node
• Since it only takes in the pubkey at creation, this will not actually be the leader/staked pubkey for many operating nodes
  • this means even if we were leader the pubkey comparison in DecisionMaker would likely fail
• DecisionMaker looks very shortly into the future, 2 slots. We should not be in a situation where there is not an EpochSchedule in the cache for that slot

Summary of Changes

• Remove pubkey from DecisionMaker
  • take path which was previously due to pubkey mismatch

Fixes #

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.2%. Comparing base (5222e94) to head (3e77501).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7077   +/-   ##
=======================================
  Coverage    83.2%    83.2%           
=======================================
  Files         854      854           
  Lines      374686   374613   -73     
=======================================
- Hits       312083   312025   -58     
+ Misses      62603    62588   -15     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bw-solana]

LGTM, but I'm too stupid to understand why we ever needed the old code.. so want to make sure I'm not missing something

[bw-solana]

I'm not understanding the case where we would even hit the Hold case for the old logic.

1. If we didn't know the leader (because we don't have the leader schedule), but in reality we generate the schedule so far in advance relative to how far ahead we look that this seems impossible outside of weird, contrived test cases.
2. If we somehow don't think we will be leader within 20 slots based on ticks but also think we will be leader in 2 slots based on the leader schedule. Is there even a corner case where this could happen?

[apfitzge]

Want to be clear, that we hit Hold very often. But there are 3 different cases in which the old logic could have resulted in Hold:

1. would_be_leader_shortly_fn returns true:
   • This is the one we hit a lot.
   • We are within 2 slots of being leader
   • This also includes when we are currently leader, but don't yet a Bank to operate on.
2. leader_pubkey_fn returns Some(x) where x IS the nodes ID pubkey (at start up)
   • We search 2 slots into the future in cached leader schedule. If it IS us, then we return Hold. BUT this can only happen if our POH has an inconsistent view of who is leader in slot N+2 from the leader-schedule cache.
   • In practice the if doesn't even work. Many operators have failover setup so that when they are starting up they are in a non-voting mode with a dummy key...since we grab it at start-up, we're comparing against the wrong key in this case anyway.
3. leader_pubkey_fn returns None
   • this shouldn't ever happen because we generate leader schedules ahead of time and only look a small offset into the future (2 slots)

[bw-solana]

yeah, sorry, 1 makes perfect sense. I was referring to the cases we're removing (2 and 3)

[apfitzge]

Right, and to summarize my comment - 2 and 3 should NOT happen unless there are other bugs, to the best of my knowledge.

[tao-stones]

looks good, thank for the explanations.

[apfitzge]

This is non-urgent, gonna wait for @jstarry's review since he is good at finding edge-cases in my understanding of poh.

[jstarry]

lgtm