refactor: rename PohRecorder::is_same_fork_as_previous_leader
#4939
Conversation
poh/src/poh_recorder.rs
Outdated
| @@ -432,7 +432,7 @@ impl PohRecorder { | |||
| self.leader_bank_notifier.clone() | |||
| } | |||
|
|
|||
| fn is_same_fork_as_previous_leader(&self, slot: Slot) -> bool { | |||
| fn start_slot_was_mine_or_previous_leader(&self, slot: Slot) -> bool { | |||
There was a problem hiding this comment.
I'm thinking this should just be "start slot was previous leader" even though it would technically return true if we happen to be starting from our own slot.
Rationale is:
- If it's actually our block, we would want to skip grace ticks.
- Immediately before this, we check the start bank collector ID to determine if it was ours, so practically speaking, it shouldn't return true due to matching on our slot.
There was a problem hiding this comment.
I'm more in favor of explicitly documenting the true behavior to avoid misuse of this method in the future. It's true that the current usage effectively means "start slot was previous leader" but that depends on first calling fn start_slot_was_mine. If fn start_slot_was_mine isn't called first, it's possible that we return false for fn can_skip_grace_ticks when building off our own slot because fn start_slot_was_mine_or_previous_leader would be true and fn building_off_previous_leader_last_block would be false.
There was a problem hiding this comment.
that makes sense. Although it depends on the slot being passed in (obviously expectation being it is the next leader slot this node will attempt to build). Maybe we should s/slot/my_next_leader_slot/ ?)
or maybe I'm just overthinking this one :)
what you have is definitely an improvement
There was a problem hiding this comment.
Yeah I'll update that param name, it was bothering me too
There was a problem hiding this comment.
Updated to next_leader_slot everywhere
jstarry
force-pushed
the
refactor/rename-poh-fn
branch
from
97655f6 to
89039e8
Compare
poh/src/poh_recorder.rs
Outdated
| @@ -432,7 +432,7 @@ impl PohRecorder { | |||
| self.leader_bank_notifier.clone() | |||
| } | |||
|
|
|||
| fn is_same_fork_as_previous_leader(&self, slot: Slot) -> bool { | |||
| fn start_slot_was_mine_or_previous_leader(&self, slot: Slot) -> bool { | |||
There was a problem hiding this comment.
that makes sense. Although it depends on the slot being passed in (obviously expectation being it is the next leader slot this node will attempt to build). Maybe we should s/slot/my_next_leader_slot/ ?)
or maybe I'm just overthinking this one :)
what you have is definitely an improvement
Problem
is_same_fork_as_previous_leaderis not a great name. The previous leader could produce 4 different forks with each of its leader blocks so saying "is same fork as previous leader" is kinda meaningless. This function is really checking if the start bank was produced by itself or the current leader.Summary of Changes
Rename for clarity
Fixes #