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.

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

Respect grace ticks #2354

Merged
merged 4 commits into from
Aug 1, 2024
Merged

Conversation

bw-solana
Copy link

@bw-solana bw-solana commented Jul 30, 2024

Problem

There's a bug where we will not respect grace ticks (currently set to 2 slots worth) because once we tick through 1 slot worth of ticks, we think we will now be building off our own block (or at least have produced a block) and don't need to respect grace ticks.

Summary of Changes

  • Instead of deriving the previous slot from ticks and using the leader cache to extract the pubkey, just grab it from the start bank. Note that this is a subtle algorithm change
  • Update test_reached_leader_tick to cover all the possible paths.

Notes

  1. I'm hoping this PR can be a discussion starter. One potential problem I see is that it's possible we already produced (or partially produced) our first block but have now switched the fork we're building off of. This would causes us to respect grace ticks even if we don't really have to. I think this is an obscure case with relatively benign consequences, but I want to make sure this side effect isn't ignored.
  2. There's still work to do... by respecting grace ticks, the current leader is potentially losing slots. This is because the next_poh_slot computed in reached_leader_slot is based on the current tick_height

@bw-solana bw-solana marked this pull request as ready for review July 30, 2024 19:45
@bw-solana bw-solana requested review from jstarry and AshwinSekar July 30, 2024 19:45
Copy link

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

Nice, this was a straightforward fix!

poh/src/poh_recorder.rs Outdated Show resolved Hide resolved
poh/src/poh_recorder.rs Outdated Show resolved Hide resolved
@bw-solana bw-solana force-pushed the respect_grace_ticks branch from e4cb4ef to 80bb3d0 Compare July 31, 2024 14:01
@bw-solana bw-solana requested a review from jstarry July 31, 2024 14:03
@mergify mergify bot mentioned this pull request Jul 31, 2024
jstarry
jstarry previously approved these changes Jul 31, 2024
@jstarry
Copy link

jstarry commented Jul 31, 2024

changes look good but there's a failing poh related test

AshwinSekar
AshwinSekar previously approved these changes Jul 31, 2024
Copy link

@AshwinSekar AshwinSekar left a comment

Choose a reason for hiding this comment

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

I'm in favor of this change.

One potential problem I see is that it's possible we already produced (or partially produced) our first block but have now switched the fork we're building off of. This would causes us to respect grace ticks even if we don't really have to.

As a follow up I think we should keep track of whether we have already waited for grace ticks / produced any shreds for our current leader slots in order to skip waiting again.

This is because the next_poh_slot computed in reached_leader_slot is based on the current tick_height

I am also in favor of always producing the earliest slot we can once we reach grace ticks. Otherwise it seems like a slow previous leader can always cause you to skip half your slots.

Also it might be too early to call without some more data, but it seems like turning delay_leader_block_for_pending_fork on as the default is beneficial. Of the 7 slots we saw on mainnet recently that fit this category:

279650852
279589292
279773684
280090288
280260040
280519864
280352012

All were skipped with the flag off

@bw-solana bw-solana dismissed stale reviews from AshwinSekar and jstarry via 9cc3eda August 1, 2024 00:47
@bw-solana
Copy link
Author

changes look good but there's a failing poh related test

Had to update this test to pass in the pubkey from the PoH start bank so it would match, conclude that previous slot is ours, ignore grace ticks, and thus return we've reached our leader slot.

@bw-solana bw-solana merged commit c8685ce into anza-xyz:master Aug 1, 2024
42 checks passed
@bw-solana bw-solana deleted the respect_grace_ticks branch August 1, 2024 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants