Add special handling for snapshot root slot in get_confirmed_block (bp #7430) (#7434)

automerge

Cargo.lock

@@ -3571,6 +3571,7 @@ dependencies = [
 name = "solana-ledger"
 version = "0.21.3"
 dependencies = [
+ "assert_matches 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "bincode 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "byteorder 1.3.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "bzip2 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)",

client/src/rpc_request.rs

@@ -31,7 +31,7 @@ pub struct RpcConfirmedBlock {
     pub transactions: Vec<(Transaction, Option<RpcTransactionStatus>)>,
 }
 
-#[derive(Debug, PartialEq, Serialize, Deserialize)]
+#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
 pub struct RpcTransactionStatus {
     pub status: Result<()>,
     pub fee: u64,

ledger/Cargo.toml

@@ -54,6 +54,7 @@ default-features = false
 features = ["lz4"]
 
 [dev-dependencies]
+assert_matches = "1.3.0"
 matches = "0.1.6"
 solana-budget-program = { path = "../programs/budget", version = "0.21.3" }
 

ledger/src/blocktree.rs

@@ -1183,25 +1183,31 @@ impl Blocktree {
                 .expect("Rooted slot must exist in SlotMeta");
 
             let slot_entries = self.get_slot_entries(slot, 0, None)?;
+            if !slot_entries.is_empty() {
                 let slot_transaction_iterator = slot_entries
                     .iter()
                     .cloned()
                     .flat_map(|entry| entry.transactions);
                 let parent_slot_entries = self.get_slot_entries(slot_meta.parent_slot, 0, None)?;
+                let previous_blockhash = if !parent_slot_entries.is_empty() {
+                    get_last_hash(parent_slot_entries.iter()).unwrap()
+                } else {
+                    Hash::default()
+                };
 
                 let block = RpcConfirmedBlock {
-                previous_blockhash: get_last_hash(parent_slot_entries.iter())
+                    previous_blockhash,
-                    .expect("Rooted parent slot must have blockhash"),
                     blockhash: get_last_hash(slot_entries.iter())
-                    .expect("Rooted slot must have blockhash"),
+                        .unwrap_or_else(|| panic!("Rooted slot {:?} must have blockhash", slot)),
                     parent_slot: slot_meta.parent_slot,
-                transactions: self.map_transactions_to_statuses(slot, slot_transaction_iterator),
+                    transactions: self
+                        .map_transactions_to_statuses(slot, slot_transaction_iterator),
                 };
-            Ok(block)
+                return Ok(block);
-        } else {
-            Err(BlocktreeError::SlotNotRooted)
             }
         }
+        Err(BlocktreeError::SlotNotRooted)
+    }
 
     fn map_transactions_to_statuses<'a>(
         &self,
@@ -2118,11 +2124,13 @@ pub mod tests {
         leader_schedule::{FixedSchedule, LeaderSchedule},
         shred::{max_ticks_per_n_shreds, DataShredHeader},
     };
+    use assert_matches::assert_matches;
+    use bincode::serialize;
     use itertools::Itertools;
     use rand::{seq::SliceRandom, thread_rng};
     use solana_runtime::bank::Bank;
     use solana_sdk::{
-        hash::{self, Hash},
+        hash::{self, hash, Hash},
         instruction::CompiledInstruction,
         packet::PACKET_DATA_SIZE,
         pubkey::Pubkey,
@@ -2134,7 +2142,7 @@ pub mod tests {
     // used for tests only
     fn make_slot_entries_with_transactions(num_entries: u64) -> Vec<Entry> {
         let mut entries: Vec<Entry> = Vec::new();
-        for _ in 0..num_entries {
+        for x in 0..num_entries {
             let transaction = Transaction::new_with_compiled_instructions(
                 &[&Keypair::new()],
                 &[Pubkey::new_rand()],
@@ -2143,7 +2151,7 @@ pub mod tests {
                 vec![CompiledInstruction::new(1, &(), vec![0])],
             );
             entries.push(next_entry_mut(&mut Hash::default(), 0, vec![transaction]));
-            let mut tick = create_ticks(1, 0, Hash::default());
+            let mut tick = create_ticks(1, 0, hash(&serialize(&x).unwrap()));
             entries.append(&mut tick);
         }
         entries
@@ -4189,13 +4197,22 @@ pub mod tests {
 
     #[test]
     fn test_get_confirmed_block() {
-        let slot = 0;
+        let slot = 10;
         let entries = make_slot_entries_with_transactions(100);
-        let shreds = entries_to_test_shreds(entries.clone(), slot, 0, true, 0);
+        let blockhash = get_last_hash(entries.iter()).unwrap();
+        let shreds = entries_to_test_shreds(entries.clone(), slot, slot - 1, true, 0);
+        let more_shreds = entries_to_test_shreds(entries.clone(), slot + 1, slot, true, 0);
         let ledger_path = get_tmp_ledger_path!();
         let ledger = Blocktree::open(&ledger_path).unwrap();
         ledger.insert_shreds(shreds, None, false).unwrap();
-        ledger.set_roots(&[0]).unwrap();
+        ledger.insert_shreds(more_shreds, None, false).unwrap();
+        ledger.set_roots(&[slot - 1, slot, slot + 1]).unwrap();
+
+        let mut parent_meta = SlotMeta::default();
+        parent_meta.parent_slot = std::u64::MAX;
+        ledger
+            .put_meta_bytes(slot - 1, &serialize(&parent_meta).unwrap())
+            .unwrap();
 
         let expected_transactions: Vec<(Transaction, Option<RpcTransactionStatus>)> = entries
             .iter()
@@ -4214,6 +4231,16 @@ pub mod tests {
                         },
                     )
                     .unwrap();
+                ledger
+                    .transaction_status_cf
+                    .put(
+                        (slot + 1, signature),
+                        &RpcTransactionStatus {
+                            status: Ok(()),
+                            fee: 42,
+                        },
+                    )
+                    .unwrap();
                 (
                     transaction,
                     Some(RpcTransactionStatus {
@@ -4224,17 +4251,33 @@ pub mod tests {
             })
             .collect();
 
-        let confirmed_block = ledger.get_confirmed_block(0).unwrap();
+        // Even if marked as root, a slot that is empty of entries should return an error
+        let confirmed_block_err = ledger.get_confirmed_block(slot - 1).unwrap_err();
+        assert_matches!(confirmed_block_err, BlocktreeError::SlotNotRooted);
+
+        let confirmed_block = ledger.get_confirmed_block(slot).unwrap();
+        assert_eq!(confirmed_block.transactions.len(), 100);
+
+        let mut expected_block = RpcConfirmedBlock::default();
+        expected_block.transactions = expected_transactions.clone();
+        expected_block.parent_slot = slot - 1;
+        expected_block.blockhash = blockhash;
+        // The previous_blockhash of `expected_block` is default because its parent slot is a
+        // root, but empty of entries. This is special handling for snapshot root slots.
+        assert_eq!(confirmed_block, expected_block);
+
+        let confirmed_block = ledger.get_confirmed_block(slot + 1).unwrap();
         assert_eq!(confirmed_block.transactions.len(), 100);
 
         let mut expected_block = RpcConfirmedBlock::default();
         expected_block.transactions = expected_transactions;
-        // The blockhash and previous_blockhash of `expected_block` are default only because
+        expected_block.parent_slot = slot;
-        // `make_slot_entries_with_transactions` sets all entry hashes to default
+        expected_block.previous_blockhash = blockhash;
+        expected_block.blockhash = blockhash;
         assert_eq!(confirmed_block, expected_block);
 
-        let not_root = ledger.get_confirmed_block(1);
+        let not_root = ledger.get_confirmed_block(slot + 2).unwrap_err();
-        assert!(not_root.is_err());
+        assert_matches!(not_root, BlocktreeError::SlotNotRooted);
 
         drop(ledger);
         Blocktree::destroy(&ledger_path).expect("Expected successful database destruction");