feat(alpenglow): introduce alpenclock primitives to bank#10331
feat(alpenglow): introduce alpenclock primitives to bank#10331ksn6 merged 2 commits intoanza-xyz:masterfrom
Conversation
ksn6
force-pushed
the
alpenglow-clock-primitives
branch
from
e26b9d3 to
67ca792
Compare
| if !self.feature_set.is_active(&feature_set::alpenglow::id()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Added these lines in to prevent accidental invocation.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10331 +/- ##
=========================================
- Coverage 83.0% 83.0% -0.1%
=========================================
Files 848 848
Lines 317293 317331 +38
=========================================
- Hits 263586 263560 -26
- Misses 53707 53771 +64 🚀 New features to boost your workflow:
|
ksn6
force-pushed
the
alpenglow-clock-primitives
branch
2 times, most recently
from
6e4ffa6 to
7dbdf6d
Compare
runtime/src/bank.rs
Outdated
| }); | ||
|
|
||
| // Update Alpenglow clock | ||
| let alpenclock_size = wincode::serialized_size(&unix_timestamp_nanos).unwrap(); |
There was a problem hiding this comment.
In the previous change, I had this as just inspecting the number of bytes in unix_timestamp_nanos, but that's brittle; not ideal that this requires adding in a dependency, though wincode likely gets introduced at some point anyways.
There was a problem hiding this comment.
I believe AccountSharedData::new_data below is using bincode
Should we just use wincode instead?
| // On epoch boundaries, update epoch_start_timestamp | ||
| let unix_timestamp_s = unix_timestamp_nanos / 1_000_000_000; | ||
| let epoch_start_timestamp = match (self.slot, self.parent()) { | ||
| (0, _) => self.unix_timestamp_from_genesis(), |
There was a problem hiding this comment.
This line is basically just for tests.
There was a problem hiding this comment.
is this function actually called for tests for the genesis block?
If we start a cluster with alpenglow enabled at genesis how is the initial value of the clock set?
Just want to make sure I completely understand the corner cases before we upstream
There was a problem hiding this comment.
is this function actually called for tests for the genesis block?
It's not; the genesis block's bank is created via new_from_genesis, which calls update_clock unconditionally. In update_clock, we have a check for whether slot == 0, and if that's the case, the clock is set to self.unix_timestamp_from_genesis().
You're correct in that we can probably get rid of this first match statement, though I'd rather keep this here for completeness.
I'll add in a comment for clarity, though.
Just want to make sure I completely understand the corner cases before we upstream
Writing this here for completeness - the rest of the banks are initialized via _new_from_parent, where there's this logic:
if new.get_alpenglow_genesis_certificate().is_none() {
new.update_clock(Some(parent.epoch()));
}During migration, this is the line that runs; concretely, on the zero'th slot (i.e., the zero'th Alpenglow slot), the genesis certificate should be set to None, meaning that we update the clock via the classic stake-weighted approach.
Later in the block, we then observe the footer, which updates the clock for the second time in the zero'th Alpenglow slot. So the clock is actually updated twice in the zero'th Alpenglow slot, when migrating from TowerBFT.
For Alpenglow slots 1+, the new.update_clock logic doesn't fire, because the Alpenglow genesis certificate is present; it's purely footer-based after that.
ksn6
force-pushed
the
alpenglow-clock-primitives
branch
from
7dbdf6d to
b3359dd
Compare
ksn6
force-pushed
the
alpenglow-clock-primitives
branch
from
2be126d to
667b480
Compare
| let data = wincode::serialize(&unix_timestamp_nanos).unwrap(); | ||
| let lamports = Rent::default().minimum_balance(data.len()); | ||
| let mut alpenclock_acct = AccountSharedData::new(lamports, data.len(), &system_program::ID); | ||
| alpenclock_acct.set_data_from_slice(&data); |
There was a problem hiding this comment.
no action for this PR, but since this wincode::serialize -> AccountSharedData::set_data_from_slice has popped up a couple times maybe we make a new constructor for it
3 participants
Problem
This PR upstreams the primitives needed within
Bankto implement the Alpenglow clock (i.e., the Alpenclock).See solana-foundation/solana-improvement-documents#363 for details on how the Alpenclock works.
Summary of Changes
In this PR, we introduce:
We feature-gate the setter to avoid accidental invocation prior to launching Alpenglow.