Re-add --output-file to verify-checkpoints

src/historywork/WriteVerifiedCheckpointHashesWork.cpp

@@ -89,8 +89,8 @@ WriteVerifiedCheckpointHashesWork::loadHashFromJsonOutput(
 }
 
 WriteVerifiedCheckpointHashesWork::WriteVerifiedCheckpointHashesWork(
-    Application& app, LedgerNumHashPair rangeEnd,
-    std::string const& trustedHashFile,
+    Application& app, LedgerNumHashPair rangeEnd, std::string const& outputFile,
+    std::optional<std::string> const& trustedHashFile,
     std::optional<uint32_t> const& fromLedger, uint32_t nestedBatchSize,
     std::shared_ptr<HistoryArchive> archive)
     : BatchWork(app, "write-verified-checkpoint-hashes")
@@ -101,7 +101,7 @@ WriteVerifiedCheckpointHashesWork::WriteVerifiedCheckpointHashesWork(
     , mCurrCheckpoint(rangeEnd.first)
     , mArchive(archive)
     , mTrustedHashFileName(trustedHashFile)
-    , mOutputFileName(mTrustedHashFileName + ".tmp")
+    , mOutputFileName(outputFile)
     , mFromLedger(fromLedger)
 {
     mRangeEndPromise.set_value(mRangeEnd);
@@ -238,13 +238,13 @@ WriteVerifiedCheckpointHashesWork::startOutputFile()
 void
 WriteVerifiedCheckpointHashesWork::maybeParseTrustedHashFile()
 {
-    if (mFromLedger)
+    if (mFromLedger || !mTrustedHashFileName)
     {
         return;
     }
     mLatestTrustedHashPair =
-        loadLatestHashPairFromJsonOutput(mTrustedHashFileName);
-    CLOG_INFO(History, "trusted hash from {}: {}", mTrustedHashFileName,
+        loadLatestHashPairFromJsonOutput(*mTrustedHashFileName);
+    CLOG_INFO(History, "trusted hash from {}: {}", *mTrustedHashFileName,
               hexAbbrev(*mLatestTrustedHashPair->second));
 }
 
@@ -253,11 +253,12 @@ WriteVerifiedCheckpointHashesWork::endOutputFile()
 {
     if (mOutputFile && mOutputFile->is_open())
     {
-        if (std::filesystem::exists(mTrustedHashFileName))
+        if (mTrustedHashFileName &&
+            std::filesystem::exists(*mTrustedHashFileName))
         {
             // Append everything except the first line of mTrustedHashFile to
             // mOutputFile.
-            std::ifstream trustedHashFile(mTrustedHashFileName);
+            std::ifstream trustedHashFile(*mTrustedHashFileName);
             if (trustedHashFile)
             {
                 std::string line;
@@ -273,7 +274,7 @@ WriteVerifiedCheckpointHashesWork::endOutputFile()
             else
             {
                 CLOG_WARNING(History, "failed to open trusted hash file {}",
-                             mTrustedHashFileName);
+                             *mTrustedHashFileName);
             }
         }
         else
@@ -287,12 +288,6 @@ WriteVerifiedCheckpointHashesWork::endOutputFile()
         }
         mOutputFile->close();
         mOutputFile.reset();
-        // Rename mOutputFileName to mTrustedHashFileName.
-        if (std::rename(mOutputFileName.c_str(), mTrustedHashFileName.c_str()))
-        {
-            CLOG_ERROR(History, "failed to rename {} to {}", mOutputFileName,
-                       mTrustedHashFileName);
-        }
     }
 }
 

src/historywork/WriteVerifiedCheckpointHashesWork.h

@@ -27,7 +27,8 @@ class WriteVerifiedCheckpointHashesWork : public BatchWork
   public:
     WriteVerifiedCheckpointHashesWork(
         Application& app, LedgerNumHashPair rangeEnd,
-        std::string const& trustedHashFile,
+        std::string const& outputFile,
+        std::optional<std::string> const& trustedHashFile,
         std::optional<uint32_t> const& fromLedger,
         uint32_t nestedBatchSize = NESTED_DOWNLOAD_BATCH_SIZE,
         std::shared_ptr<HistoryArchive> archive = nullptr);
@@ -84,7 +85,7 @@ class WriteVerifiedCheckpointHashesWork : public BatchWork
     void startOutputFile();
     void endOutputFile();
     std::shared_ptr<std::ofstream> mOutputFile;
-    std::string const mTrustedHashFileName;
+    std::optional<std::string> const mTrustedHashFileName;
     std::string const mOutputFileName;
     std::optional<LedgerNumHashPair> mLatestTrustedHashPair;
     std::optional<uint32_t> const& mFromLedger;

src/historywork/test/HistoryWorkTests.cpp

@@ -33,29 +33,33 @@ TEST_CASE("write verified checkpoint hashes", "[historywork]")
     LedgerNumHashPair pair = pairs.back();
     auto tmpDir = catchupSimulation.getApp().getTmpDirManager().tmpDir(
         "write-checkpoint-hashes-test");
-    std::string const file = tmpDir.getName() + "/verified-ledgers.json";
+    std::string file = tmpDir.getName() + "/verified-ledgers.json";
     auto& wm = catchupSimulation.getApp().getWorkScheduler();
     std::optional<std::uint32_t> noFromLedger = std::nullopt;
+    std::optional<std::string> noVerifiedLedgerFile = std::nullopt;
 
-    size_t startingPair = 0;
+    size_t startingPairIdx = 0;
     {
         SECTION("from genesis")
         {
             auto w = wm.executeWork<WriteVerifiedCheckpointHashesWork>(
-                pair, file, noFromLedger, nestedBatchSize);
+                pair, file, noVerifiedLedgerFile, noFromLedger,
+                nestedBatchSize);
             REQUIRE(w->getState() == BasicWork::State::WORK_SUCCESS);
         }
         SECTION("from specified ledger")
         {
-            startingPair = 1;
-            std::optional<std::uint32_t> fromLedger = pairs[startingPair].first;
+            startingPairIdx = 1;
+            std::optional<std::uint32_t> fromLedger =
+                pairs[startingPairIdx].first;
             auto w = wm.executeWork<WriteVerifiedCheckpointHashesWork>(
-                pair, file, fromLedger, nestedBatchSize);
+                pair, file, noVerifiedLedgerFile, fromLedger, nestedBatchSize);
             REQUIRE(w->getState() == BasicWork::State::WORK_SUCCESS);
         }
     }
 
-    auto checkFileContents = [&](const auto& pairs) {
+    auto checkFileContents = [](const auto& pairs, auto startingPairIdx,
+                                std::string file) {
         for (size_t i = 0; i < pairs.size(); ++i)
         {
             auto p = pairs[i];
@@ -65,7 +69,7 @@ TEST_CASE("write verified checkpoint hashes", "[historywork]")
                 p.first, file);
             // If we did not start from the beginning, the hashes before the
             // starting pair should not be in the file.
-            if (i < startingPair)
+            if (i < startingPairIdx)
             {
                 REQUIRE(h == Hash{});
             }
@@ -82,7 +86,7 @@ TEST_CASE("write verified checkpoint hashes", "[historywork]")
         REQUIRE(latest->first == pairs.back().first);
     };
 
-    checkFileContents(pairs);
+    checkFileContents(pairs, startingPairIdx, file);
 
     // Advance the simulation.
     auto secondCheckpointLedger =
@@ -91,15 +95,17 @@ TEST_CASE("write verified checkpoint hashes", "[historywork]")
                                                   5 * nestedBatchSize);
     pairs = catchupSimulation.getAllPublishedCheckpoints();
 
+    std::optional<std::string> trustedHashFile = file;
+    file += ".new";
     // Run work again with existing file.
     {
         auto w = wm.executeWork<WriteVerifiedCheckpointHashesWork>(
-            pairs.back(), file, noFromLedger, nestedBatchSize);
+            pairs.back(), file, trustedHashFile, noFromLedger, nestedBatchSize);
         REQUIRE(w->getState() == BasicWork::State::WORK_SUCCESS);
     }
 
     // Ensure the file contains all pairs, from the first run and the second.
-    checkFileContents(pairs);
+    checkFileContents(pairs, startingPairIdx, file);
 }
 
 TEST_CASE("check single ledger header work", "[historywork]")

src/main/CommandLine.cpp

@@ -206,9 +206,10 @@ outputFileParser(std::string& string)
 }
 
 clara::Opt
-trustedHashFileParser(std::string& string)
+trustedHashFileParser(std::optional<std::string>& string)
 {
-    return clara::Opt{string, "FILE-NAME"}["--trusted-hash-file"](
+    return clara::Opt{[&](std::string const& arg) { string = arg; },
+                      "FILE-NAME"}["--trusted-hash-file"](
         "file containing trusted hashes, generated by a previous call to "
         "verify-checkpoints or a non-existent file to generate a new one");
 }
@@ -1029,7 +1030,8 @@ runPublish(CommandLineArgs const& args)
 int
 runWriteVerifiedCheckpointHashes(CommandLineArgs const& args)
 {
-    std::string trustedHashFile;
+    std::string outputFile;
+    std::optional<std::string> trustedHashFile;
     uint32_t startLedger = 0;
     std::string startHash;
     std::optional<std::uint32_t> fromLedger;
@@ -1038,8 +1040,14 @@ runWriteVerifiedCheckpointHashes(CommandLineArgs const& args)
         args,
         {configurationParser(configOption), historyLedgerNumber(startLedger),
          historyHashParser(startHash), fromLedgerNumberParser(fromLedger),
-         trustedHashFileParser(trustedHashFile).required()},
+         trustedHashFileParser(trustedHashFile),
+         outputFileParser(outputFile).required()},
         [&] {
+            if (fromLedger && trustedHashFile)
+            {
+                LOG_ERROR(DEFAULT_LOG, "Cannot specify both --from-ledger and "
+                                       "--trusted-hash-file");
+            }
             VirtualClock clock(VirtualClock::REAL_TIME);
             auto cfg = configOption.getConfig();
 
@@ -1066,7 +1074,7 @@ runWriteVerifiedCheckpointHashes(CommandLineArgs const& args)
                 app->getHerder().shutdown();
                 app->getWorkScheduler()
                     .executeWork<WriteVerifiedCheckpointHashesWork>(
-                        authPair, trustedHashFile, fromLedger);
+                        authPair, outputFile, trustedHashFile, fromLedger);
                 app->gracefulStop();
                 return 0;
             }