Skip to content

fix: remove emptied lineages from non_empty_histories in truncate#818

Open
MozirDmitriy wants to merge 4 commits into0xMiden:nextfrom
MozirDmitriy:fix/truncate-non-empty-histories-invariant
Open

fix: remove emptied lineages from non_empty_histories in truncate#818
MozirDmitriy wants to merge 4 commits into0xMiden:nextfrom
MozirDmitriy:fix/truncate-non-empty-histories-invariant

Conversation

@MozirDmitriy
Copy link
Contributor

@MozirDmitriy MozirDmitriy commented Feb 5, 2026

[LargeSmtForest::truncate]

/// Removes all tree versions in the forest that are older than the provided `version`, but
/// always retains the latest tree in each lineage.
pub fn truncate(&mut self, version: VersionId) {
let mut newly_empty = Set::default();
self.non_empty_histories.iter().for_each(|l| {
if let Some(d) = self.lineage_data.get_mut(l)
&& d.truncate(version)
{
newly_empty.insert(*l);
}
});
self.non_empty_histories.extend(newly_empty);
}
}
collected lineages whose histories became empty into a newly_empty set, but then called self.non_empty_histories.extend(newly_empty) instead of removing them. Since these lineages were already in the set, extend was a no-op, and emptied lineages were never cleaned up — breaking the invariant that non_empty_histories only contains lineages with actual deltas.

Replaceed extend with a removal loop so emptied lineages are properly evicted from non_empty_histories after truncation.

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Nice catch, it would be great to have a test that would have caught this bug :)

@MozirDmitriy
Copy link
Contributor Author

MozirDmitriy commented Feb 9, 2026

Nice catch, it would be great to have a test that would have caught this bug :)

Added two tests
First test fails with old behaviour
Second - additional test, partially truncates a lineage (version < latest_version) so history still has entries. Asserts the lineage stays in non_empty_histories.

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

LGTM, this just needs a Changelog entry

@MozirDmitriy
Copy link
Contributor Author

LGTM, this just needs a Changelog entry

Added

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

TYVM!

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