Deleting a block doesn't set the version number to MAX_UINT64, but just increases it by one. This makes sure, that if (in case of a synchronization conflict) the block is reintroduced by another client, this is accepted.

src/blockstore/implementations/versioncounting/KnownBlockVersions.cpp

@@ -71,20 +71,16 @@ bool KnownBlockVersions::checkAndUpdateVersion(uint32_t clientId, const Key &key
 uint64_t KnownBlockVersions::incrementVersion(const Key &key, uint64_t lastVersion) {
     unique_lock<mutex> lock(_mutex);
     uint64_t &found = _knownVersions[{_myClientId, key}]; // If the entry doesn't exist, this creates it with value 0.
-    found = std::max(lastVersion + 1, found + 1);
+    uint64_t newVersion = std::max(lastVersion + 1, found + 1);
+    if (newVersion == std::numeric_limits<uint64_t>::max()) {
+        // It's *very* unlikely we ever run out of version numbers in 64bit...but just to be sure...
+        throw std::runtime_error("Version overflow");
+    }
+    found = newVersion;
     _lastUpdateClientId[key] = _myClientId;
     return found;
 }
 
-void KnownBlockVersions::setVersion(uint32_t clientId, const Key &key, uint64_t version) {
-    uint64_t &found = _knownVersions[{clientId, key}];
-    if (found > version) {
-        throw std::runtime_error("Version can only be set to higher version numbers.");
-    }
-    found = version;
-    _lastUpdateClientId[key] = clientId;
-}
-
 void KnownBlockVersions::_loadStateFile() {
     optional<Data> file = Data::LoadFromFile(_stateFilePath);
     if (file == none) {

src/blockstore/implementations/versioncounting/KnownBlockVersions.h

@@ -26,7 +26,6 @@ namespace blockstore {
             uint64_t incrementVersion(const Key &key, uint64_t lastVersion);
 
             uint64_t getBlockVersion(uint32_t clientId, const Key &key) const;
-            void setVersion(uint32_t clientId, const Key &key, uint64_t version);
 
             uint32_t myClientId() const;
 

src/blockstore/implementations/versioncounting/VersionCountingBlock.h

@@ -41,6 +41,7 @@ public:
   size_t size() const override;
   void resize(size_t newSize) override;
 
+  uint64_t version() const;
   cpputils::unique_ref<Block> releaseBlock();
 
 private:
@@ -59,14 +60,13 @@ private:
 
   // This header is prepended to blocks to allow future versions to have compatibility.
   static constexpr uint16_t FORMAT_VERSION_HEADER = 0;
+  static constexpr uint64_t VERSION_ZERO = 0;
 
   std::mutex _mutex;
 
   DISALLOW_COPY_AND_ASSIGN(VersionCountingBlock);
 
 public:
-    static constexpr uint64_t VERSION_ZERO = 0;
-    static constexpr uint64_t VERSION_DELETED = std::numeric_limits<uint64_t>::max();
     static constexpr unsigned int CLIENTID_HEADER_OFFSET = sizeof(FORMAT_VERSION_HEADER);
     static constexpr unsigned int VERSION_HEADER_OFFSET = sizeof(FORMAT_VERSION_HEADER) + sizeof(uint32_t);
     static constexpr unsigned int HEADER_LENGTH = sizeof(FORMAT_VERSION_HEADER) + sizeof(uint32_t) + sizeof(VERSION_ZERO);
@@ -186,11 +186,6 @@ inline void VersionCountingBlock::resize(size_t newSize) {
 inline void VersionCountingBlock::_storeToBaseBlock() {
   if (_dataChanged) {
     _version = _knownBlockVersions->incrementVersion(key(), _version);
-    if (_version == VERSION_DELETED) {
-      // It's *very* unlikely we ever run out of version numbers in 64bit...but just to be sure...
-      throw std::runtime_error("Version overflow");
-      static_assert(VERSION_DELETED == std::numeric_limits<uint64_t>::max(), "The check above assumes VERSION_DELETE to be an upper bound.");
-    }
     uint32_t myClientId = _knownBlockVersions->myClientId();
     std::memcpy(_dataWithHeader.dataOffset(CLIENTID_HEADER_OFFSET), &myClientId, sizeof(myClientId));
     std::memcpy(_dataWithHeader.dataOffset(VERSION_HEADER_OFFSET), &_version, sizeof(_version));
@@ -212,6 +207,10 @@ inline uint64_t VersionCountingBlock::blockSizeFromPhysicalBlockSize(uint64_t bl
   return blockSize - HEADER_LENGTH;
 }
 
+inline uint64_t VersionCountingBlock::version() const {
+  return _version;
+}
+
 }
 }
 

src/blockstore/implementations/versioncounting/VersionCountingBlockStore.h

@@ -62,9 +62,9 @@ inline void VersionCountingBlockStore::remove(cpputils::unique_ref<Block> block)
   Key key = block->key();
   auto versionCountingBlock = cpputils::dynamic_pointer_move<VersionCountingBlock>(block);
   ASSERT(versionCountingBlock != boost::none, "Block is not an VersionCountingBlock");
+  _knownBlockVersions.incrementVersion(key, (*versionCountingBlock)->version());
   auto baseBlock = (*versionCountingBlock)->releaseBlock();
   _baseBlockStore->remove(std::move(baseBlock));
-  _knownBlockVersions.setVersion(_knownBlockVersions.myClientId(), key, VersionCountingBlock::VERSION_DELETED);
 }
 
 inline uint64_t VersionCountingBlockStore::numBlocks() const {

test/blockstore/implementations/versioncounting/KnownBlockVersionsTest.cpp

@@ -19,6 +19,12 @@ public:
     TempFile stateFile;
     KnownBlockVersions testobj;
 
+    void setVersion(KnownBlockVersions *testobj, uint32_t clientId, const blockstore::Key &key, uint64_t version) {
+        if (!testobj->checkAndUpdateVersion(clientId, key, version)) {
+            throw std::runtime_error("Couldn't increase version");
+        }
+    }
+
     void EXPECT_VERSION_IS(uint64_t version, KnownBlockVersions *testobj, blockstore::Key &key, uint32_t clientId) {
         EXPECT_FALSE(testobj->checkAndUpdateVersion(clientId, key, version-1));
         EXPECT_TRUE(testobj->checkAndUpdateVersion(clientId, key, version+1));
@@ -26,34 +32,34 @@ public:
 };
 
 TEST_F(KnownBlockVersionsTest, setandget) {
-    testobj.setVersion(clientId, key, 5);
+    setVersion(&testobj, clientId, key, 5);
     EXPECT_EQ(5u, testobj.getBlockVersion(clientId, key));
 }
 
 TEST_F(KnownBlockVersionsTest, setandget_isPerClientId) {
-    testobj.setVersion(clientId, key, 5);
+    setVersion(&testobj, clientId, key, 5);
-    testobj.setVersion(clientId2, key, 3);
+    setVersion(&testobj, clientId2, key, 3);
     EXPECT_EQ(5u, testobj.getBlockVersion(clientId, key));
     EXPECT_EQ(3u, testobj.getBlockVersion(clientId2, key));
 }
 
 TEST_F(KnownBlockVersionsTest, setandget_isPerBlock) {
-    testobj.setVersion(clientId, key, 5);
+    setVersion(&testobj, clientId, key, 5);
-    testobj.setVersion(clientId, key2, 3);
+    setVersion(&testobj, clientId, key2, 3);
     EXPECT_EQ(5u, testobj.getBlockVersion(clientId, key));
     EXPECT_EQ(3u, testobj.getBlockVersion(clientId, key2));
 }
 
 TEST_F(KnownBlockVersionsTest, setandget_allowsIncreasing) {
-    testobj.setVersion(clientId, key, 5);
+    setVersion(&testobj, clientId, key, 5);
-    testobj.setVersion(clientId, key, 6);
+    setVersion(&testobj, clientId, key, 6);
     EXPECT_EQ(6u, testobj.getBlockVersion(clientId, key));
 }
 
 TEST_F(KnownBlockVersionsTest, setandget_doesntAllowDecreasing) {
-    testobj.setVersion(clientId, key, 5);
+    setVersion(&testobj, clientId, key, 5);
     EXPECT_ANY_THROW(
-      testobj.setVersion(clientId, key, 4);
+      setVersion(&testobj, clientId, key, 4);
     );
 }
 
@@ -74,28 +80,28 @@ TEST_F(KnownBlockVersionsTest, incrementVersion_newentry_versionnotzero) {
 }
 
 TEST_F(KnownBlockVersionsTest, incrementVersion_oldentry_sameVersion) {
-    testobj.setVersion(testobj.myClientId(), key, 5);
+    setVersion(&testobj, testobj.myClientId(), key, 5);
     auto version = testobj.incrementVersion(key, 5);
     EXPECT_EQ(6u, version);
     EXPECT_EQ(6u, testobj.getBlockVersion(testobj.myClientId(), key));
 }
 
 TEST_F(KnownBlockVersionsTest, incrementVersion_oldentry_lowerVersion1) {
-    testobj.setVersion(testobj.myClientId(), key, 5);
+    setVersion(&testobj, testobj.myClientId(), key, 5);
     auto version = testobj.incrementVersion(key, 4);
     EXPECT_EQ(6u, version);
     EXPECT_EQ(6u, testobj.getBlockVersion(testobj.myClientId(), key));
 }
 
 TEST_F(KnownBlockVersionsTest, incrementVersion_oldentry_lowerVersion2) {
-    testobj.setVersion(testobj.myClientId(), key, 5);
+    setVersion(&testobj, testobj.myClientId(), key, 5);
     auto version = testobj.incrementVersion(key, 3);
     EXPECT_EQ(6u, version);
     EXPECT_EQ(6u, testobj.getBlockVersion(testobj.myClientId(), key));
 }
 
 TEST_F(KnownBlockVersionsTest, incrementVersion_oldentry_higherVersion) {
-    testobj.setVersion(testobj.myClientId(), key, 5);
+    setVersion(&testobj, testobj.myClientId(), key, 5);
     auto version = testobj.incrementVersion(key, 6);
     EXPECT_EQ(7u, version);
     EXPECT_EQ(7u, testobj.getBlockVersion(testobj.myClientId(), key));
@@ -107,46 +113,46 @@ TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_newentry) {
 }
 
 TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_oldentry_sameClientSameVersion) {
-    testobj.setVersion(clientId, key, 5);
+    setVersion(&testobj, clientId, key, 5);
     EXPECT_TRUE(testobj.checkAndUpdateVersion(clientId, key, 5));
     EXPECT_EQ(5u, testobj.getBlockVersion(clientId, key));
 }
 
 TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_oldentry_sameClientLowerVersion) {
-    testobj.setVersion(clientId, key, 5);
+    setVersion(&testobj, clientId, key, 5);
     EXPECT_FALSE(testobj.checkAndUpdateVersion(clientId, key, 4));
     EXPECT_EQ(5u, testobj.getBlockVersion(clientId, key));
 }
 
 TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_oldentry_sameClientNewerVersion) {
-    testobj.setVersion(clientId, key, 5);
+    setVersion(&testobj, clientId, key, 5);
     EXPECT_TRUE(testobj.checkAndUpdateVersion(clientId, key, 6));
     EXPECT_EQ(6u, testobj.getBlockVersion(clientId, key));
 }
 
 TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_oldentry_differentClientSameVersion) {
-    testobj.setVersion(clientId, key, 5);
+    setVersion(&testobj, clientId, key, 5);
     EXPECT_TRUE(testobj.checkAndUpdateVersion(clientId2, key, 5));
     EXPECT_EQ(5u, testobj.getBlockVersion(clientId, key));
     EXPECT_EQ(5u, testobj.getBlockVersion(clientId2, key));
 }
 
 TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_oldentry_differentClientLowerVersion) {
-    testobj.setVersion(clientId, key, 5);
+    setVersion(&testobj, clientId, key, 5);
     EXPECT_TRUE(testobj.checkAndUpdateVersion(clientId2, key, 3));
     EXPECT_EQ(5u, testobj.getBlockVersion(clientId, key));
     EXPECT_EQ(3u, testobj.getBlockVersion(clientId2, key));
 }
 
 TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_oldentry_differentClientHigherVersion) {
-    testobj.setVersion(clientId, key, 5);
+    setVersion(&testobj, clientId, key, 5);
     EXPECT_TRUE(testobj.checkAndUpdateVersion(clientId2, key, 7));
     EXPECT_EQ(5u, testobj.getBlockVersion(clientId, key));
     EXPECT_EQ(7u, testobj.getBlockVersion(clientId2, key));
 }
 
 TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_oldentry_oldClientLowerVersion) {
-    testobj.setVersion(clientId, key, 5);
+    setVersion(&testobj, clientId, key, 5);
     EXPECT_TRUE(testobj.checkAndUpdateVersion(clientId2, key, 7));
     EXPECT_FALSE(testobj.checkAndUpdateVersion(clientId, key, 3));
     EXPECT_EQ(5u, testobj.getBlockVersion(clientId, key));
@@ -154,7 +160,7 @@ TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_oldentry_oldClientLowerVers
 }
 
 TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_oldentry_oldClientSameVersion) {
-    testobj.setVersion(clientId, key, 5);
+    setVersion(&testobj, clientId, key, 5);
     EXPECT_TRUE(testobj.checkAndUpdateVersion(clientId2, key, 7));
     EXPECT_FALSE(testobj.checkAndUpdateVersion(clientId, key, 5)); // Don't allow rollback to old client's newest block, if it was superseded by another client
     EXPECT_EQ(5u, testobj.getBlockVersion(clientId, key));
@@ -162,7 +168,7 @@ TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_oldentry_oldClientSameVersi
 }
 
 TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_oldentry_oldClientHigherVersion) {
-    testobj.setVersion(clientId, key, 5);
+    setVersion(&testobj, clientId, key, 5);
     EXPECT_TRUE(testobj.checkAndUpdateVersion(clientId2, key, 7));
     EXPECT_TRUE(testobj.checkAndUpdateVersion(clientId, key, 6));
     EXPECT_EQ(6u, testobj.getBlockVersion(clientId, key));
@@ -194,7 +200,7 @@ TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_oldentry_oldClientHigherVer
 }
 
 TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_oldentry_oldClientLowerVersion_newClientIsSelf) {
-    testobj.setVersion(clientId, key, 5);
+    setVersion(&testobj, clientId, key, 5);
     testobj.incrementVersion(key, 6);
     EXPECT_FALSE(testobj.checkAndUpdateVersion(clientId, key, 3));
     EXPECT_EQ(5u, testobj.getBlockVersion(clientId, key));
@@ -202,7 +208,7 @@ TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_oldentry_oldClientLowerVers
 }
 
 TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_oldentry_oldClientSameVersion_newClientIsSelf) {
-    testobj.setVersion(clientId, key, 5);
+    setVersion(&testobj, clientId, key, 5);
     testobj.incrementVersion(key, 6);
     EXPECT_FALSE(testobj.checkAndUpdateVersion(clientId, key, 5)); // Don't allow rollback to old client's newest block, if it was superseded by another client
     EXPECT_EQ(5u, testobj.getBlockVersion(clientId, key));
@@ -210,7 +216,7 @@ TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_oldentry_oldClientSameVersi
 }
 
 TEST_F(KnownBlockVersionsTest, checkAndUpdateVersion_oldentry_oldClientHigherVersion_newClientIsSelf) {
-    testobj.setVersion(clientId, key, 5);
+    setVersion(&testobj, clientId, key, 5);
     testobj.incrementVersion(key, 6);
     EXPECT_TRUE(testobj.checkAndUpdateVersion(clientId, key, 6));
     EXPECT_EQ(6u, testobj.getBlockVersion(clientId, key));