diff --git a/crates/core/src/checkpoint.rs b/crates/core/src/checkpoint.rs index 23d5731f9..9bfdf2eed 100644 --- a/crates/core/src/checkpoint.rs +++ b/crates/core/src/checkpoint.rs @@ -21,6 +21,34 @@ struct CPInner { prev: Option>, } +/// When a `CPInner` is dropped we need to go back down the chain and manually remove any +/// no-longer referenced checkpoints. Letting the default rust dropping mechanism handle this +/// leads to recursive logic and stack overflows +/// +/// https://github.com/bitcoindevkit/bdk/issues/1634 +impl Drop for CPInner { + fn drop(&mut self) { + // Take out `prev` so its `drop` won't be called when this drop is finished + let mut current = self.prev.take(); + while let Some(arc_node) = current { + // Get rid of the Arc around `prev` if we're the only one holding a ref + // So the `drop` on it won't be called when the `Arc` is dropped. + // + // FIXME: When MSRV > 1.70.0 this should use Arc::into_inner which actually guarantees + // that no recursive drop calls can happen even with multiple threads. + match Arc::try_unwrap(arc_node).ok() { + Some(mut node) => { + // Keep going backwards + current = node.prev.take(); + // Don't call `drop` on `CPInner` since that risks it becoming recursive. + core::mem::forget(node); + } + None => break, + } + } + } +} + impl PartialEq for CheckPoint { fn eq(&self, other: &Self) -> bool { let self_cps = self.iter().map(|cp| cp.block_id()); diff --git a/crates/core/tests/test_checkpoint.rs b/crates/core/tests/test_checkpoint.rs index 2ec96c153..a5194e5b9 100644 --- a/crates/core/tests/test_checkpoint.rs +++ b/crates/core/tests/test_checkpoint.rs @@ -1,5 +1,5 @@ -use bdk_core::CheckPoint; -use bdk_testenv::block_id; +use bdk_core::{BlockId, CheckPoint}; +use bdk_testenv::{block_id, hash}; /// Inserting a block that already exists in the checkpoint chain must always succeed. #[test] @@ -32,3 +32,22 @@ fn checkpoint_insert_existing() { } } } + +#[test] +fn checkpoint_destruction_is_sound() { + // Prior to the fix in https://github.com/bitcoindevkit/bdk/pull/1731 + // this could have caused a stack overflow due to drop recursion in Arc. + // We test that a long linked list can clean itself up without blowing + // out the stack. + let mut cp = CheckPoint::new(BlockId { + height: 0, + hash: hash!("g"), + }); + let end = 10_000; + for height in 1u32..end { + let hash = bitcoin::hashes::Hash::hash(height.to_be_bytes().as_slice()); + let block = BlockId { height, hash }; + cp = cp.push(block).unwrap(); + } + assert_eq!(cp.iter().count() as u32, end); +}