normalization tests pass

src/claimtrie.cpp

@@ -181,13 +181,16 @@ bool CClaimTrieCacheBase::deleteNodeIfPossible(const std::string& name, std::str
     if (name.empty()) return false;
     // to remove a node it must have one or less children and no claims
     vector_builder<std::string, std::string> claimsBuilder;
-    db << "SELECT name FROM claims WHERE name = ? AND validHeight < ? AND expirationHeight >= ? "
+    db << "SELECT name FROM claims WHERE nodeName = ? AND validHeight < ? AND expirationHeight >= ? "
               << name << nNextHeight << nNextHeight >> claimsBuilder;
     claims = std::move(claimsBuilder);
     if (!claims.empty()) return false; // still has claims
     // we now know it has no claims, but we need to check its children
     int64_t count;
     std::string childName;
+    // this line assumes that we've set the parent on child nodes already,
+    // which means we are len(name) desc in our parent method
+    // alternately: SELECT COUNT(DISTINCT nodeName) FROM claims WHERE SUBSTR(nodeName, 1, len(?)) == ? AND LENGTH(nodeName) > len(?)
     db << "SELECT COUNT(*),MAX(name) FROM nodes WHERE parent = ?" << name >> std::tie(count, childName);
     if (count > 1) return false; // still has multiple children
     // okay. it's going away
@@ -208,21 +211,23 @@ bool CClaimTrieCacheBase::deleteNodeIfPossible(const std::string& name, std::str
 void CClaimTrieCacheBase::ensureTreeStructureIsUpToDate() {
     if (!transacting) return;
 
-    // your children are your nodes that match your key, go at least one longer,
+    // your children are your nodes that match your key but go at least one longer,
-    // and have nothing in common with the other nodes in that set -- a hard query w/o parent
+    // and have no trailing prefix in common with the other nodes in that set -- a hard query w/o parent field
+
+    // when we get into this method, we have some claims that have been added, removed, and updated
+    // those each have a corresponding node in the list with a null hash
+    // some of our nodes will go away, some new ones will be added, some will be reparented
+
 
     // the plan: update all the claim hashes first
     vector_builder<std::string, std::string> names;
     db << "SELECT name FROM nodes WHERE hash IS NULL ORDER BY LENGTH(name) DESC, name DESC" >> names;
-
+    if (names.empty()) return; // nothing to do
-    if (names.empty()) return; // could track this with a dirty flag to avoid the above query (but it would be some maintenance)
 
     // there's an assumption that all nodes with claims are here; we do that as claims are inserted
-    // should we do the same to remove nodes? no; we need their last takeover height if they come back
-    //float time = 0;
 
     // assume parents are not set correctly here:
-    auto parentQuery = db << "SELECT name FROM nodes WHERE parent IS NOT NULL AND "
+    auto parentQuery = db << "SELECT name FROM nodes WHERE "
                               "name IN (WITH RECURSIVE prefix(p) AS (VALUES(?) UNION ALL "
                               "SELECT SUBSTR(p, 1, LENGTH(p)-1) FROM prefix WHERE p != '') SELECT p FROM prefix) "
                               "ORDER BY LENGTH(name) DESC LIMIT 1";
@@ -235,25 +240,17 @@ void CClaimTrieCacheBase::ensureTreeStructureIsUpToDate() {
             deleteNodeIfPossible(parent, grandparent, claims);
             continue;
         }
-        if (claims.empty())
+        if (name.empty() || claims.empty())
-            continue;
+            continue; // if you have no claims but we couldn't delete you, you must have legitimate children
 
-        // pretend that we hash them all together:
+        parentQuery << name.substr(0, name.size() - 1);
-        std::string nameOrParent;
-        parentQuery << name;
         auto queryIt = parentQuery.begin();
         if (queryIt != parentQuery.end())
-            *queryIt >> nameOrParent;
+            *queryIt >> parent;
+        else
+            parent.clear();
         parentQuery++; // reusing knocks off about 10% of the query time
 
-        // if it already exists:
-        if (nameOrParent == name) { // TODO: we could actually set the hash here
-            db << "UPDATE nodes SET hash = NULL WHERE name = ?" << name;
-            continue;
-        }
-
-        parent = std::move(nameOrParent);
-
         // we know now that we need to insert it,
         // but we may need to insert a parent node for it first (also called a split)
         vector_builder<std::string, std::string> siblings;
@@ -951,24 +948,26 @@ bool CClaimTrieCacheBase::incrementBlock(insertUndoType& insertUndo, claimUndoTy
         }
 
         // if somebody activates on this block and they are the new best, then everybody activates on this block
-        CClaimValue value;
+        CClaimValue candidateValue;
-        auto hasCurrent = getInfoForName(nameWithTakeover, value, 1);
+        auto hasCandidate = getInfoForName(nameWithTakeover, candidateValue, 1);
         // now that they're all in get the winner:
         int existingHeight;
         std::unique_ptr<uint160> existingID;
         db << "SELECT IFNULL(takeoverHeight, 0), takeoverID FROM nodes WHERE name = ?"
               << nameWithTakeover >> std::tie(existingHeight, existingID);
 
-        auto newOwner = needsActivate || existingID == nullptr || !hasCurrent || *existingID != value.claimId;
+        auto hasBeenSetBefore = existingID != nullptr && !existingID->IsNull();
-        if (newOwner && activateAllFor(insertUndo, insertSupportUndo, nameWithTakeover))
+        auto takeoverHappening = needsActivate || !hasCandidate || (hasBeenSetBefore && *existingID != candidateValue.claimId);
-            getInfoForName(nameWithTakeover, value, 1);
+        if (takeoverHappening && activateAllFor(insertUndo, insertSupportUndo, nameWithTakeover))
+            hasCandidate = getInfoForName(nameWithTakeover, candidateValue, 1);
 
-        if (existingID != nullptr)
+        if (takeoverHappening || !hasBeenSetBefore) {
-            takeoverUndo.emplace_back(nameWithTakeover, std::make_pair(existingHeight, *existingID));
+            takeoverUndo.emplace_back(nameWithTakeover, std::make_pair(existingHeight, hasBeenSetBefore ? *existingID : uint160()));
-
+            if (hasCandidate)
-        if (newOwner) {
                 db << "UPDATE nodes SET takeoverHeight = ?, takeoverID = ? WHERE name = ?"
-               << nNextHeight << value.claimId << nameWithTakeover;
+                      << nNextHeight << candidateValue.claimId << nameWithTakeover;
+            else
+                db << "UPDATE nodes SET takeoverHeight = NULL, takeoverID = NULL WHERE name = ?" << nameWithTakeover;
             assert(db.rows_modified());
         }
     }
@@ -1057,10 +1056,13 @@ bool CClaimTrieCacheBase::finalizeDecrement(takeoverUndoType& takeoverUndo)
     db << "UPDATE nodes SET hash = NULL WHERE name IN "
           "(SELECT nodeName FROM supports WHERE validHeight = ?)" << nNextHeight;
 
-    for (auto it = takeoverUndo.crbegin(); it != takeoverUndo.crend(); ++it)
+    for (auto it = takeoverUndo.crbegin(); it != takeoverUndo.crend(); ++it) {
+        if (it->second.second.IsNull())
+            db << "UPDATE nodes SET takeoverHeight = NULL, takeoverID = NULL, hash = NULL WHERE name = ?" << it->first;
+        else
             db << "UPDATE nodes SET takeoverHeight = ?, takeoverID = ?, hash = NULL WHERE name = ?"
                   << it->second.first << it->second.second << it->first;
-
+    }
     return true;
 }
 

src/claimtrie.h

@@ -465,7 +465,8 @@ protected:
     int getDelayForName(const std::string& name, const uint160& claimId) const override;
 
 private:
-    bool normalizeAllNamesInTrieIfNecessary(bool forward);
+    bool normalizeAllNamesInTrieIfNecessary(takeoverUndoType& takeovers);
+    bool unnormalizeAllNamesInTrieIfNecessary();
 };
 
 class CClaimTrieCacheHashFork : public CClaimTrieCacheNormalizationFork

src/claimtrieforks.cpp

@@ -135,44 +135,76 @@ std::string CClaimTrieCacheNormalizationFork::normalizeClaimName(const std::stri
     return normalized;
 }
 
-bool CClaimTrieCacheNormalizationFork::normalizeAllNamesInTrieIfNecessary(bool forward)
+bool CClaimTrieCacheNormalizationFork::normalizeAllNamesInTrieIfNecessary(takeoverUndoType& takeoverUndo)
 {
-    if (nNextHeight != Params().GetConsensus().nNormalizedNameForkHeight)
-        return false;
-
     if (!transacting) { transacting = true; db << "begin"; }
 
     // run the one-time upgrade of all names that need to change
-    // it modifies the (cache) trie as it goes, so we need to grab everything to be modified first
-
     db.define("NORMALIZED", [this](const std::string& str) { return normalizeClaimName(str, true); });
 
-    auto query = db << "SELECT NORMALIZED(name) as nn, name, claimID FROM claims WHERE nodeName != nn";
+    // make the new nodes
+    db << "INSERT INTO nodes(name) SELECT NORMALIZED(name) AS nn FROM claims WHERE nn != nodeName "
+          "ON CONFLICT(name) DO UPDATE SET hash = NULL";
+    db << "UPDATE nodes SET hash = NULL WHERE name IN "
+          "(SELECT NORMALIZED(name) AS nn FROM supports WHERE nn != nodeName)";
+
+    // update the claims and supports
+    db << "UPDATE claims SET nodeName = NORMALIZED(name)";
+    db << "UPDATE supports SET nodeName = NORMALIZED(name)";
+
+    // remove the old nodes
+    auto query = db << "SELECT name, IFNULL(takeoverHeight, 0), takeoverID FROM nodes "
+                       "WHERE name NOT IN (SELECT nodeName FROM claims)";
     for (auto&& row: query) {
-        std::string newName, oldName;
+        std::string name;
-        uint160 claimID;
+        int takeoverHeight;
-        row >> newName >> oldName >> claimID;
+        std::unique_ptr<uint160> takeoverID;
-        if (!forward) std::swap(newName, oldName);
+        row >> name >> takeoverHeight >> takeoverID;
-        db << "UPDATE claims SET nodeName = ? WHERE claimID = ?" << newName << claimID;
+        if (name.empty()) continue; // preserve our root node
-        db << "DELETE FROM nodes WHERE name = ?" << oldName;
+        takeoverUndo.emplace_back(name, std::make_pair(takeoverHeight, takeoverID ? *takeoverID : uint160()));
-        db << "INSERT INTO nodes(name) VALUES(?) ON CONFLICT(name) DO UPDATE SET hash = NULL" << newName;
+        // we need to let the tree structure method do the actual node delete:
+        db << "UPDATE nodes SET hash = NULL WHERE name = ?" << name;
     }
 
     return true;
 }
 
+bool CClaimTrieCacheNormalizationFork::unnormalizeAllNamesInTrieIfNecessary()
+{
+    if (!transacting) { transacting = true; db << "begin"; }
+
+    // run the one-time upgrade of all names that need to change
+    db.define("NORMALIZED", [this](const std::string& str) { return normalizeClaimName(str, true); });
+
+    db << "INSERT INTO nodes(name) SELECT name FROM claims WHERE name != nodeName "
+          "ON CONFLICT(name) DO UPDATE SET hash = NULL";
+    db << "UPDATE nodes SET hash = NULL WHERE name IN "
+          "(SELECT name FROM supports WHERE name != nodeName UNION "
+          "SELECT nodeName FROM supports WHERE name != nodeName UNION "
+          "SELECT nodeName FROM claims WHERE name != nodeName)";
+
+    db << "UPDATE claims SET nodeName = name";
+    db << "UPDATE supports SET nodeName = name";
+    // we need to let the tree structure method do the actual node delete
+    db << "UPDATE nodes SET hash = NULL WHERE name NOT IN "
+          "(SELECT name FROM claims)";
+
+    return true;
+}
+
 bool CClaimTrieCacheNormalizationFork::incrementBlock(insertUndoType& insertUndo, claimUndoType& expireUndo,
         insertUndoType& insertSupportUndo, supportUndoType& expireSupportUndo, takeoverUndoType& takeoverUndo)
 {
-    normalizeAllNamesInTrieIfNecessary(true);
+    if (nNextHeight == Params().GetConsensus().nNormalizedNameForkHeight)
+        normalizeAllNamesInTrieIfNecessary(takeoverUndo);
     return CClaimTrieCacheExpirationFork::incrementBlock(insertUndo, expireUndo, insertSupportUndo, expireSupportUndo, takeoverUndo);
 }
 
 bool CClaimTrieCacheNormalizationFork::decrementBlock(insertUndoType& insertUndo, claimUndoType& expireUndo, insertUndoType& insertSupportUndo, supportUndoType& expireSupportUndo)
 {
     auto ret = CClaimTrieCacheExpirationFork::decrementBlock(insertUndo, expireUndo, insertSupportUndo, expireSupportUndo);
-    if (ret)
+    if (ret && nNextHeight == Params().GetConsensus().nNormalizedNameForkHeight)
-        normalizeAllNamesInTrieIfNecessary(false);
+        unnormalizeAllNamesInTrieIfNecessary();
     return ret;
 }
 

src/test/claimtrienormalization_tests.cpp

@@ -213,16 +213,15 @@ BOOST_AUTO_TEST_CASE(claimtriecache_normalization)
     CBlockIndex* pindex = chainActive.Tip();
     CBlock block;
     int amelieValidHeight;
+    std::string removed;
     BOOST_CHECK(trieCache.shouldNormalize());
     BOOST_CHECK(ReadBlockFromDisk(block, pindex, Params().GetConsensus()));
     BOOST_CHECK(g_chainstate.DisconnectBlock(block, pindex, coins, trieCache) == DisconnectResult::DISCONNECT_OK);
     BOOST_CHECK(!trieCache.shouldNormalize());
-    BOOST_CHECK(!trieCache.removeClaim(ClaimIdHash(tx2.GetHash(), 0), COutPoint(tx2.GetHash(), 0), name_normd, amelieValidHeight));
+    BOOST_CHECK(trieCache.removeClaim(ClaimIdHash(tx2.GetHash(), 0), COutPoint(tx2.GetHash(), 0), removed, amelieValidHeight));
-    BOOST_CHECK(trieCache.removeClaim(ClaimIdHash(tx2.GetHash(), 0), COutPoint(tx2.GetHash(), 0), name_upper, amelieValidHeight));
 
     BOOST_CHECK(trieCache.getInfoForName(name, nval1));
-    BOOST_CHECK(trieCache.addClaim(name, COutPoint(tx1.GetHash(), 0), ClaimIdHash(tx1.GetHash(), 0), CAmount(2), currentHeight + 1));
+    BOOST_CHECK_EQUAL(nval1.claimId, ClaimIdHash(tx1.GetHash(), 0));
-    BOOST_CHECK(trieCache.getInfoForName(name, nval1));
     insertUndoType insertUndo;
     claimUndoType expireUndo;
     insertUndoType insertSupportUndo;
@@ -356,7 +355,7 @@ BOOST_AUTO_TEST_CASE(normalization_does_not_kill_supports)
     fixture.DecrementBlocks(1);
     BOOST_CHECK(fixture.best_claim_effective_amount_equals("A", 2));
     fixture.IncrementBlocks(5);
-    BOOST_CHECK(fixture.best_claim_effective_amount_equals("a", 3));
+    BOOST_CHECK(fixture.best_claim_effective_amount_equals("a", 2));
 }
 
 BOOST_AUTO_TEST_CASE(normalization_does_not_fail_on_spend)