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

Refactor btree iterator last index #125

Merged
merged 3 commits into from
Sep 26, 2022

Conversation

cheme
Copy link
Collaborator

@cheme cheme commented Sep 22, 2022

After working on #124.
I did realize many variants of LastIndex were useless.
This PR changes:
LastIndex::Start -> LastIndex::Before(0)
LastIndex::End -> LastIndex::Before(last separator + 1) . (so can be equal to ORDER).
LastIndex::After(i) -> LastIndex::Before(i + 1)

Also a seek operation do not need a direction.
And seek_to_last is now using a variant of SeekTo enum.

Close #124

@cheme cheme requested a review from Tpt September 22, 2022 09:41
Copy link
Contributor

@Tpt Tpt left a comment

Choose a reason for hiding this comment

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

Thank you!

@cheme cheme requested a review from arkpar September 22, 2022 10:16
Copy link
Contributor

@Tpt Tpt left a comment

Choose a reason for hiding this comment

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

Sadly, I believe I there is still an issue. If we have a value >= ORDER in the LastIndex::At value, the following line will fail because there are only ORDER separators:

if let Some(address) = state.1.separator_address(at) {

(I discovered this problem while fuzzing this change)

@cheme
Copy link
Collaborator Author

cheme commented Sep 22, 2022

Thank you, I also see other issue with seek, I will fix this.

@cheme cheme changed the title Remove incorrect btree iterator assertion Refactor btree iterator last index Sep 22, 2022
@cheme
Copy link
Collaborator Author

cheme commented Sep 22, 2022

I ended up refactoring (see updated description) a bit but should be a good thing.

Copy link
Contributor

@Tpt Tpt left a comment

Choose a reason for hiding this comment

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

Thank you for the update!

I have fuzzed it during 1/2h and it seems to work properly now.

@arkpar arkpar merged commit ea53652 into paritytech:master Sep 26, 2022
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.

Debug assertion failure during btree forward iteration
3 participants