Skip to content
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.

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

fix Serializer to correctly restore() #530

Merged
merged 1 commit into from
Jan 8, 2025
Merged

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Jan 7, 2025

to allow resumption of serialization.

It turns out my original patch wasn't tested quite well enough. This adds a test for the important case of restoring the Serializer to an earlier state and still be valid to support serialize more CLVM structures.

A somewhat unrelated change is to pass in the sentinel node to the Serializer constructor, rather than at every call to add().

@arvidn arvidn force-pushed the fix-incremental-serialization branch from 98a0de3 to e03dde6 Compare January 7, 2025 20:32
Copy link

coveralls-official bot commented Jan 7, 2025

Pull Request Test Coverage Report for Build 12658875055

Details

  • 148 of 148 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 93.803%

Totals Coverage Status
Change from base Build 12657501765: 0.2%
Covered Lines: 6009
Relevant Lines: 6406

💛 - Coveralls

@arvidn arvidn requested a review from richardkiss January 7, 2025 20:57
Copy link
Contributor

@richardkiss richardkiss left a comment

Choose a reason for hiding this comment

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

Near as I can tell, you've modified it so the sentinel node is now a property of the instance rather than being passed in to .add. I don't completely understand all the other changes made or the genesis of the bug, but I expect you have a pretty good handle on this.

Copy link
Contributor

@richardkiss richardkiss left a comment

Choose a reason for hiding this comment

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

Ah, you're now copying the ReadCacheLookup to the undo state. Seems reasonable.

@arvidn
Copy link
Contributor Author

arvidn commented Jan 8, 2025

yeah, exactly. the ReadCacheLookup is recording the current state of the parse stack. so it has to be restored as well

@arvidn arvidn merged commit 11574b2 into main Jan 8, 2025
29 checks passed
@arvidn arvidn deleted the fix-incremental-serialization branch January 8, 2025 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants