8000 croaring-rs upgrade 0.5.2 -> 1.0.1 by yeastplume · Pull Request #3779 · mimblewimble/grin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

croaring-rs upgrade 0.5.2 -> 1.0.1 #3779

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 8 commits into from
Feb 6, 2024
Merged

croaring-rs upgrade 0.5.2 -> 1.0.1 #3779

merged 8 commits into from
Feb 6, 2024

Conversation

yeastplume
Copy link
Member
@yeastplume yeastplume commented Jan 30, 2024 •
edited
Loading

Attempt to upgrade croaring-rs to 1.0.1, from 0.5.2, in an attempt to address odd instruction set issues such as found in mimblewimble/grin-gui#73

Note this has not yet been tested, and this is here as a first pass, mostly to determine whether it does indeed address issues. A windows test was failing on the CI server at pow::lean::test::lean_miner. If this no longer fails after the update, this is a good sign.

pub fn trim(&mut self) {
        // trimming successively
        while self.edges.cardinality() > (7 * (self.params.num_edges >> 8) / 8) as u64 {
            self.count_and_kill();
        }
    }

Issues that need to be looked into:

  • Several Bitmap functions that take a range are required to be u32 as opposed to u64 in croaring 0.5.6. I've just cast types for now, we need to confirm this is okay.
  • croaring 1.0.1 now has several serialization formats, and I can't find a clear statement in the documentation saying which one corresponds to the serialization format in 0.5.2. I'm assuming this is Portable (and seems to work with existing chain data, though I could still be wrong).
  • Tests in store appear to be failing following this update, and we need to trace why:
---- pmmr_reload stdout ----
env logger already initialized: SetLoggerError(())
idx: 1, shift_cache.len(): 0
thread 'pmmr_reload' panicked at store\src\prune_list.rs:156:26:
attempt to subtract with overflow


failures:
    compact_twice
    pmmr_compact_entire_peak
    pmmr_compact_horizon
    pmmr_compact_single_leaves
    pmmr_reload
    pmmr_rewind

8000
@yeastplume yeastplume requested a review from tromp January 30, 2024 12:58
@yeastplume
Copy link
Member Author
  • Subtle change bitmap.remove_range_closed became remove_range and accepts an inclusive range as an argument, took a bit to track down. Caught by store pmmr tests.

@yeastplume yeastplume changed the title [WIP] croaring-rs upgrade croaring-rs upgrade 0.5.2 -> 1.0.1 Feb 6, 2024
@yeastplume
Copy link
Member Author

Also note this reverts the CI testing environment to the windows-2019 image as per compiler bug found in RoaringBitmap/croaring-rs#128

tromp
tromp approved these changes Feb 6, 2024
View reviewed changes
Copy link
Contributor
@tromp tromp left a comment

Choose a reason for hiding this comment

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

Looks ok, but this code needs to be made future proof in the next few years so it can support MMR indices beyond 2^32.

@yeastplume yeastplume merged commit 43b43d9 into mimblewimble:master Feb 6, 2024
bayk added a commit to mwcproject/mwc-node that referenced this pull request Jun 23, 2024
@bayk
grin v5.3 (0136) croaring-rs upgrade 0.5.2 -> 1.0.1 (mimblewimble#3779)
b0e747f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tromp tromp tromp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yeastplume @tromp
0