remove consensus logic from replay #45

carllin · 2025-02-18T02:27:55Z

Problem

Replay lacks alpenglow awareness

Summary of Changes

Call the Poh migration path from replay, but doesn't handle lockouts/epoch boundaries safely yet
Handles starting a leader in replay
Handles pushing votes from replay

With this, we can now spin up a single alpenglow leader in a one node cluster and watch it make blocks

This builds on #44 and #43, only the last commit is relevant
Fixes #

wen-coding

Need to jump on the plane soon, I'll review rest of it later.

core/src/voting_service.rs

wen-coding · 2025-02-18T19:20:38Z

core/src/voting_service.rs

+        let leader_fanout = {
+            match &vote_op {
+                // Alpenglow relies on leaders to propagate votes otherwise we have to rely
+                // on gossip, especially true for skip votes


I thought we would send all-to-all as well? Otherwise we can't guarantee 3 delta... We can discuss this in SLC

wen-coding · 2025-02-18T19:29:21Z

gossip/src/cluster_info.rs

+        let mut num_crds_votes = 0;
+        let vote_index = {
+            let gossip_crds =
+                self.time_gossip_read_lock("gossip_read_push_vote", &self.stats.push_vote_read);


nit: push_alpenglow_vote

And maybe give alpenglow vote its own stats?

wen-coding · 2025-02-18T19:30:29Z

gossip/src/cluster_info.rs

+                self.time_gossip_read_lock("gossip_read_push_vote", &self.stats.push_vote_read);
+            (0..MAX_LOCKOUT_HISTORY as u8)
+                .filter_map(|ix| {
+                    let vote = CrdsValueLabel::Vote(ix, self_pubkey);


And our own label as well, don't share the same queue with the old votes.

wen-coding · 2025-02-18T19:32:57Z

gossip/src/cluster_info.rs

+        let vote_index = {
+            let gossip_crds =
+                self.time_gossip_read_lock("gossip_read_push_vote", &self.stats.push_vote_read);
+            (0..MAX_LOCKOUT_HISTORY as u8)


Is 32 enough for Alpenglow? You are sharing the same vote buffer for Notar/Skip/Final, and in the async execution world Final might be further away from Notar.

wen-coding · 2025-02-18T19:36:31Z

gossip/src/cluster_info.rs

+                    let vote: &CrdsData = gossip_crds.get(&vote)?;
+                    num_crds_votes += 1;
+                    match &vote {
+                        CrdsData::Vote(_, vote) => Some((vote.wallclock, ix)),


Wait, there was a should_evict_vote check in the original code, we don't need it any more?

Also we need to think carefully whether we should evict the oldest vote. Could all the finalize votes be kicked out because we have too many skip/notar on the newer votes?

I'm inclined to say "please give each type of vote its own queue" now...

wen-coding · 2025-02-25T00:36:28Z

core/src/banking_stage.rs

 ) -> bool {
    let tpu_bank = bank_forks.write().unwrap().insert(tpu_bank);
    let parent_slot = tpu_bank.parent_slot();
+    info!(


Do we need the info log here? Can we just log at maybe_start_leader?

wen-coding · 2025-02-25T01:25:02Z

core/src/replay_stage.rs

+                .unwrap()
+                .root_bank()
+                .feature_set
+                .activated_slot(&solana_feature_set::alpenglow::id());


When validators enter the next Epoch at different speed, would it happen that some validators don't have enough votes for root to enter the new Epoch, so they were forever stuck in the old Epoch? (Because there are no new TowerBFT votes to confirm the fork they are on maybe?)

Should we enable Alpenglow at Epoch N+2 to make sure everyone's on the same page?

Also, we are checking this outside the loop? Could we enter a new Epoch inside the loop?

migration will not be blocked on votes from the previous epoch, you just need to respect lockout and switching rules for all proposed alpenglow slots until at least one alpenglow slot gets a notarization

wen-coding · 2025-02-25T22:21:08Z

core/src/replay_stage.rs

-                let mut process_duplicate_slots_time = Measure::start("process_duplicate_slots");
-                if !tpu_has_bank {
-                    Self::process_duplicate_slots(
+                if !is_alpenglow_migration_complete {


Is it possible to move all non_alpenglow operations into a separate function for readability?

wen-coding · 2025-02-25T22:22:43Z

core/src/replay_stage.rs

+                        retransmit_not_propagated_time.as_us(),
+                    );
+                } else {
+                    // Alpenglow specific logic


similarly, move all Alpenglow specific logic into separate function for better readability?

yup, refactored

wen-coding · 2025-02-26T00:28:13Z

core/src/replay_stage.rs

+                        // Note that like today, if this occurs during your leader slot
+                        // it will cause you to dump your leader slot. This should be ok because
+                        // the fact that there is a greater/valid slot than your own must mean there
+                        // was a skip certificate for your slot, so it's ok to abandon your leader slot


Hmm, your comment implies:
if poh_recorder.read().unwrap().start_slot() < highest_frozen_bank.slot() {
but your actual comparison uses !=
Should we reset poh_recorder if start_slot is larger than highest_frozen_bank.slot()?

It's impossible for start slot > highest_frozen_bank, we only ever start leader banks from parents that are

notarized

frozen

which necessarily means the highest frozen bank > start slot, added an assert and a comment

wen-coding · 2025-02-26T00:57:16Z

core/src/replay_stage.rs

-                );
-                retransmit_not_propagated_time.stop();
+                    // Try to finalize the highest notarized block
+                    let highest_notarized_slot = cert_pool.highest_notarized_slot();


Should we finalize the highest non-finalized block instead?

Could it be notarizations keep coming:
Slot 100 notarized -> Didn't get to finalize Slot 100 before Slot 101 is also notarized -> Didn't get to finalize Slot 101 before Slot 102 is also notarized ...

I know we will eventually get all of them finalized, but that might mean Slot 100 finalization takes more than 3 delta.

wen-coding · 2025-02-26T00:58:03Z

core/src/replay_stage.rs

+                        let maybe_vote_bank =
+                            bank_forks.read().unwrap().get(highest_notarized_slot);
+                        if let Some(vote_bank) = maybe_vote_bank {
+                            if vote_bank.is_frozen() {


If highest_notarized_slot is not frozen, I think we should start repair somewhere?

repair will happen in the background like it does today based on votes in gossip, we just need to have the repair structure read the new votes #56

core/src/replay_stage.rs

wen-coding · 2025-02-26T23:50:49Z

core/src/replay_stage.rs

+        if authorized_voter_keypairs.is_empty() {
+            return GenerateVoteTxResult::NonVoting;
+        }
+        if let Some(slot) = wait_to_vote_slot {


Does this still work in Alpenglow? In PoH if you wait long enough some leader will eventually start building for a future slot X you are waiting on, but I believe now we need to send skip?

I think this is only used to manipulate tests in local cluster, should be fine

wen-coding · 2025-03-01T05:26:10Z

core/src/replay_stage.rs

+                Self::initiate_alpenglow_migration(poh_recorder, is_alpenglow_migration_complete);
+            }
+        }
+


Can't comment on line 2254:
Suddenly confused:

if parent_slot < *first_alpenglow_slot.as_ref().unwrap_or(&u64::MAX) { assert!(parent_bank.is_frozen()); }

Why don't we need to make sure parent_bank is frozen if we are past first alpenglow slot?

Ah yeah, this is a bug from moving the start leader logic out of the certificate pool, where we used to check the alpenglow slot is frozen before starting the leader.

Because in alpenglow mode we set the parent_slot to cert_pool.highest_not_skip_certificate_slot(), we can have a certificate for a non frozen slot if our replay is slow. This assert exists in today's code because parent bank is always frozen

We need to return in alpenglow mode if the parent bank isn't frozen. I need to refactor this alpegnlow-specific logic anyways so will do that

Sounds perfect. When you refactor, please try to split TowerBFT and Alpenglow logic as much as possible. It's really hard to read the code when the two are mixed together. I know we have to do it somehow for the migration, but hopefully we can at least split things into smaller functions.

split up the logic in maybe_start_leader()

carllin requested review from AshwinSekar and wen-coding February 18, 2025 02:27

wen-coding reviewed Feb 18, 2025

View reviewed changes

wen-coding reviewed Feb 25, 2025

View reviewed changes

wen-coding reviewed Feb 26, 2025

View reviewed changes

wen-coding reviewed Feb 27, 2025

View reviewed changes

carllin marked this pull request as ready for review February 28, 2025 01:09

carllin changed the title ~~Add simple Alpenglow migration to Replay~~ Add simple Alpenglow start leader logic to Replay Feb 28, 2025

carllin changed the title ~~Add simple Alpenglow start leader logic to Replay~~ Add Alpenglow start leader logic to Replay Feb 28, 2025

carllin changed the title ~~Add Alpenglow start leader logic to Replay~~ Add Alpenglow start leader logic to ReplayStage Feb 28, 2025

carllin force-pushed the master branch 2 times, most recently from fcf67b2 to d9bd8cd Compare March 1, 2025 04:45

wen-coding reviewed Mar 1, 2025

View reviewed changes

carllin force-pushed the master branch from d9bd8cd to 39e2fad Compare March 1, 2025 08:52

carllin added 14 commits March 1, 2025 03:53

add alpenglow migration logic to replay

26cf9af

Fixup PR comments

1c2a318

Rename properly

541318f

Refactor out alpenglow specific logic

8291d61

Refactor alpenglow specific logic

ed7949a

Coalesce all handle_new_root logic into one function

4f980ef

remove log

32402c7

fixup

f146950

implement abi

2c72e7c

Hack for AbiExample for Vote

802aa29

Don't start alpenglow slot if parent is not frozen

e71f339

Factor out poh vs alpenglow start leader

47be5da

purge cert pool on new root

902ea08

Factor out alpenglow_handle_new_root

e116c40

carllin merged commit fdf2f25 into anza-xyz:master Mar 3, 2025
7 checks passed

AshwinSekar added this to Alpenglow Jun 13, 2025

AshwinSekar moved this to Pending migration in Alpenglow Jun 13, 2025

AshwinSekar changed the title ~~Add Alpenglow start leader logic to ReplayStage~~ remove consensus logic from replay Jun 13, 2025

bw-solana pushed a commit to bw-solana/alpenglow that referenced this pull request Aug 1, 2025

Add Alpenglow start leader logic to ReplayStage (anza-xyz#45)

317be0c

bw-solana pushed a commit to bw-solana/alpenglow that referenced this pull request Aug 1, 2025

Add Alpenglow start leader logic to ReplayStage (anza-xyz#45)

499b898

bw-solana pushed a commit to bw-solana/alpenglow that referenced this pull request Aug 1, 2025

Add Alpenglow start leader logic to ReplayStage (anza-xyz#45)

d972ac0

bw-solana pushed a commit to bw-solana/alpenglow that referenced this pull request Aug 1, 2025

Add Alpenglow start leader logic to ReplayStage (anza-xyz#45)

5942217

bw-solana pushed a commit to bw-solana/alpenglow that referenced this pull request Aug 1, 2025

Add Alpenglow start leader logic to ReplayStage (anza-xyz#45)

db327ce

bw-solana pushed a commit to bw-solana/alpenglow that referenced this pull request Aug 1, 2025

Add Alpenglow start leader logic to ReplayStage (anza-xyz#45)

313446b

bw-solana pushed a commit to bw-solana/alpenglow that referenced this pull request Aug 2, 2025

Add Alpenglow start leader logic to ReplayStage (anza-xyz#45)

5e91a3c

AshwinSekar removed this from Alpenglow Aug 7, 2025

remove consensus logic from replay #45

remove consensus logic from replay #45

Uh oh!

Conversation

carllin commented Feb 18, 2025

Problem

Summary of Changes

Uh oh!

wen-coding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carllin Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

carllin Feb 28, 2025 •

edited

Loading