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

croaring-rs upgrade 0.5.2 -> 1.0.1 #3779

Merged
merged 8 commits into from
Feb 6, 2024

Conversation

yeastplume
Copy link
Member

@yeastplume yeastplume commented Jan 30, 2024

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

@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

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
12 checks passed
bayk added a commit to mwcproject/mwc-node that referenced this pull request Jun 23, 2024
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