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

Scheduler: Improve TTL #3161

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

apfitzge
Copy link

Problem

  • Scheduler tracks the end of epoch slot so that transactions are re-sanitized if the epoch rolls over
  • Does not tell us when ALTs may expire

Summary of Changes

  • track when ALTs expire to avoid re-checking ALT resolution every time
  • re-resolve if the ALT expiration is hit

Fixes #

@apfitzge apfitzge marked this pull request as ready for review October 14, 2024 22:19
@apfitzge apfitzge requested a review from jstarry October 14, 2024 22:19
@@ -81,12 +81,14 @@ impl Accounts {
}
}

/// Return loaded addresses and the deactivation slot.
/// If no tables are de-activating, the deactivation slot is `u64::MAX`.
Copy link

Choose a reason for hiding this comment

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

nit: this function only looks up addresses for one ALT

Suggested change
/// If no tables are de-activating, the deactivation slot is `u64::MAX`.
/// If the table hasn't been deactivated, the deactivation slot is `u64::MAX`.

deactivation_slot: Slot,
current_slot: Slot,
) -> MaxAge {
let alt_min_expire_slot = estimate_last_valid_slot(deactivation_slot.min(current_slot));
Copy link

Choose a reason for hiding this comment

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

Technically this should be current_slot + 1 I think because we know that the ALT didn't get deactivated in the current_slot

}
}

impl Bank {
/// Load addresses from an iterator of `SVMMessageAddressTableLookup`.
/// Load addresses from an iterator of `SVMMessageAddressTableLookup`,
/// additionally returning the deactivation slot
Copy link

Choose a reason for hiding this comment

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

Suggested change
/// additionally returning the deactivation slot
/// additionally returning the minimum deactivation slot across all referenced ALTs

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.

looks good to me, definitely not great that we were reloading ALTs after just doing that work in the scheduler.. can we backport this to v2.0?

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.

2 participants