From 178f850ec9bc3cecee20ad8580b09d4caea304ea Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Wed, 11 Sep 2024 16:02:39 +0200 Subject: [PATCH 1/2] fix: always aquire numbers lock first --- crates/chain-state/src/in_memory.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/crates/chain-state/src/in_memory.rs b/crates/chain-state/src/in_memory.rs index 25362cfd913b..b9296ecacc46 100644 --- a/crates/chain-state/src/in_memory.rs +++ b/crates/chain-state/src/in_memory.rs @@ -40,6 +40,16 @@ pub(crate) struct InMemoryStateMetrics { /// /// This tracks blocks and their state that haven't been persisted to disk yet but are part of the /// canonical chain that can be traced back to a canonical block on disk. +/// +/// # Locking behavior on state updates +/// +/// All update calls must be atomic, meaning that they must acquire all locks at once, before +/// modifying the state. This is to ensure that the internal state is always consistent. +/// Update functions ensure that the numbers write lock is always acquired first, because lookup by +/// numbers first read the numbers map and then the blocks map. +/// By acquiring the numbers lock first, we ensure that read-only lookups don't deadlock updates. +/// This holds, because only lookup by number functions need to acquire the numbers lock first to +/// get the block hash. #[derive(Debug, Default)] pub(crate) struct InMemoryState { /// All canonical blocks that are not on disk yet. @@ -138,8 +148,9 @@ impl CanonicalInMemoryStateInner { /// Clears all entries in the in memory state. fn clear(&self) { { - let mut blocks = self.in_memory_state.blocks.write(); + // acquire locks, starting with the numbers lock let mut numbers = self.in_memory_state.numbers.write(); + let mut blocks = self.in_memory_state.blocks.write(); blocks.clear(); numbers.clear(); self.in_memory_state.pending.send_modify(|p| { @@ -239,7 +250,7 @@ impl CanonicalInMemoryState { I: IntoIterator, { { - // acquire all locks + // acquire locks, starting with the numbers lock let mut numbers = self.inner.in_memory_state.numbers.write(); let mut blocks = self.inner.in_memory_state.blocks.write(); @@ -301,8 +312,9 @@ impl CanonicalInMemoryState { } { - let mut blocks = self.inner.in_memory_state.blocks.write(); + // acquire locks, starting with the numbers lock let mut numbers = self.inner.in_memory_state.numbers.write(); + let mut blocks = self.inner.in_memory_state.blocks.write(); let BlockNumHash { number: persisted_height, hash: _ } = persisted_num_hash; From 026bfcc0a0b21e3cb2af2018c55281ac85680159 Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Wed, 11 Sep 2024 16:13:49 +0200 Subject: [PATCH 2/2] swap order --- crates/chain-state/src/in_memory.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/chain-state/src/in_memory.rs b/crates/chain-state/src/in_memory.rs index b9296ecacc46..2b1f9e8ce311 100644 --- a/crates/chain-state/src/in_memory.rs +++ b/crates/chain-state/src/in_memory.rs @@ -151,8 +151,8 @@ impl CanonicalInMemoryStateInner { // acquire locks, starting with the numbers lock let mut numbers = self.in_memory_state.numbers.write(); let mut blocks = self.in_memory_state.blocks.write(); - blocks.clear(); numbers.clear(); + blocks.clear(); self.in_memory_state.pending.send_modify(|p| { p.take(); });