diff --git a/accounts-db/src/accounts_db.rs b/accounts-db/src/accounts_db.rs index d528c4a3c6b47d..76b8e3f001c1c8 100644 --- a/accounts-db/src/accounts_db.rs +++ b/accounts-db/src/accounts_db.rs @@ -4247,6 +4247,7 @@ impl AccountsDb { zero_lamport_single_ref_pubkeys: &[&Pubkey], slot: Slot, stats: &ShrinkStats, + do_assert: bool, ) { stats.purged_zero_lamports.fetch_add( zero_lamport_single_ref_pubkeys.len() as u64, @@ -4255,10 +4256,15 @@ impl AccountsDb { // we have to unref before we `purge_keys_exact`. Otherwise, we could race with the foreground with tx processing // reviving this index entry and then we'd unref the revived version, which is a refcount bug. + self.accounts_index.scan( zero_lamport_single_ref_pubkeys.iter().cloned(), |_pubkey, _slots_refs, _entry| AccountsIndexScanResult::Unref, - Some(AccountsIndexScanResult::Unref), + if do_assert { + Some(AccountsIndexScanResult::UnrefAssert0) + } else { + Some(AccountsIndexScanResult::UnrefLog0) + }, false, ScanFilter::All, ); @@ -4286,6 +4292,7 @@ impl AccountsDb { &shrink_collect.zero_lamport_single_ref_pubkeys, shrink_collect.slot, stats, + false, ); // Purge old, overwritten storage entries @@ -11150,6 +11157,35 @@ pub mod tests { assert_eq!(accounts.alive_account_count_in_slot(1), 0); } + #[test] + #[should_panic(expected = "ref count expected to be zero")] + fn test_remove_zero_lamport_multi_ref_accounts_panic() { + let accounts = AccountsDb::new_single_for_tests(); + let pubkey_zero = Pubkey::from([1; 32]); + let one_lamport_account = + AccountSharedData::new(1, 0, AccountSharedData::default().owner()); + + let zero_lamport_account = + AccountSharedData::new(0, 0, AccountSharedData::default().owner()); + let slot = 1; + + accounts.store_for_tests(slot, &[(&pubkey_zero, &one_lamport_account)]); + accounts.calculate_accounts_delta_hash(slot); + accounts.add_root_and_flush_write_cache(slot); + + accounts.store_for_tests(slot + 1, &[(&pubkey_zero, &zero_lamport_account)]); + accounts.calculate_accounts_delta_hash(slot + 1); + accounts.add_root_and_flush_write_cache(slot + 1); + + // This should panic because there are 2 refs for pubkey_zero. + accounts.remove_zero_lamport_single_ref_accounts_after_shrink( + &[&pubkey_zero], + slot, + &ShrinkStats::default(), + true, + ); + } + #[test] fn test_remove_zero_lamport_single_ref_accounts_after_shrink() { for pass in 0..3 { @@ -11195,39 +11231,67 @@ pub mod tests { (false, ()) }); - let zero_lamport_single_ref_pubkeys = [&pubkey_zero]; + let zero_lamport_single_ref_pubkeys = + if pass < 2 { vec![&pubkey_zero] } else { vec![] }; accounts.remove_zero_lamport_single_ref_accounts_after_shrink( &zero_lamport_single_ref_pubkeys, slot, &ShrinkStats::default(), + true, ); accounts.accounts_index.get_and_then(&pubkey_zero, |entry| { - if pass == 0 { - // should not exist in index at all - assert!(entry.is_none(), "{pass}"); - } else { - // alive only in slot + 1 - assert_eq!(entry.unwrap().slot_list.read().unwrap().len(), 1); - assert_eq!( - entry + match pass { + 0 => { + // should not exist in index at all + assert!(entry.is_none(), "{pass}"); + } + 1 => { + // alive only in slot + 1 + assert_eq!(entry.unwrap().slot_list.read().unwrap().len(), 1); + assert_eq!( + entry + .unwrap() + .slot_list + .read() + .unwrap() + .first() + .map(|(s, _)| s) + .cloned() + .unwrap(), + slot + 1 + ); + let expected_ref_count = 0; + assert_eq!( + entry.map(|e| e.ref_count()), + Some(expected_ref_count), + "{pass}" + ); + } + 2 => { + // alive in both slot, slot + 1 + assert_eq!(entry.unwrap().slot_list.read().unwrap().len(), 2); + + let slots = entry .unwrap() .slot_list .read() .unwrap() - .first() + .iter() .map(|(s, _)| s) .cloned() - .unwrap(), - slot + 1 - ); - // refcount = 1 if we flushed the write cache for slot + 1 - let expected_ref_count = if pass < 2 { 0 } else { 1 }; - assert_eq!( - entry.map(|e| e.ref_count()), - Some(expected_ref_count), - "{pass}" - ); + .collect::>(); + assert_eq!(slots, vec![slot, slot + 1]); + let expected_ref_count = 2; + assert_eq!( + entry.map(|e| e.ref_count()), + Some(expected_ref_count), + "{pass}" + ); + } + _ => { + unreachable!("Shouldn't reach here.") + } } (false, ()) }); diff --git a/accounts-db/src/accounts_index.rs b/accounts-db/src/accounts_index.rs index fbd99409a50771..a849d08af385f6 100644 --- a/accounts-db/src/accounts_index.rs +++ b/accounts-db/src/accounts_index.rs @@ -311,14 +311,15 @@ impl AccountMapEntryInner { } /// decrement the ref count - /// return true if the old refcount was already 0. This indicates an under refcounting error in the system. - pub fn unref(&self) -> bool { + /// return the refcount prior to subtracting 1 + /// 0 indicates an under refcounting error in the system. + pub fn unref(&self) -> RefCount { let previous = self.ref_count.fetch_sub(1, Ordering::Release); self.set_dirty(true); if previous == 0 { inc_new_counter_info!("accounts_index-deref_from_0", 1); } - previous == 0 + previous } pub fn dirty(&self) -> bool { @@ -647,6 +648,10 @@ pub enum AccountsIndexScanResult { KeepInMemory, /// reduce refcount by 1 Unref, + /// reduce refcount by 1 and assert that ref_count = 0 after unref + UnrefAssert0, + /// reduce refcount by 1 and log if ref_count != 0 after unref + UnrefLog0, } #[derive(Debug)] @@ -1453,12 +1458,34 @@ impl + Into> AccountsIndex { }; cache = match result { AccountsIndexScanResult::Unref => { - if locked_entry.unref() { + if locked_entry.unref() == 0 { info!("scan: refcount of item already at 0: {pubkey}"); self.unref_zero_count.fetch_add(1, Ordering::Relaxed); } true } + AccountsIndexScanResult::UnrefAssert0 => { + assert_eq!( + locked_entry.unref(), + 1, + "ref count expected to be zero, but is {}! {pubkey}, {:?}", + locked_entry.ref_count(), + locked_entry.slot_list.read().unwrap(), + ); + true + } + AccountsIndexScanResult::UnrefLog0 => { + let old_ref = locked_entry.unref(); + if old_ref != 1 { + info!("Unexpected unref {pubkey} with {old_ref} {:?}, expect old_ref to be 1", locked_entry.slot_list.read().unwrap()); + datapoint_warn!( + "accounts_db-unexpected-unref-zero", + ("old_ref", old_ref, i64), + ("pubkey", pubkey.to_string(), String), + ); + } + true + } AccountsIndexScanResult::KeepInMemory => true, AccountsIndexScanResult::OnlyKeepInMemoryIfDirty => false, }; @@ -4065,9 +4092,9 @@ pub mod tests { assert!(map.get_internal_inner(&key, |entry| { // check refcount BEFORE the unref assert_eq!(u64::from(!expected), entry.unwrap().ref_count()); - // first time, ref count was at 1, we can unref once. Unref should return false. - // second time, ref count was at 0, it is an error to unref. Unref should return true - assert_eq!(expected, entry.unwrap().unref()); + // first time, ref count was at 1, we can unref once. Unref should return 1. + // second time, ref count was at 0, it is an error to unref. Unref should return 0 + assert_eq!(u64::from(!expected), entry.unwrap().unref()); // check refcount AFTER the unref assert_eq!( if expected {