Add protocol versions to {Worker,Serve}Proto::*Conn

This will allow us to factor out logic, which is currently scattered
inline, into several reusable instances

The tests are also updated to support versioning. Currently all Worker
and Serve protocol tests are using the minimum version, since no
version-specific serialisers have been created yet. But in subsequent
commits when that changes, we will test individual versions to ensure
complete coverage.

src/libstore/daemon.cc

@@ -277,8 +277,14 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
     TrustedFlag trusted, RecursiveFlag recursive, WorkerProto::Version clientVersion,
     Source & from, BufferedSink & to, WorkerProto::Op op)
 {
-    WorkerProto::ReadConn rconn { .from = from };
+    WorkerProto::ReadConn rconn {
-    WorkerProto::WriteConn wconn { .to = to };
+        .from = from,
+        .version = clientVersion,
+    };
+    WorkerProto::WriteConn wconn {
+        .to = to,
+        .version = clientVersion,
+    };
 
     switch (op) {
 
@@ -1052,7 +1058,10 @@ void processConnection(
         auto temp = trusted
             ? store->isTrustedClient()
             : std::optional { NotTrusted };
-        WorkerProto::WriteConn wconn { .to = to };
+        WorkerProto::WriteConn wconn {
+            .to = to,
+            .version = clientVersion,
+        };
         WorkerProto::write(*store, wconn, temp);
     }
 

src/libstore/legacy-ssh-store.cc

@@ -60,6 +60,7 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor
         {
             return ServeProto::ReadConn {
                 .from = from,
+                .version = remoteVersion,
             };
         }
 
@@ -75,6 +76,7 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor
         {
             return ServeProto::WriteConn {
                 .to = to,
+                .version = remoteVersion,
             };
         }
     };

src/libstore/path-info.cc

@@ -141,7 +141,10 @@ ValidPathInfo ValidPathInfo::read(Source & source, const Store & store, unsigned
     ValidPathInfo info(path, narHash);
     if (deriver != "") info.deriver = store.parseStorePath(deriver);
     info.references = WorkerProto::Serialise<StorePathSet>::read(store,
-        WorkerProto::ReadConn { .from = source });
+        WorkerProto::ReadConn {
+            .from = source,
+            .version = format,
+        });
     source >> info.registrationTime >> info.narSize;
     if (format >= 16) {
         source >> info.ultimate;
@@ -163,7 +166,10 @@ void ValidPathInfo::write(
     sink << (deriver ? store.printStorePath(*deriver) : "")
          << narHash.to_string(Base16, false);
     WorkerProto::write(store,
-        WorkerProto::WriteConn { .to = sink },
+        WorkerProto::WriteConn {
+            .to = sink,
+            .version = format,
+        },
         references);
     sink << registrationTime << narSize;
     if (format >= 16) {

src/libstore/remote-store-connection.hh

@@ -70,6 +70,7 @@ struct RemoteStore::Connection
     {
         return WorkerProto::ReadConn {
             .from = from,
+            .version = daemonVersion,
         };
     }
 
@@ -85,6 +86,7 @@ struct RemoteStore::Connection
     {
         return WorkerProto::WriteConn {
             .to = to,
+            .version = daemonVersion,
         };
     }
 

src/libstore/serve-protocol.hh

@@ -40,23 +40,19 @@ struct ServeProto
     /**
      * A unidirectional read connection, to be used by the read half of the
      * canonical serializers below.
-     *
-     * This currently is just a `Source &`, but more fields will be added
-     * later.
      */
     struct ReadConn {
         Source & from;
+        Version version;
     };
 
     /**
      * A unidirectional write connection, to be used by the write half of the
      * canonical serializers below.
-     *
-     * This currently is just a `Sink &`, but more fields will be added
-     * later.
      */
     struct WriteConn {
         Sink & to;
+        Version version;
     };
 
     /**

src/libstore/tests/common-protocol.cc

@@ -13,10 +13,71 @@ namespace nix {
 
 const char commonProtoDir[] = "common-protocol";
 
-using CommonProtoTest = ProtoTest<CommonProto, commonProtoDir>;
+class CommonProtoTest : public ProtoTest<CommonProto, commonProtoDir>
+{
+public:
+    /**
+     * Golden test for `T` reading
+     */
+    template<typename T>
+    void readTest(PathView testStem, T value)
+    {
+        if (testAccept())
+        {
+            GTEST_SKIP() << "Cannot read golden master because another test is also updating it";
+        }
+        else
+        {
+            auto expected = readFile(goldenMaster(testStem));
+
+            T got = ({
+                StringSource from { expected };
+                CommonProto::Serialise<T>::read(
+                    *store,
+                    CommonProto::ReadConn { .from = from });
+            });
+
+            ASSERT_EQ(got, value);
+        }
+    }
+
+    /**
+     * Golden test for `T` write
+     */
+    template<typename T>
+    void writeTest(PathView testStem, const T & value)
+    {
+        auto file = goldenMaster(testStem);
+
+        StringSink to;
+        CommonProto::write(
+            *store,
+            CommonProto::WriteConn { .to = to },
+            value);
+
+        if (testAccept())
+        {
+            createDirs(dirOf(file));
+            writeFile(file, to.s);
+            GTEST_SKIP() << "Updating golden master";
+        }
+        else
+        {
+            auto expected = readFile(file);
+            ASSERT_EQ(to.s, expected);
+        }
+    }
+};
+
+#define CHARACTERIZATION_TEST(NAME, STEM, VALUE) \
+    TEST_F(CommonProtoTest, NAME ## _read) { \
+        readTest(STEM, VALUE); \
+    } \
+    TEST_F(CommonProtoTest, NAME ## _write) { \
+        writeTest(STEM, VALUE); \
+    }
 
 CHARACTERIZATION_TEST(
-    CommonProtoTest,
     string,
     "string",
     (std::tuple<std::string, std::string, std::string, std::string, std::string> {
@@ -28,7 +89,6 @@ CHARACTERIZATION_TEST(
     }))
 
 CHARACTERIZATION_TEST(
-    CommonProtoTest,
     storePath,
     "store-path",
     (std::tuple<StorePath, StorePath> {
@@ -37,7 +97,6 @@ CHARACTERIZATION_TEST(
     }))
 
 CHARACTERIZATION_TEST(
-    CommonProtoTest,
     contentAddress,
     "content-address",
     (std::tuple<ContentAddress, ContentAddress, ContentAddress> {
@@ -56,7 +115,6 @@ CHARACTERIZATION_TEST(
     }))
 
 CHARACTERIZATION_TEST(
-    CommonProtoTest,
     drvOutput,
     "drv-output",
     (std::tuple<DrvOutput, DrvOutput> {
@@ -71,7 +129,6 @@ CHARACTERIZATION_TEST(
     }))
 
 CHARACTERIZATION_TEST(
-    CommonProtoTest,
     realisation,
     "realisation",
     (std::tuple<Realisation, Realisation> {
@@ -103,7 +160,6 @@ CHARACTERIZATION_TEST(
     }))
 
 CHARACTERIZATION_TEST(
-    CommonProtoTest,
     vector,
     "vector",
     (std::tuple<std::vector<std::string>, std::vector<std::string>, std::vector<std::string>, std::vector<std::vector<std::string>>> {
@@ -114,7 +170,6 @@ CHARACTERIZATION_TEST(
     }))
 
 CHARACTERIZATION_TEST(
-    CommonProtoTest,
     set,
     "set",
     (std::tuple<std::set<std::string>, std::set<std::string>, std::set<std::string>, std::set<std::set<std::string>>> {
@@ -125,7 +180,6 @@ CHARACTERIZATION_TEST(
     }))
 
 CHARACTERIZATION_TEST(
-    CommonProtoTest,
     optionalStorePath,
     "optional-store-path",
     (std::tuple<std::optional<StorePath>, std::optional<StorePath>> {
@@ -136,7 +190,6 @@ CHARACTERIZATION_TEST(
     }))
 
 CHARACTERIZATION_TEST(
-    CommonProtoTest,
     optionalContentAddress,
     "optional-content-address",
     (std::tuple<std::optional<ContentAddress>, std::optional<ContentAddress>> {

src/libstore/tests/protocol.hh

@@ -9,26 +9,23 @@ namespace nix {
 template<class Proto, const char * protocolDir>
 class ProtoTest : public LibStoreTest
 {
-    /**
+protected:
-     * Read this as simply `using S = Inner::Serialise;`.
-     *
-     * See `LengthPrefixedProtoHelper::S` for the same trick, and its
-     * rationale.
-     */
-    template<typename U> using S = typename Proto::template Serialise<U>;
-
-public:
     Path unitTestData = getUnitTestData() + "/libstore/" + protocolDir;
 
     Path goldenMaster(std::string_view testStem) {
         return unitTestData + "/" + testStem + ".bin";
     }
+};
 
+template<class Proto, const char * protocolDir>
+class VersionedProtoTest : public ProtoTest<Proto, protocolDir>
+{
+public:
     /**
      * Golden test for `T` reading
      */
     template<typename T>
-    void readTest(PathView testStem, T value)
+    void readTest(PathView testStem, typename Proto::Version version, T value)
     {
         if (testAccept())
         {
@@ -36,13 +33,16 @@ public:
         }
         else
         {
-            auto expected = readFile(goldenMaster(testStem));
+            auto expected = readFile(ProtoTest<Proto, protocolDir>::goldenMaster(testStem));
 
             T got = ({
                 StringSource from { expected };
-                S<T>::read(
+                Proto::template Serialise<T>::read(
-                    *store,
+                    *LibStoreTest::store,
-                    typename Proto::ReadConn { .from = from });
+                    typename Proto::ReadConn {
+                        .from = from,
+                        .version = version,
+                    });
             });
 
             ASSERT_EQ(got, value);
@@ -53,14 +53,17 @@ public:
      * Golden test for `T` write
      */
     template<typename T>
-    void writeTest(PathView testStem, const T & value)
+    void writeTest(PathView testStem, typename Proto::Version version, const T & value)
     {
-        auto file = goldenMaster(testStem);
+        auto file = ProtoTest<Proto, protocolDir>::goldenMaster(testStem);
 
         StringSink to;
         Proto::write(
-            *store,
+            *LibStoreTest::store,
-            typename Proto::WriteConn { .to = to },
+            typename Proto::WriteConn {
+                .to = to,
+                .version = version,
+            },
             value);
 
         if (testAccept())
@@ -77,12 +80,12 @@ public:
     }
 };
 
-#define CHARACTERIZATION_TEST(FIXTURE, NAME, STEM, VALUE) \
+#define VERSIONED_CHARACTERIZATION_TEST(FIXTURE, NAME, STEM, VERSION, VALUE) \
     TEST_F(FIXTURE, NAME ## _read) { \
-        readTest(STEM, VALUE); \
+        readTest(STEM, VERSION, VALUE); \
     } \
     TEST_F(FIXTURE, NAME ## _write) { \
-        writeTest(STEM, VALUE); \
+        writeTest(STEM, VERSION, VALUE); \
     }
 
 }

src/libstore/tests/serve-protocol.cc

@@ -11,14 +11,22 @@
 
 namespace nix {
 
-const char commonProtoDir[] = "serve-protocol";
+const char serveProtoDir[] = "serve-protocol";
 
-using ServeProtoTest = ProtoTest<ServeProto, commonProtoDir>;
+struct ServeProtoTest : VersionedProtoTest<ServeProto, serveProtoDir>
+{
+    /**
+     * For serializers that don't care about the minimum version, we
+     * used the oldest one: 1.0.
+     */
+    ServeProto::Version defaultVersion = 1 << 8 | 0;
+};
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     ServeProtoTest,
     string,
     "string",
+    defaultVersion,
     (std::tuple<std::string, std::string, std::string, std::string, std::string> {
         "",
         "hi",
@@ -27,19 +35,21 @@ CHARACTERIZATION_TEST(
         "oh no \0\0\0 what was that!",
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     ServeProtoTest,
     storePath,
     "store-path",
+    defaultVersion,
     (std::tuple<StorePath, StorePath> {
         StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo" },
         StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo-bar" },
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     ServeProtoTest,
     contentAddress,
     "content-address",
+    defaultVersion,
     (std::tuple<ContentAddress, ContentAddress, ContentAddress> {
         ContentAddress {
             .method = TextIngestionMethod {},
@@ -55,10 +65,11 @@ CHARACTERIZATION_TEST(
         },
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     ServeProtoTest,
     drvOutput,
     "drv-output",
+    defaultVersion,
     (std::tuple<DrvOutput, DrvOutput> {
         {
             .drvHash = Hash::parseSRI("sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="),
@@ -70,10 +81,11 @@ CHARACTERIZATION_TEST(
         },
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     ServeProtoTest,
     realisation,
     "realisation",
+    defaultVersion,
     (std::tuple<Realisation, Realisation> {
         Realisation {
             .id = DrvOutput {
@@ -102,10 +114,11 @@ CHARACTERIZATION_TEST(
         },
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     ServeProtoTest,
     vector,
     "vector",
+    defaultVersion,
     (std::tuple<std::vector<std::string>, std::vector<std::string>, std::vector<std::string>, std::vector<std::vector<std::string>>> {
         { },
         { "" },
@@ -113,10 +126,11 @@ CHARACTERIZATION_TEST(
         { {}, { "" }, { "", "1", "2" } },
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     ServeProtoTest,
     set,
     "set",
+    defaultVersion,
     (std::tuple<std::set<std::string>, std::set<std::string>, std::set<std::string>, std::set<std::set<std::string>>> {
         { },
         { "" },
@@ -124,10 +138,11 @@ CHARACTERIZATION_TEST(
         { {}, { "" }, { "", "1", "2" } },
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     ServeProtoTest,
     optionalStorePath,
     "optional-store-path",
+    defaultVersion,
     (std::tuple<std::optional<StorePath>, std::optional<StorePath>> {
         std::nullopt,
         std::optional {
@@ -135,10 +150,11 @@ CHARACTERIZATION_TEST(
         },
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     ServeProtoTest,
     optionalContentAddress,
     "optional-content-address",
+    defaultVersion,
     (std::tuple<std::optional<ContentAddress>, std::optional<ContentAddress>> {
         std::nullopt,
         std::optional {

src/libstore/tests/worker-protocol.cc

@@ -14,12 +14,21 @@ namespace nix {
 
 const char workerProtoDir[] = "worker-protocol";
 
-using WorkerProtoTest = ProtoTest<WorkerProto, workerProtoDir>;
+struct WorkerProtoTest : VersionedProtoTest<WorkerProto, workerProtoDir>
+{
+    /**
+     * For serializers that don't care about the minimum version, we
+     * used the oldest one: 1.0.
+     */
+    WorkerProto::Version defaultVersion = 1 << 8 | 0;
+};
 
-CHARACTERIZATION_TEST(
+
+VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
     string,
     "string",
+    defaultVersion,
     (std::tuple<std::string, std::string, std::string, std::string, std::string> {
         "",
         "hi",
@@ -28,19 +37,21 @@ CHARACTERIZATION_TEST(
         "oh no \0\0\0 what was that!",
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
     storePath,
     "store-path",
+    defaultVersion,
     (std::tuple<StorePath, StorePath> {
         StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo" },
         StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo-bar" },
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
     contentAddress,
     "content-address",
+    defaultVersion,
     (std::tuple<ContentAddress, ContentAddress, ContentAddress> {
         ContentAddress {
             .method = TextIngestionMethod {},
@@ -56,10 +67,11 @@ CHARACTERIZATION_TEST(
         },
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
     derivedPath,
     "derived-path",
+    defaultVersion,
     (std::tuple<DerivedPath, DerivedPath> {
         DerivedPath::Opaque {
             .path = StorePath { "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-foo" },
@@ -72,10 +84,11 @@ CHARACTERIZATION_TEST(
         },
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
     drvOutput,
     "drv-output",
+    defaultVersion,
     (std::tuple<DrvOutput, DrvOutput> {
         {
             .drvHash = Hash::parseSRI("sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="),
@@ -87,10 +100,11 @@ CHARACTERIZATION_TEST(
         },
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
     realisation,
     "realisation",
+    defaultVersion,
     (std::tuple<Realisation, Realisation> {
         Realisation {
             .id = DrvOutput {
@@ -119,10 +133,11 @@ CHARACTERIZATION_TEST(
         },
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
     buildResult,
     "build-result",
+    defaultVersion,
     ({
         using namespace std::literals::chrono_literals;
         std::tuple<BuildResult, BuildResult, BuildResult> t {
@@ -177,10 +192,11 @@ CHARACTERIZATION_TEST(
         t;
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
     keyedBuildResult,
     "keyed-build-result",
+    defaultVersion,
     ({
         using namespace std::literals::chrono_literals;
         std::tuple<KeyedBuildResult, KeyedBuildResult/*, KeyedBuildResult*/> t {
@@ -213,20 +229,22 @@ CHARACTERIZATION_TEST(
         t;
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
     optionalTrustedFlag,
     "optional-trusted-flag",
+    defaultVersion,
     (std::tuple<std::optional<TrustedFlag>, std::optional<TrustedFlag>, std::optional<TrustedFlag>> {
         std::nullopt,
         std::optional { Trusted },
         std::optional { NotTrusted },
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
     vector,
     "vector",
+    defaultVersion,
     (std::tuple<std::vector<std::string>, std::vector<std::string>, std::vector<std::string>, std::vector<std::vector<std::string>>> {
         { },
         { "" },
@@ -234,10 +252,11 @@ CHARACTERIZATION_TEST(
         { {}, { "" }, { "", "1", "2" } },
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
     set,
     "set",
+    defaultVersion,
     (std::tuple<std::set<std::string>, std::set<std::string>, std::set<std::string>, std::set<std::set<std::string>>> {
         { },
         { "" },
@@ -245,10 +264,11 @@ CHARACTERIZATION_TEST(
         { {}, { "" }, { "", "1", "2" } },
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
     optionalStorePath,
     "optional-store-path",
+    defaultVersion,
     (std::tuple<std::optional<StorePath>, std::optional<StorePath>> {
         std::nullopt,
         std::optional {
@@ -256,10 +276,11 @@ CHARACTERIZATION_TEST(
         },
     }))
 
-CHARACTERIZATION_TEST(
+VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
     optionalContentAddress,
     "optional-content-address",
+    defaultVersion,
     (std::tuple<std::optional<ContentAddress>, std::optional<ContentAddress>> {
         std::nullopt,
         std::optional {

src/libstore/worker-protocol.hh

@@ -57,23 +57,19 @@ struct WorkerProto
     /**
      * A unidirectional read connection, to be used by the read half of the
      * canonical serializers below.
-     *
-     * This currently is just a `Source &`, but more fields will be added
-     * later.
      */
     struct ReadConn {
         Source & from;
+        Version version;
     };
 
     /**
      * A unidirectional write connection, to be used by the write half of the
      * canonical serializers below.
-     *
-     * This currently is just a `Sink &`, but more fields will be added
-     * later.
      */
     struct WriteConn {
         Sink & to;
+        Version version;
     };
 
     /**

src/nix-store/nix-store.cc

@@ -820,8 +820,14 @@ static void opServe(Strings opFlags, Strings opArgs)
     out.flush();
     ServeProto::Version clientVersion = readInt(in);
 
-    ServeProto::ReadConn rconn { .from = in };
+    ServeProto::ReadConn rconn {
-    ServeProto::WriteConn wconn { .to = out };
+        .from = in,
+        .version = clientVersion,
+    };
+    ServeProto::WriteConn wconn {
+        .to = out,
+        .version = clientVersion,
+    };
 
     auto getBuildSettings = [&]() {
         // FIXME: changing options here doesn't work if we're