From c11b4714b5acaf19e53b5d091a0d668542053bec Mon Sep 17 00:00:00 2001
From: Andrew Noyes <andrew@weaselab.dev>
Date: Mon, 26 Aug 2024 12:43:07 -0700
Subject: [PATCH] Check more preconditions in Debug mode

---
 ConflictSet.cpp | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/ConflictSet.cpp b/ConflictSet.cpp
index 986b222..d9d4408 100644
--- a/ConflictSet.cpp
+++ b/ConflictSet.cpp
@@ -2912,9 +2912,8 @@ checkMaxBetweenExclusiveImpl<true>(Node *n, int begin, int end,
 // of the result will have `maxVersion` set to `writeVersion` as a
 // postcondition. Nodes along the search path may be invalidated. Callers must
 // ensure that the max version of the self argument is updated.
-[[nodiscard]]
-Node **insert(Node **self, std::span<const uint8_t> key,
-              InternalVersionT writeVersion, WriteContext *tls) {
+[[nodiscard]] Node **insert(Node **self, std::span<const uint8_t> key,
+                            InternalVersionT writeVersion, WriteContext *tls) {
 
   for (; key.size() != 0; ++tls->accum.insert_iterations) {
     self = &getOrCreateChild(*self, key, writeVersion, tls);
@@ -3149,6 +3148,7 @@ struct __attribute__((visibility("hidden"))) ConflictSet::Impl {
     tls.impl = this;
     int64_t check_byte_accum = 0;
     for (int i = 0; i < count; ++i) {
+      assert(reads[i].readVersion >= 0);
       assert(reads[i].readVersion <= newestVersionFullPrecision);
       const auto &r = reads[i];
       check_byte_accum += r.begin.len + r.end.len;
@@ -3206,7 +3206,10 @@ struct __attribute__((visibility("hidden"))) ConflictSet::Impl {
 
       newestVersionFullPrecision = writeVersion;
       newest_version.set(newestVersionFullPrecision);
-      setOldestVersion(newestVersionFullPrecision - kNominalVersionWindow);
+      if (newestVersionFullPrecision - kNominalVersionWindow >
+          oldestVersionFullPrecision) {
+        setOldestVersion(newestVersionFullPrecision - kNominalVersionWindow);
+      }
       while (oldestExtantVersion <
              newestVersionFullPrecision - kMaxCorrectVersionWindow) {
         gcScanStep(1000);
@@ -3214,7 +3217,10 @@ struct __attribute__((visibility("hidden"))) ConflictSet::Impl {
     } else {
       newestVersionFullPrecision = writeVersion;
       newest_version.set(newestVersionFullPrecision);
-      setOldestVersion(newestVersionFullPrecision - kNominalVersionWindow);
+      if (newestVersionFullPrecision - kNominalVersionWindow >
+          oldestVersionFullPrecision) {
+        setOldestVersion(newestVersionFullPrecision - kNominalVersionWindow);
+      }
     }
 
     for (int i = 0; i < count; ++i) {
@@ -3299,13 +3305,18 @@ struct __attribute__((visibility("hidden"))) ConflictSet::Impl {
     return fuel;
   }
 
-  void setOldestVersion(int64_t o) {
-    assert(o <= newestVersionFullPrecision);
-    if (o <= oldestVersionFullPrecision) {
+  void setOldestVersion(int64_t newOldestVersion) {
+    assert(newOldestVersion >= 0);
+    assert(newOldestVersion <= newestVersionFullPrecision);
+    // If addWrites advances oldestVersion to keep within valid window, a
+    // subsequent setOldestVersion can be legitimately called with a version
+    // older than `oldestVersionFullPrecision`. < instead of <= so that we can
+    // do garbage collection work without advancing the oldest version.
+    if (newOldestVersion < oldestVersionFullPrecision) {
       return;
     }
-    InternalVersionT oldestVersion{o};
-    this->oldestVersionFullPrecision = o;
+    InternalVersionT oldestVersion{newOldestVersion};
+    this->oldestVersionFullPrecision = newOldestVersion;
     this->oldestVersion = oldestVersion;
 #if !USE_64_BIT
     InternalVersionT::zero = tls.zero = oldestVersion;
@@ -3366,6 +3377,7 @@ struct __attribute__((visibility("hidden"))) ConflictSet::Impl {
   }
 
   explicit Impl(int64_t oldestVersion) {
+    assert(oldestVersion >= 0);
     init(oldestVersion);
     initMetrics();
   }