Skip to content

PohService needs to set use_alpenglow_tick_producer flag on startup #59

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 28, 2025
Merged

PohService needs to set use_alpenglow_tick_producer flag on startup #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions poh/src/poh_recorder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ pub struct PohRecorder {
last_reported_slot_for_pending_fork: Arc<Mutex<Slot>>,
pub is_exited: Arc<AtomicBool>,
pub is_alpenglow_enabled: bool,
pub use_alpenglow_tick_produer: bool,
pub use_alpenglow_tick_producer: bool,
}

impl PohRecorder {
Expand Down Expand Up @@ -651,7 +651,7 @@ impl PohRecorder {

fn reset_poh(&mut self, reset_bank: Arc<Bank>, reset_start_bank: bool) {
let blockhash = reset_bank.last_blockhash();
let hashes_per_tick = if self.use_alpenglow_tick_produer {
let hashes_per_tick = if self.use_alpenglow_tick_producer {
None
} else {
*reset_bank.hashes_per_tick()
Expand Down Expand Up @@ -1142,7 +1142,7 @@ impl PohRecorder {
last_reported_slot_for_pending_fork: Arc::default(),
is_exited,
is_alpenglow_enabled,
use_alpenglow_tick_produer: is_alpenglow_enabled,
use_alpenglow_tick_producer: is_alpenglow_enabled,
},
working_bank_receiver,
record_receiver,
Expand Down
23 changes: 12 additions & 11 deletions poh/src/poh_service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,24 +159,25 @@ impl PohService {
&& poh_recorder.read().unwrap().is_alpenglow_enabled
Copy link
Contributor

@wen-coding wen-coding Feb 28, 2025

Choose a reason for hiding this comment

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

Do you still need this test? Maybe just say the only way it exits the previous loop is either:

  1. poh_exit is true
  2. alpenglow is enabled
    So only exit the loop if 1 is true.
    Then you can move the info loop to line 169~175?

Copy link
Contributor Author

@carllin carllin Feb 28, 2025

Choose a reason for hiding this comment

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

The info log is only necessary if we started in not alpenglow mode, and then migrated to alpenglow. If we started in alpenglow mode, there's no need to print that we're migrating

Copy link
Contributor

@wen-coding wen-coding Feb 28, 2025

Choose a reason for hiding this comment

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

Hmm ok, we can also do:
if poh_exit.load(Ordering::Relaxed) {
return;
} else { // must exit because of Alpenglow
info(...)
}
Your choice.

Copy link
Contributor Author

@carllin carllin Feb 28, 2025

Choose a reason for hiding this comment

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

updated!

Copy link
Contributor Author

@carllin carllin Feb 28, 2025

Choose a reason for hiding this comment

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

hmm actually I might like the original better, I want to explicitly check that alpenglow was enabled in case somebody else adds another path/error on which poh would exit

Copy link
Contributor

@wen-coding wen-coding Feb 28, 2025

Choose a reason for hiding this comment

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

Ok, that's fair.

{
info!("Migrating poh service to alpenglow tick producer");
// Important this is called *before* any new alpenglow
// leaders call `set_bank()`, otherwise, the old PoH
// tick producer will still tick in that alpenglow bank
//
// TODO: Can essentailly replace this with no ticks
// once we properly remove poh/entry verification in replay
{
let mut w_poh_recorder = poh_recorder.write().unwrap();
w_poh_recorder.migrate_to_alpenglow_poh();
w_poh_recorder.use_alpenglow_tick_produer = true;
}
} else {
poh_exit.store(true, Ordering::Relaxed);
return;
}
}

// Start alpenglow
//
// Important this is called *before* any new alpenglow
// leaders call `set_bank()`, otherwise, the old PoH
// tick producer will still tick in that alpenglow bank
//
// TODO: Can essentailly replace this with no ticks
// once we properly remove poh/entry verification in replay
{
let mut w_poh_recorder = poh_recorder.write().unwrap();
w_poh_recorder.migrate_to_alpenglow_poh();
w_poh_recorder.use_alpenglow_tick_producer = true;
}
Self::alpenglow_tick_producer(poh_recorder, &poh_exit, record_receiver);
poh_exit.store(true, Ordering::Relaxed);
})
Expand Down