From 01cd55a27a7dadd971ac3b2c789985add991d3be Mon Sep 17 00:00:00 2001 From: steviez Date: Thu, 1 Dec 2022 14:42:35 -0600 Subject: [PATCH] Change SlotMeta is_connected bool to bitflags (#29001) We currently use the is_connected field to be able to signal to ReplayStage that a slot has replayable updates. It was discovered that this functionality is effectively broken, and that is_connected is never true. In order to convey this information to ReplayStage more effectively, we need extra state information so this PR changes the existing bool to bitflags with two bits. From a compatibility standpoint, the is_connected bool was already occupying one byte in the serialized SlotMeta in blockstore. Thus, the change from a bool to bitflags still "fits" in that one byte allotment. In consideration of a case where a client may wish to downgrade software and use the same ledger, deserializing the bitflags into a bool could fail if the new bit is set. As such, this PR introduces the second bit field, but does not set it anywhere. Once clusters have mass adopted a software version with this PR, a subsequent change to actually set and use the new field can be introduced. --- ledger/src/blockstore.rs | 36 ++++----- ledger/src/blockstore_meta.rs | 133 +++++++++++++++++++++++++++++++++- 2 files changed, 148 insertions(+), 21 deletions(-) diff --git a/ledger/src/blockstore.rs b/ledger/src/blockstore.rs index b0753b3aaa..08760786ed 100644 --- a/ledger/src/blockstore.rs +++ b/ledger/src/blockstore.rs @@ -3728,13 +3728,13 @@ fn handle_chaining_for_slot( // connected to trunk of the ledger let should_propagate_is_connected = is_newly_completed_slot(&RefCell::borrow(meta), meta_backup) - && RefCell::borrow(meta).is_connected; + && RefCell::borrow(meta).is_connected(); if should_propagate_is_connected { // slot_function returns a boolean indicating whether to explore the children // of the input slot let slot_function = |slot: &mut SlotMeta| { - slot.is_connected = true; + slot.set_connected(); // We don't want to set the is_connected flag on the children of non-full // slots @@ -3815,7 +3815,9 @@ fn chain_new_slot_to_prev_slot( current_slot_meta: &mut SlotMeta, ) { prev_slot_meta.next_slots.push(current_slot); - current_slot_meta.is_connected = prev_slot_meta.is_connected && prev_slot_meta.is_full(); + if prev_slot_meta.is_connected() && prev_slot_meta.is_full() { + current_slot_meta.set_connected(); + } } fn is_newly_completed_slot(slot_meta: &SlotMeta, backup_slot_meta: &Option) -> bool { @@ -3829,7 +3831,7 @@ fn slot_has_updates(slot_meta: &SlotMeta, slot_meta_backup: &Option) - // from block 0, which is true iff: // 1) The block with index prev_block_index is itself part of the trunk of consecutive blocks // starting from block 0, - slot_meta.is_connected && + slot_meta.is_connected() && // AND either: // 1) The slot didn't exist in the database before, and now we have a consecutive // block for that slot @@ -4817,7 +4819,7 @@ pub mod tests { assert_eq!(meta.parent_slot, Some(0)); assert_eq!(meta.last_index, Some(num_shreds - 1)); assert!(meta.next_slots.is_empty()); - assert!(meta.is_connected); + assert!(meta.is_connected()); } #[test] @@ -5337,7 +5339,7 @@ pub mod tests { let meta1 = blockstore.meta(1).unwrap().unwrap(); assert!(meta1.next_slots.is_empty()); // Slot 1 is not trunk because slot 0 hasn't been inserted yet - assert!(!meta1.is_connected); + assert!(!meta1.is_connected()); assert_eq!(meta1.parent_slot, Some(0)); assert_eq!(meta1.last_index, Some(shreds_per_slot as u64 - 1)); @@ -5349,7 +5351,7 @@ pub mod tests { let meta2 = blockstore.meta(2).unwrap().unwrap(); assert!(meta2.next_slots.is_empty()); // Slot 2 is not trunk because slot 0 hasn't been inserted yet - assert!(!meta2.is_connected); + assert!(!meta2.is_connected()); assert_eq!(meta2.parent_slot, Some(1)); assert_eq!(meta2.last_index, Some(shreds_per_slot as u64 - 1)); @@ -5357,7 +5359,7 @@ pub mod tests { // but still isn't part of the trunk let meta1 = blockstore.meta(1).unwrap().unwrap(); assert_eq!(meta1.next_slots, vec![2]); - assert!(!meta1.is_connected); + assert!(!meta1.is_connected()); assert_eq!(meta1.parent_slot, Some(0)); assert_eq!(meta1.last_index, Some(shreds_per_slot as u64 - 1)); @@ -5376,7 +5378,7 @@ pub mod tests { assert_eq!(meta.parent_slot, Some(slot - 1)); } assert_eq!(meta.last_index, Some(shreds_per_slot as u64 - 1)); - assert!(meta.is_connected); + assert!(meta.is_connected()); } } @@ -5435,9 +5437,9 @@ pub mod tests { } if slot == 0 { - assert!(meta.is_connected); + assert!(meta.is_connected()); } else { - assert!(!meta.is_connected); + assert!(!meta.is_connected()); } } @@ -5462,7 +5464,7 @@ pub mod tests { assert_eq!(meta.parent_slot, Some(slot - 1)); } assert_eq!(meta.last_index, Some(shreds_per_slot as u64 - 1)); - assert!(meta.is_connected); + assert!(meta.is_connected()); } } @@ -5508,10 +5510,10 @@ pub mod tests { // Additionally, slot 0 should be the only connected slot if slot == 0 { assert_eq!(meta.parent_slot, Some(0)); - assert!(meta.is_connected); + assert!(meta.is_connected()); } else { assert_eq!(meta.parent_slot, Some(slot - 1)); - assert!(!meta.is_connected); + assert!(!meta.is_connected()); } assert_eq!(meta.last_index, Some(shreds_per_slot as u64 - 1)); @@ -5532,9 +5534,9 @@ pub mod tests { assert!(meta.next_slots.is_empty()); } if slot <= slot_index + 3 { - assert!(meta.is_connected); + assert!(meta.is_connected()); } else { - assert!(!meta.is_connected); + assert!(!meta.is_connected()); } if slot == 0 { @@ -5614,7 +5616,7 @@ pub mod tests { let slot_meta = blockstore.meta(slot).unwrap().unwrap(); assert_eq!(slot_meta.consumed, entries_per_slot); assert_eq!(slot_meta.received, entries_per_slot); - assert!(slot_meta.is_connected); + assert!(slot_meta.is_connected()); let slot_parent = { if slot == 0 { 0 diff --git a/ledger/src/blockstore_meta.rs b/ledger/src/blockstore_meta.rs index 5cacf78198..8394d1a8a6 100644 --- a/ledger/src/blockstore_meta.rs +++ b/ledger/src/blockstore_meta.rs @@ -1,5 +1,6 @@ use { crate::shred::{Shred, ShredType}, + bitflags::bitflags, serde::{Deserialize, Deserializer, Serialize, Serializer}, solana_sdk::{ clock::{Slot, UnixTimestamp}, @@ -11,6 +12,43 @@ use { }, }; +bitflags! { + #[derive(Deserialize, Serialize)] + /// Flags to indicate whether a slot is a descendant of a slot on the main fork + pub struct ConnectedFlags:u8 { + // A slot S should be considered to be connected if: + // 1) S is a rooted slot itself OR + // 2) S's parent is connected AND S is full (S's complete block present) + // + // 1) is a straightfoward case, roots are finalized blocks on the main fork + // so by definition, they are connected. All roots are connected, but not + // all connected slots are (or will become) roots. + // + // Based on the criteria stated in 2), S is connected iff it has a series + // of ancestors (that are each connected) that form a chain back to + // some root slot. + // + // A ledger that is updating with a cluster will have either begun at + // genesis or at at some snapshot slot. + // - Genesis is obviously a special case, and slot 0's parent is deemed + // to be connected in order to kick off the induction + // - Snapshots are taken at rooted slots, and as such, the snapshot slot + // should be marked as connected so that a connected chain can start + // + // CONNECTED is explicitly the first bit to ensure backwards compatibility + // with the boolean field that ConnectedFlags replaced in SlotMeta. + const CONNECTED = 0b0000_0001; + // PARENT_CONNECTED IS INTENTIIONALLY UNUSED FOR NOW + const PARENT_CONNECTED = 0b1000_0000; + } +} + +impl Default for ConnectedFlags { + fn default() -> Self { + ConnectedFlags::empty() + } +} + #[derive(Clone, Debug, Default, Deserialize, Serialize, Eq, PartialEq)] // The Meta column family pub struct SlotMeta { @@ -37,9 +75,8 @@ pub struct SlotMeta { // The list of slots, each of which contains a block that derives // from this one. pub next_slots: Vec, - // True if this slot is full (consumed == last_index + 1) and if every - // slot that is a parent of this slot is also connected. - pub is_connected: bool, + // Connected status flags of this slot + pub connected_flags: ConnectedFlags, // Shreds indices which are marked data complete. pub completed_data_indexes: BTreeSet, } @@ -214,6 +251,14 @@ impl SlotMeta { Some(self.consumed) == self.last_index.map(|ix| ix + 1) } + pub fn is_connected(&self) -> bool { + self.connected_flags.contains(ConnectedFlags::CONNECTED) + } + + pub fn set_connected(&mut self) { + self.connected_flags.set(ConnectedFlags::CONNECTED, true); + } + /// Dangerous. Currently only needed for a local-cluster test pub fn unset_parent(&mut self) { self.parent_slot = None; @@ -225,10 +270,17 @@ impl SlotMeta { } pub(crate) fn new(slot: Slot, parent_slot: Option) -> Self { + let connected_flags = if slot == 0 { + // Slot 0 is the start, mark it as having its' parent connected + // such that slot 0 becoming full will be updated as connected + ConnectedFlags::CONNECTED + } else { + ConnectedFlags::default() + }; SlotMeta { slot, parent_slot, - is_connected: slot == 0, + connected_flags, ..SlotMeta::default() } } @@ -426,6 +478,79 @@ mod test { } } + #[test] + fn test_connected_flags_compatibility() { + // Define a couple structs with bool and ConnectedFlags to illustrate + // that that ConnectedFlags can be deserialized into a bool if the + // PARENT_CONNECTED bit is NOT set + #[derive(Debug, Deserialize, PartialEq, Serialize)] + struct WithBool { + slot: Slot, + connected: bool, + } + #[derive(Debug, Deserialize, PartialEq, Serialize)] + struct WithFlags { + slot: Slot, + connected: ConnectedFlags, + } + + let slot = 3; + let mut with_bool = WithBool { + slot, + connected: false, + }; + let mut with_flags = WithFlags { + slot, + connected: ConnectedFlags::default(), + }; + + // Confirm that serialized byte arrays are same length + assert_eq!( + bincode::serialized_size(&with_bool).unwrap(), + bincode::serialized_size(&with_flags).unwrap() + ); + + // Confirm that connected=false equivalent to ConnectedFlags::default() + assert_eq!( + bincode::serialize(&with_bool).unwrap(), + bincode::serialize(&with_flags).unwrap() + ); + + // Set connected in WithBool and confirm inequality + with_bool.connected = true; + assert_ne!( + bincode::serialize(&with_bool).unwrap(), + bincode::serialize(&with_flags).unwrap() + ); + + // Set connected in WithFlags and confirm equality regained + with_flags.connected.set(ConnectedFlags::CONNECTED, true); + assert_eq!( + bincode::serialize(&with_bool).unwrap(), + bincode::serialize(&with_flags).unwrap() + ); + + // Dserializing WithBool into WithFlags succeeds + assert_eq!( + with_flags, + bincode::deserialize::(&bincode::serialize(&with_bool).unwrap()).unwrap() + ); + + // Deserializing WithFlags into WithBool succeeds + assert_eq!( + with_bool, + bincode::deserialize::(&bincode::serialize(&with_flags).unwrap()).unwrap() + ); + + // Deserializing WithFlags with extra bit set into WithBool fails + with_flags + .connected + .set(ConnectedFlags::PARENT_CONNECTED, true); + assert!( + bincode::deserialize::(&bincode::serialize(&with_flags).unwrap()).is_err() + ); + } + #[test] fn test_clear_unconfirmed_slot() { let mut slot_meta = SlotMeta::new_orphan(5);