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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

refactor!: Box the changeset of StoreErrorWithDump #1998

wants to merge 1 commit into from

Conversation

110CodingP
Copy link

@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

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
refactor!: Box changeset in StoreErrorWithDump
56f1fad 
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.
@110CodingP
Copy link
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

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.

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

@LagginTimes LagginTimes LagginTimes approved these changes

@nymius nymius nymius approved these changes

Assignees

@110CodingP 110CodingP

Labels
None yet
Projects
BDK Chain
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[file_store] Change StoreErrorWithDump to box the aggregate changeset
5 participants
@110CodingP @ValuedMammal @oleonardolima @LagginTimes @nymius