SkipList.cpp decided TooOld with reads[i].readVersion < newestVersion - 2e9, where 2e9 is a double literal. The subtraction and comparison were therefore done in double, and since a double has a 53 bit mantissa the boundary newestVersion - 2000000000 was off by up to ~512 versions for newestVersion >= 2^53. skip_list returned Commit for reads one version below the window (the case the window exists to prevent) and TooOld for reads exactly at it. ReferenceImpl::check in Internal.h had the same expression, so the TestDriver/corpus oracle agreed with the bug.
Changes:
Move the window to a single integer constant, kNominalVersionWindow = 2000000000, in Internal.h, next to a comment explaining why it must not be a double, and use it in both SkipList.cpp and ReferenceImpl::check.
Drop the duplicate definition from ConflictSet.cpp (it was already constexpr int64_t, so the radix tree was correct; it now shares the same constant).
Add test_too_old_window_precision to test_conflict_set.py, which bisimulates skip_list against the radix tree at and one below the window boundary for newestVersion in {2^53, 2^53+1, 2^53+7, INT64_MAX}. It fails without the fix (the skip list disagrees with the radix tree) and passes with it.
Verification:
The issue's repro now prints TooOld with exit 0 for both shipped implementations.
ctest -R 'script_test|api_test|show_memory|interleaving': 39/39 pass (includes the new test, also under valgrind).
All 2874 conflict_set_blackbox_* and all 2845 conflict_set_fuzz_* corpus tests (radix tree vs. the fixed ReferenceImpl oracle) pass.
driver_skip_list (skip_list vs. ReferenceImpl) passes on a sample of 60 corpus tests.
clang-format, black, and the other pre-commit hooks pass.
Closes #99.
`SkipList.cpp` decided `TooOld` with `reads[i].readVersion < newestVersion - 2e9`, where `2e9` is a `double` literal. The subtraction and comparison were therefore done in `double`, and since a `double` has a 53 bit mantissa the boundary `newestVersion - 2000000000` was off by up to ~512 versions for `newestVersion >= 2^53`. skip_list returned `Commit` for reads one version below the window (the case the window exists to prevent) and `TooOld` for reads exactly at it. `ReferenceImpl::check` in `Internal.h` had the same expression, so the `TestDriver`/corpus oracle agreed with the bug.
Changes:
- Move the window to a single integer constant, `kNominalVersionWindow = 2000000000`, in `Internal.h`, next to a comment explaining why it must not be a `double`, and use it in both `SkipList.cpp` and `ReferenceImpl::check`.
- Drop the duplicate definition from `ConflictSet.cpp` (it was already `constexpr int64_t`, so the radix tree was correct; it now shares the same constant).
- Add `test_too_old_window_precision` to `test_conflict_set.py`, which bisimulates skip_list against the radix tree at and one below the window boundary for `newestVersion` in `{2^53, 2^53+1, 2^53+7, INT64_MAX}`. It fails without the fix (the skip list disagrees with the radix tree) and passes with it.
Verification:
- The issue's repro now prints `TooOld` with exit 0 for both shipped implementations.
- `ctest -R 'script_test|api_test|show_memory|interleaving'`: 39/39 pass (includes the new test, also under valgrind).
- All 2874 `conflict_set_blackbox_*` and all 2845 `conflict_set_fuzz_*` corpus tests (radix tree vs. the fixed `ReferenceImpl` oracle) pass.
- `driver_skip_list` (skip_list vs. `ReferenceImpl`) passes on a sample of 60 corpus tests.
- `clang-format`, `black`, and the other pre-commit hooks pass.
CI / test (arm64, -DCMAKE_BUILD_TYPE=Debug -DMSAN_TOOLCHAIN_PATH=/opt/msan, 22, https://minio.weaselab.dev/public/aarch64/msan-toolchain-22.1.8.tar.zst, debug-arm64, ubuntu-latest-arm64) (pull_request) Successful in 3m54s
CI / test (amd64, -DCMAKE_BUILD_TYPE=Debug -DMSAN_TOOLCHAIN_PATH=/opt/msan, 21, https://minio.weaselab.dev/public/x86_64/msan-toolchain-21.1.8.tar.zst, debug, ubuntu-latest-amd64) (pull_request) Successful in 4m1s
`newestVersion - 2e9` is evaluated in `double`, whose 53 bit mantissa
makes the `newestVersion - 2000000000` boundary off by up to 512 versions
once `newestVersion >= 2^53`. skip_list therefore reported `Commit` for
reads one version below the window, and `TooOld` for reads exactly at it.
`ReferenceImpl` in Internal.h had the same expression, so the differential
tests could not catch it.
Share a single integer `kNominalVersionWindow` via Internal.h, use it in
both places, and drop the duplicate (already integer) definition from
ConflictSet.cpp. Adds a regression test that bisimulates skip_list against
the radix tree above 2^53.
Closes#99
All checks were successful
CI / test (arm64, -DCMAKE_BUILD_TYPE=Debug -DMSAN_TOOLCHAIN_PATH=/opt/msan, 22, https://minio.weaselab.dev/public/aarch64/msan-toolchain-22.1.8.tar.zst, debug-arm64, ubuntu-latest-arm64) (pull_request) Successful in 3m54s
CI / test (amd64, -DCMAKE_BUILD_TYPE=Debug -DMSAN_TOOLCHAIN_PATH=/opt/msan, 21, https://minio.weaselab.dev/public/x86_64/msan-toolchain-21.1.8.tar.zst, debug, ubuntu-latest-amd64) (pull_request) Successful in 4m1s
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #99.
SkipList.cppdecidedTooOldwithreads[i].readVersion < newestVersion - 2e9, where2e9is adoubleliteral. The subtraction and comparison were therefore done indouble, and since adoublehas a 53 bit mantissa the boundarynewestVersion - 2000000000was off by up to ~512 versions fornewestVersion >= 2^53. skip_list returnedCommitfor reads one version below the window (the case the window exists to prevent) andTooOldfor reads exactly at it.ReferenceImpl::checkinInternal.hhad the same expression, so theTestDriver/corpus oracle agreed with the bug.Changes:
kNominalVersionWindow = 2000000000, inInternal.h, next to a comment explaining why it must not be adouble, and use it in bothSkipList.cppandReferenceImpl::check.ConflictSet.cpp(it was alreadyconstexpr int64_t, so the radix tree was correct; it now shares the same constant).test_too_old_window_precisiontotest_conflict_set.py, which bisimulates skip_list against the radix tree at and one below the window boundary fornewestVersionin{2^53, 2^53+1, 2^53+7, INT64_MAX}. It fails without the fix (the skip list disagrees with the radix tree) and passes with it.Verification:
TooOldwith exit 0 for both shipped implementations.ctest -R 'script_test|api_test|show_memory|interleaving': 39/39 pass (includes the new test, also under valgrind).conflict_set_blackbox_*and all 2845conflict_set_fuzz_*corpus tests (radix tree vs. the fixedReferenceImploracle) pass.driver_skip_list(skip_list vs.ReferenceImpl) passes on a sample of 60 corpus tests.clang-format,black, and the other pre-commit hooks pass.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.