Skip to content

migration: validate that migrationary blocks are VoM #566

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
Nov 3, 2025
Merged

migration: validate that migrationary blocks are VoM #566

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
18 changes: 16 additions & 2 deletions core/src/replay_stage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,6 +809,7 @@ impl ReplayStage {
&slot_status_notifier,
&mut progress,
&mut replay_timing,
migration_status.as_ref(),
);
generate_new_bank_forks_time.stop();

Expand Down Expand Up @@ -4359,6 +4360,7 @@ impl ReplayStage {
slot_status_notifier: &Option<SlotStatusNotifier>,
progress: &mut ProgressMap,
replay_timing: &mut ReplayLoopTiming,
migration_status: &MigrationStatus,
) {
// Find the next slot that chains to the old slot
let mut generate_new_bank_forks_read_lock =
Expand Down Expand Up @@ -4406,14 +4408,21 @@ impl ReplayStage {
parent_slot,
forks.root()
);
// Migration period banks are VoM
let options = NewBankOptions {
vote_only_bank: migration_status.should_bank_be_vote_only(child_slot),
};
if options.vote_only_bank {
info!("Replaying block in slot {child_slot} in VoM");
}
let child_bank = Self::new_bank_from_parent_with_notify(
parent_bank.clone(),
child_slot,
forks.root(),
&leader,
rpc_subscriptions,
slot_status_notifier,
NewBankOptions::default(),
options,
);
blockstore_processor::set_alpenglow_ticks(&child_bank);
let empty: Vec<Pubkey> = vec![];
Expand All @@ -4433,7 +4442,6 @@ impl ReplayStage {
let mut generate_new_bank_forks_write_lock =
Measure::start("generate_new_bank_forks_write_lock");

// TODO(ksn): should we have this if-statement check?
if !new_banks.is_empty() {
let mut forks = bank_forks.write().unwrap();
let root = forks.root();
Expand Down Expand Up @@ -4805,6 +4813,7 @@ pub(crate) mod tests {
&None,
&mut progress,
&mut replay_timing,
&MigrationStatus::default(),
);
assert!(bank_forks
.read()
Expand All @@ -4829,6 +4838,7 @@ pub(crate) mod tests {
&None,
&mut progress,
&mut replay_timing,
&MigrationStatus::default(),
);
assert!(bank_forks
.read()
Expand Down Expand Up @@ -6732,6 +6742,7 @@ pub(crate) mod tests {
&None,
&mut progress,
&mut replay_timing,
&MigrationStatus::default(),
);
assert_eq!(bank_forks.read().unwrap().active_bank_slots(), vec![3]);

Expand Down Expand Up @@ -6762,6 +6773,7 @@ pub(crate) mod tests {
&None,
&mut progress,
&mut replay_timing,
&MigrationStatus::default(),
);
assert_eq!(bank_forks.read().unwrap().active_bank_slots(), vec![5]);

Expand Down Expand Up @@ -6793,6 +6805,7 @@ pub(crate) mod tests {
&None,
&mut progress,
&mut replay_timing,
&MigrationStatus::default(),
);
assert_eq!(bank_forks.read().unwrap().active_bank_slots(), vec![6]);

Expand Down Expand Up @@ -6823,6 +6836,7 @@ pub(crate) mod tests {
&None,
&mut progress,
&mut replay_timing,
&MigrationStatus::default(),
);
assert_eq!(bank_forks.read().unwrap().active_bank_slots(), vec![7]);
}
Expand Down
27 changes: 23 additions & 4 deletions ledger/src/blockstore_processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -833,6 +833,9 @@ pub enum BlockstoreProcessorError {

#[error("non consecutive leader slot for bank {0} parent {1}")]
NonConsecutiveLeaderSlot(Slot, Slot),

#[error("user transactions found in vote only mode bank at slot {0}")]
UserTransactionsInVoteOnlyBank(Slot),
}

/// Callback for accessing bank state after each slot is confirmed while
Expand Down Expand Up @@ -1673,14 +1676,30 @@ fn confirm_slot_entries(
.expect("Transaction verification generates entries");

let mut replay_timer = Measure::start("replay_elapsed");
let is_vote_only_bank = bank.vote_only_bank();
let replay_entries: Vec<_> = entries
.into_iter()
.zip(entry_tx_starting_indexes)
.map(|(entry, tx_starting_index)| ReplayEntry {
entry,
starting_index: tx_starting_index,
.map(|(entry, tx_starting_index)| {
// If bank is in vote-only mode, validate that entries contain only vote transactions
Copy link
Contributor Author

@AshwinSekar AshwinSekar Oct 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

for some context the reason we never validated this before was because VoM is based on a local-trigger (tower distance) so we could end up rejecting blocks from honest nodes.

I believe that this is safe, since the migration slot is known via a rooted bank. The only scenario where this could be an issue is:

  • Migration happens very quickly, first vote only bank is S
  • Everyone purges S and it's descendants and we remake S and further slots as alpenglow
  • Our node is really slow / has network issues somehow misses the tower bft blocks, and instead sees the alpenglow version of S and its descendants.

In this scenario we'd mark the Alpenglow block as dead because it's not vote only - however we would already mark it as dead because it's missing PoH ticks.

The only way to remedy this situation is to wait for our slow node to eventually see the genesis certificate / a finalization certificate and then we'd purge, repair, and re-replay the Alpenglow blocks as not vote only.

Copy link
Contributor

@bw-solana bw-solana Oct 24, 2025

Choose a reason for hiding this comment

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

For the current, Tower case, we only ever mark are our own leader slot banks as VoM, right?

So they wouldn't actually go through this filtering process IIUC, which seems fine.

Copy link
Contributor Author

@AshwinSekar AshwinSekar Oct 24, 2025

Choose a reason for hiding this comment

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

yep we don't validate (and with this change won't validate) tower blocks as VoM.

Only concern is if this validation could screw up something in Alpenglow which I don't believe it can

Copy link
Contributor

@carllin carllin Oct 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Our node is really slow / has network issues somehow misses the tower bft blocks, and instead sees the alpenglow version of S and its descendants.

If we check for the migration certificate in the block header, could we avoid this?

Copy link
Contributor Author

@AshwinSekar AshwinSekar Nov 3, 2025

Choose a reason for hiding this comment

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

If we check for the migration certificate in the block header, could we avoid this?

Yep, since we will have the genesis certificate in the BlockMarker, at this point we can get to ReadyToEnable and then progress with the migration.

if let EntryType::Transactions(ref transactions) = entry {
if is_vote_only_bank
&& transactions
.iter()
.any(|tx| !tx.is_simple_vote_transaction())
Comment on lines +1685 to +1689
Copy link
Contributor

@bw-solana bw-solana Oct 24, 2025

Choose a reason for hiding this comment

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

thinking about performance, this seems fine. is_vote_only_bank always false except during the transition (in which case load is already lighter. And even then, I believe these are RuntimeTransaction types, so the is_simple_vote_transaction is super cheap

Copy link
Contributor

@carllin carllin Oct 25, 2025

Choose a reason for hiding this comment

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

Also, is the check on number of transactions is bounded at this point based on shred count and compute units?

Copy link
Contributor Author

@AshwinSekar AshwinSekar Nov 3, 2025

Choose a reason for hiding this comment

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

Yes we have block limits in form of CUs and maximum number of shreds amongst other things

{
return Err(BlockstoreProcessorError::UserTransactionsInVoteOnlyBank(
bank.slot(),
));
}
}
Ok(ReplayEntry {
entry,
starting_index: tx_starting_index,
})
})
.collect();
.collect::<result::Result<Vec<_>, _>>()?;

let process_result = process_entries(
bank,
replay_tx_thread_pool,
Expand Down