From 1f739961e5639635246ae9fd5491adb0a6c02cf6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= <joerg@thalheim.io>
Date: Thu, 18 Dec 2025 09:28:12 +0100
Subject: [PATCH] libstore: simplify registerValidPaths by removing redundant
 checkInvariants loop

The separate checkInvariants loop after addValidPath was added in 2014
(d210cdc43) to work around an assertion failure:

  nix-store: derivations.cc:242: Assertion 'store.isValidPath(i->first)' failed.

At that time, hashDerivationModulo() contained assert(store.isValidPath(...))
which required input derivations to be registered as valid in the database
before computing their hash. The workaround was to:
1. Call addValidPath with checkOutputs=false
2. Add all references to the database
3. Run checkInvariants in a separate loop after paths were valid

In 2020 (bccff827d), the isValidPath assertion was removed to fix a
deadlock in IFD through the daemon (issue #4235). The fix changed
hashDerivationModulo to use readInvalidDerivation, which reads directly
from the filesystem without requiring database validity.

This made the separate checkInvariants loop unnecessary, but nobody
noticed the code could be simplified. The comment "We can't do this in
addValidPath() above, because the references might not be valid yet"
became stale.

Now we simply call addValidPath() with the default checkOutputs=true,
which runs checkInvariants internally using the already-parsed
derivation. This commit eliminates the separate loop over derivations.
---
 src/libstore/include/nix/store/local-store.hh |  3 +-
 src/libstore/local-store.cc                   | 31 +++----------------
 2 files changed, 5 insertions(+), 29 deletions(-)

diff --git a/src/libstore/include/nix/store/local-store.hh b/src/libstore/include/nix/store/local-store.hh
index 0dd0498a2..b5de22095 100644
--- a/src/libstore/include/nix/store/local-store.hh
+++ b/src/libstore/include/nix/store/local-store.hh
@@ -420,8 +420,7 @@ private:
 
     uint64_t queryValidPathId(State & state, const StorePath & path);
 
-    uint64_t
-    addValidPath(State & state, const ValidPathInfo & info, bool checkOutputs = true, Derivation * drv = nullptr);
+    uint64_t addValidPath(State & state, const ValidPathInfo & info);
 
     void invalidatePath(State & state, const StorePath & path);
 
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index 2a5c65be2..70ce6b4f2 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -647,7 +647,7 @@ void LocalStore::cacheDrvOutputMapping(
         [&]() { state.stmts->AddDerivationOutput.use()(deriver)(outputName) (printStorePath(output)).exec(); });
 }
 
-uint64_t LocalStore::addValidPath(State & state, const ValidPathInfo & info, bool checkOutputs, Derivation * drv)
+uint64_t LocalStore::addValidPath(State & state, const ValidPathInfo & info)
 {
     if (info.ca.has_value() && !info.isContentAddressed(*this))
         throw Error(
@@ -675,8 +675,7 @@ uint64_t LocalStore::addValidPath(State & state, const ValidPathInfo & info, boo
            derivations).  Note that if this throws an error, then the
            DB transaction is rolled back, so the path validity
            registration above is undone. */
-        if (checkOutputs)
-            parsedDrv.checkInvariants(*this, info.path);
+        parsedDrv.checkInvariants(*this, info.path);
 
         for (auto & i : parsedDrv.outputsAndOptPaths(*this)) {
             /* Floating CA derivations have indeterminate output paths until
@@ -684,9 +683,6 @@ uint64_t LocalStore::addValidPath(State & state, const ValidPathInfo & info, boo
             if (i.second.second)
                 cacheDrvOutputMapping(state, id, i.first, *i.second.second);
         }
-
-        if (drv)
-            *drv = std::move(parsedDrv);
     }
 
     pathInfoCache->lock()->upsert(info.path, PathInfoCacheValue{.value = std::make_shared<const ValidPathInfo>(info)});
@@ -927,19 +923,12 @@ void LocalStore::registerValidPaths(const ValidPathInfos & infos)
         SQLiteTxn txn(state->db);
         StorePathSet paths;
 
-        std::map<StorePath, Derivation> derivations;
-
         for (auto & [_, i] : infos) {
             assert(i.narHash.algo == HashAlgorithm::SHA256);
             if (isValidPath_(*state, i.path))
                 updatePathInfo(*state, i);
-            else if (i.path.isDerivation()) {
-                Derivation drv;
-                addValidPath(*state, i, false, &drv);
-                derivations.emplace(i.path, std::move(drv));
-            } else {
-                addValidPath(*state, i, false);
-            }
+            else
+                addValidPath(*state, i);
             paths.insert(i.path);
         }
 
@@ -949,18 +938,6 @@ void LocalStore::registerValidPaths(const ValidPathInfos & infos)
                 state->stmts->AddReference.use()(referrer)(queryValidPathId(*state, j)).exec();
         }
 
-        /* Check that the derivation outputs are correct.  We can't do
-           this in addValidPath() above, because the references might
-           not be valid yet. */
-        for (auto & [_, i] : infos)
-            if (i.path.isDerivation()) {
-                if (auto drv = derivations.find(i.path); drv != derivations.end()) {
-                    drv->second.checkInvariants(*this, i.path);
-                } else {
-                    readInvalidDerivation(i.path).checkInvariants(*this, i.path);
-                }
-            }
-
         /* Do a topological sort of the paths.  This will throw an
            error if a cycle is detected and roll back the
            transaction.  Cycles can only occur when a derivation