From 50912d02e272985da8bdb5c03ebf85c126c7dde7 Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Thu, 31 Mar 2022 23:14:18 +0000
Subject: [PATCH] Get rid of `impureOutputHash`

I do not believe there is any problem with computing
`hashDerivationModulo` the normal way with impure derivations.

Conversely, the way this used to work is very suspicious because two
almost-equal derivations that only differ in depending on different
impure derivations could have the same drv hash modulo. That is very
suspicious because there is no reason to think those two different
impure derivations will end up producing the same content-addressed
data!

Co-authored-by: Alain Zscheile <zseri.devel@ytrizja.de>
---
 src/libstore/build/derivation-goal.cc | 59 ++++++++++++++-------------
 src/libstore/derivations.cc           | 12 +-----
 src/libstore/derivations.hh           |  2 -
 3 files changed, 32 insertions(+), 41 deletions(-)

diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc
index d09da1f5584..a167d926111 100644
--- a/src/libstore/build/derivation-goal.cc
+++ b/src/libstore/build/derivation-goal.cc
@@ -185,41 +185,44 @@ Goal::Co DerivationGoal::haveDerivation()
     if (!drv->type().hasKnownOutputPaths())
         experimentalFeatureSettings.require(Xp::CaDerivations);
 
-    if (drv->type().isImpure()) {
-        experimentalFeatureSettings.require(Xp::ImpureDerivations);
-
-        for (auto & [outputName, output] : drv->outputs) {
-            auto randomPath = StorePath::random(outputPathName(drv->name, outputName));
-            assert(!worker.store.isValidPath(randomPath));
-            initialOutputs.insert({
-                outputName,
-                InitialOutput {
-                    .wanted = true,
-                    .outputHash = impureOutputHash,
-                    .known = InitialOutputStatus {
-                        .path = randomPath,
-                        .status = PathStatus::Absent
-                    }
-                }
-            });
-        }
-
-        co_return gaveUpOnSubstitution();
-    }
-
     for (auto & i : drv->outputsAndOptPaths(worker.store))
         if (i.second.second)
             worker.store.addTempRoot(*i.second.second);
 
-    auto outputHashes = staticOutputHashes(worker.evalStore, *drv);
-    for (auto & [outputName, outputHash] : outputHashes)
-        initialOutputs.insert({
-            outputName,
-            InitialOutput {
+    {
+        bool impure = drv->type().isImpure();
+
+        if (impure) experimentalFeatureSettings.require(Xp::ImpureDerivations);
+
+        auto outputHashes = staticOutputHashes(worker.evalStore, *drv);
+        for (auto & [outputName, outputHash] : outputHashes) {
+            InitialOutput v{
                 .wanted = true, // Will be refined later
                 .outputHash = outputHash
+            };
+
+            /* TODO we might want to also allow randomizing the paths
+               for regular CA derivations, e.g. for sake of checking
+               determinism. */
+            if (impure) {
+                v.known = InitialOutputStatus {
+                    .path = StorePath::random(outputPathName(drv->name, outputName)),
+                    .status = PathStatus::Absent,
+                };
             }
-        });
+
+            initialOutputs.insert({
+                outputName,
+                std::move(v),
+            });
+        }
+
+        if (impure) {
+            /* We don't yet have any safe way to cache an impure derivation at
+               this step. */
+            co_return gaveUpOnSubstitution();
+        }
+    }
 
     {
         /* Check what outputs paths are not already valid. */
diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc
index 5d01c577cbe..b54838a0aa9 100644
--- a/src/libstore/derivations.cc
+++ b/src/libstore/derivations.cc
@@ -843,16 +843,6 @@ DrvHash hashDerivationModulo(Store & store, const Derivation & drv, bool maskOut
         };
     }
 
-    if (type.isImpure()) {
-        std::map<std::string, Hash> outputHashes;
-        for (const auto & [outputName, _] : drv.outputs)
-            outputHashes.insert_or_assign(outputName, impureOutputHash);
-        return DrvHash {
-            .hashes = outputHashes,
-            .kind = DrvHash::Kind::Deferred,
-        };
-    }
-
     auto kind = std::visit(overloaded {
         [](const DerivationType::InputAddressed & ia) {
             /* This might be a "pesimistically" deferred output, so we don't
@@ -865,7 +855,7 @@ DrvHash hashDerivationModulo(Store & store, const Derivation & drv, bool maskOut
                 : DrvHash::Kind::Deferred;
         },
         [](const DerivationType::Impure &) -> DrvHash::Kind {
-            assert(false);
+            return DrvHash::Kind::Deferred;
         }
     }, drv.type().raw);
 
diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh
index 7856aa9b9fc..5b2101ed53c 100644
--- a/src/libstore/derivations.hh
+++ b/src/libstore/derivations.hh
@@ -526,6 +526,4 @@ void writeDerivation(Sink & out, const StoreDirConfig & store, const BasicDeriva
  */
 std::string hashPlaceholder(const OutputNameView outputName);
 
-extern const Hash impureOutputHash;
-
 }