Skip to content

refactor!: Box the changeset of StoreErrorWithDump #1998

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 7, 2025
Merged

refactor!: Box the changeset of StoreErrorWithDump #1998

merged 1 commit into from
Aug 7, 2025

Conversation

@110CodingP
Copy link
Contributor

@110CodingP 110CodingP commented Jul 22, 2025 •
edited
Loading

Description

Fixes #1991. As the linked issue mentions Boxing helps reduce memory overhead and limits the size of enums with StoreErrorWithDump as variant.

Breaking: The enum StoreErrorWithDump and the functions load,dump and load_or_create have been changed.

Changelog Notice

   Changed
   - `changeset` field of `StoreErrorWithDump` is now of type `Option<Box<C>>`

Checklists

All Submissions:

@notmandatory notmandatory moved this to In Progress in BDK Chain Jul 22, 2025
@ValuedMammal
Copy link
Collaborator

ValuedMammal commented Jul 22, 2025

I agree with the use of Box in the StoreErrorWithDump error, but I wonder is it necessary to box the changeset when returned in the Ok value, for example in dump and load, etc. If no one finds this to be important, I would just as soon leave it out.

@ValuedMammal ValuedMammal requested a review from nymius July 22, 2025 19:53
@110CodingP
Copy link
Contributor Author

110CodingP commented Jul 28, 2025 •
edited
Loading

Removed the Box from return type of dump, load, load_or_create in #56f1fad .
Sorry for the delay.

nymius
nymius approved these changes Jul 28, 2025
View reviewed changes
Copy link
Contributor

@nymius nymius left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me!

I wonder is it necessary to box the changeset when returned in the Ok value, for example in dump and load, etc.

We can ignore that, thinking in terms of the goal of this crate, which is to provide a testing database to experiment, not performance. load and dump are called most of the time just on the startup and on the shutdown of the code using the database, with occasional dumps for debugging.

@ValuedMammal
Copy link
Collaborator

ValuedMammal commented Jul 28, 2025

ACK 56f1fad

LagginTimes
LagginTimes approved these changes Jul 29, 2025
View reviewed changes
Copy link
Contributor

@LagginTimes LagginTimes left a comment

Choose a reason for hiding this comment

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

ACK 56f1fad

oleonardolima
oleonardolima reviewed Aug 4, 2025
View reviewed changes
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

ACK 56f1fad

You need to do a rebase to get the updated rustc and MSRV fixed CI.

@110CodingP
refactor!: Box changeset in StoreErrorWithDump
418753f 
As mentioned in the corresponding issue, `Box`ing helps reduce memory
overhead while passing the enum around and also to limit the size of
enums with `StoreErrorWithDump` as a variant.

Breaking: The enum `StoreErrorWithDump` has been changed.
@ValuedMammal ValuedMammal added this to the Wallet 3.0.0 milestone Aug 6, 2025
@ValuedMammal ValuedMammal moved this to Needs Review in BDK Wallet Aug 6, 2025
evanlinjin
evanlinjin approved these changes Aug 7, 2025
View reviewed changes
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 418753f

@evanlinjin evanlinjin merged commit c43154d into bitcoindevkit:master Aug 7, 2025
31 of 34 checks passed
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Wallet Aug 7, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in BDK Chain Aug 7, 2025
@oleonardolima oleonardolima mentioned this pull request Sep 12, 2025
Closed
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oleonardolima oleonardolima oleonardolima left review comments

@evanlinjin evanlinjin evanlinjin approved these changes

@LagginTimes LagginTimes LagginTimes approved these changes

+1 more reviewer

@nymius nymius nymius approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@110CodingP 110CodingP

Labels

None yet

Projects

BDK Chain
Status: Done
BDK Wallet
Archived in project

Milestone

Wallet 3.0.0

Development

Successfully merging this pull request may close these issues.

[file_store] Change StoreErrorWithDump to box the aggregate changeset

6 participants

@110CodingP @ValuedMammal @evanlinjin @oleonardolima @LagginTimes @nymius