Skip to content

Conversation

@oleonardolima
Copy link
Contributor

@oleonardolima oleonardolima commented Oct 2, 2025

based on #1997, depends on #2055

Description

Fix memory leak bug in CheckPoint::drop by using Arc::into_inner.

Add tests (from the old PR) for memory leak + stack overflow when dropping CheckPoint.

Notes to the reviewers

It should be merged after #2055.

Changelog notice

### Fix:
- Fix memory leak bug in CheckPoint::drop by using `Arc::into_inner`.

Checklists

All Submissions:

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@oleonardolima oleonardolima self-assigned this Oct 2, 2025
@oleonardolima oleonardolima added the bug Something isn't working label Oct 2, 2025
@oleonardolima oleonardolima moved this to In Progress in BDK Chain Oct 2, 2025
@oleonardolima oleonardolima added this to the Chain 0.24.0 milestone Oct 2, 2025
@oleonardolima oleonardolima force-pushed the fix/checkpoint-drop-mem-leak branch 2 times, most recently from 548f1fa to a3bbba8 Compare October 8, 2025 00:56
@oleonardolima oleonardolima moved this from In Progress to Needs Review in BDK Chain Oct 8, 2025
@oleonardolima oleonardolima marked this pull request as ready for review October 8, 2025 01:00
Copy link
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a3bbba8

@oleonardolima oleonardolima force-pushed the fix/checkpoint-drop-mem-leak branch from a3bbba8 to 20f6844 Compare October 9, 2025 03:32
@evanlinjin evanlinjin force-pushed the fix/checkpoint-drop-mem-leak branch from 20f6844 to 5516165 Compare October 17, 2025 04:03
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self-ACK 5516165

I rebased on the current state of #2055.

Fix memory leak bug in `CheckPoint::drop` by using `Arc::into_inner`.

Fix `CPInner::drop` logic so that if `CPInner::block` becomes
generic and is of a type that required `drop`, it does not leak memory.

Add tests for memory leak + stack overflow when dropping `CheckPoint`.
@oleonardolima oleonardolima force-pushed the fix/checkpoint-drop-mem-leak branch from 5516165 to 0bd93da Compare October 17, 2025 18:27
@oleonardolima
Copy link
Contributor Author

I've rebased it on top of master, so it should be ready to go.

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 0bd93da

@evanlinjin evanlinjin merged commit 126ebda into bitcoindevkit:master Oct 19, 2025
17 checks passed
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Chain Oct 19, 2025
@oleonardolima oleonardolima deleted the fix/checkpoint-drop-mem-leak branch October 20, 2025 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants