Merge #2326: [Test][Misc] Fix libsecp256k1 init for unit test setup +…

… Allow to specify blocks storage directory

b0e0d55 doc: add '-blocksdir' option to release-notes (furszy)
50e66c2 Turn TryCreateDirectory() into TryCreateDirectories() (Marko Bencun)
035d3c2 util: Simplify path argument for CBlockTreeDB ctor (Hennadii Stepanov)
a571d24 Fail if either disk space check fails (Ben Woosley)
e567003 Improve blocksdir functional test. (Hennadii Stepanov)
ff0ae45 Make blockdir always net specific (Hennadii Stepanov)
13a4119 doc: Clarify -blocksdir usage (Daniel McNally)
a4ff899 -blocksdir: keep blockindex leveldb database in datadir (Jonas Schnelli)
3054076 QA: Add -blocksdir test (Jonas Schnelli)
39b8127 Allow to optional specify the directory for the blocks storage (Jonas Schnelli)
a821b0b BugFix: Move TestingSetup::ECCVerifyHandle member to BasicTestingSetup. (furszy)
5844452 Remove unneeded GetTempPath, only use temp directory in unit tests (furszy)
7fc893b [Test] BasicTestingSetup constructor receiving the network param. (furszy)

Pull request description:

  Grouped two different, zero risk, modifications here to not have to create two/three small and straightforward PRs:

  #### 1) Unit Tests:

  * Fixing basic unit test setup issue, was not initializing the libsecp256k1 context. Thus why we have been forced to use the extended unit testing setup for almost every test case (which includes CConnman, scheduler, validation interface, etc.. that are not needed so often).
  * Refactor: `BasicTestingSetup` receiving the network in the constructor.

  #### 2) Back ported the ability to specify a custom directory for the blocks storage.

  > Since the actual block files taking up more and more space, it may be desirable to have them stored in a different location then the data directory (use case: SSD for chainstate, etc., HD for blocks).
  This PR adds a -blocksdir option that allows one to keep the blockfiles external from the data directory (instead of creating symlinks).

  * bitcoin#9895.
  * bitcoin#12653.
  * bitcoin#14364.
  * bitcoin#14409.
  * bitcoin#15124.
  * bitcoin#17059.

ACKs for top commit:
  Fuzzbawls:
    re-ACK b0e0d55
  random-zebra:
    utACK b0e0d55 and merging...

Tree-SHA512: 289e5b826c8fc23a5933a5b30e6c043f82bad827b10f3093cbc5869a9e278d7ad0a325dd19b5ad3c97f23a3dee69f5b0bee3869e486eeed5e8d8342a39919cf5

doc/release-notes.md

@@ -44,6 +44,9 @@ Notable Changes
 
 (Developers: add your notes here as part of your pull requests whenever possible)
 
+#### Allow to optional specify the directory for the blocks storage
+
+A new init option flag '-blocksdir' will allow one to keep the blockfiles external from the data directory.
 
 #### Disable PoW mining RPC Commands
 

src/dbwrapper.cpp

@@ -72,7 +72,7 @@ CDBWrapper::CDBWrapper(const fs::path& path, size_t nCacheSize, bool fMemory, bo
             leveldb::Status result = leveldb::DestroyDB(path.string(), options);
             dbwrapper_private::HandleError(result);
         }
-        TryCreateDirectory(path);
+        TryCreateDirectories(path);
         LogPrintf("Opening LevelDB in %s\n", path.string());
     }
     leveldb::Status status = leveldb::DB::Open(options, path.string(), &pdb);

src/init.cpp

@@ -439,6 +439,7 @@ std::string HelpMessage(HelpMessageMode mode)
     strUsage += HelpMessageOpt("-?", _("This help message"));
     strUsage += HelpMessageOpt("-version", _("Print version and exit"));
     strUsage += HelpMessageOpt("-alertnotify=<cmd>", _("Execute command when a relevant alert is received or we see a really long fork (%s in cmd is replaced by message)"));
+    strUsage += HelpMessageOpt("-blocksdir=<dir>", _("Specify directory to hold blocks subdirectory for *.dat files (default: <datadir>)"));
     strUsage += HelpMessageOpt("-blocknotify=<cmd>", _("Execute command when the best block changes (%s in cmd is replaced by block hash)"));
     strUsage += HelpMessageOpt("-checkblocks=<n>", strprintf(_("How many blocks to check at startup (default: %u, 0 = all)"), DEFAULT_CHECKBLOCKS));
     strUsage += HelpMessageOpt("-conf=<file>", strprintf(_("Specify configuration file (default: %s)"), PIVX_CONF_FILENAME));
@@ -1038,6 +1039,10 @@ bool AppInitParameterInteraction()
 {
     // ********************************************************* Step 2: parameter interactions
 
+    if (!fs::is_directory(GetBlocksDir())) {
+        return UIError(strprintf(_("Specified blocks directory \"%s\" does not exist.\n"), gArgs.GetArg("-blocksdir", "").c_str()));
+    }
+
     // Make sure enough file descriptors are available
 
     // -bind and -whitebind can't be set when not listening
@@ -1371,7 +1376,7 @@ bool AppInitMain()
         if (gArgs.GetBoolArg("-resync", false)) {
             uiInterface.InitMessage(_("Preparing for resync..."));
             // Delete the local blockchain folders to force a resync from scratch to get a consitent blockchain-state
-            fs::path blocksDir = GetDataDir() / "blocks";
+            fs::path blocksDir = GetBlocksDir();
             fs::path chainstateDir = GetDataDir() / "chainstate";
             fs::path sporksDir = GetDataDir() / "sporks";
             fs::path zerocoinDir = GetDataDir() / "zerocoin";
@@ -1569,9 +1574,6 @@ bool AppInitMain()
 
     fReindex = gArgs.GetBoolArg("-reindex", false);
 
-    // Create blocks directory if it doesn't already exist
-    fs::create_directories(GetDataDir() / "blocks");
-
     // cache size calculations
     int64_t nTotalCache = (gArgs.GetArg("-dbcache", nDefaultDbCache) << 20);
     nTotalCache = std::max(nTotalCache, nMinDbCache << 20); // total cache cannot be less than nMinDbCache
@@ -1803,8 +1805,14 @@ bool AppInitMain()
 #endif
     // ********************************************************* Step 9: import blocks
 
-    if (!CheckDiskSpace())
+    if (!CheckDiskSpace(/* additional_bytes */ 0, /* blocks_dir */ false)) {
+        UIError(strprintf(_("Error: Disk space is low for %s"), GetDataDir()));
         return false;
+    }
+    if (!CheckDiskSpace(/* additional_bytes */ 0, /* blocks_dir */ true)) {
+        UIError(strprintf(_("Error: Disk space is low for %s"), GetBlocksDir()));
+        return false;
+    }
 
     // Either install a handler to notify us when genesis activates, or set fHaveGenesis directly.
     // No locking, as this happens before any background thread is started.

src/qt/intro.cpp

@@ -193,7 +193,7 @@ bool Intro::pickDataDirectory()
             }
             dataDir = intro.getDataDirectory();
             try {
-                TryCreateDirectory(GUIUtil::qstringToBoostPath(dataDir));
+                TryCreateDirectories(GUIUtil::qstringToBoostPath(dataDir));
                 break;
             } catch (const fs::filesystem_error& e) {
                 QMessageBox::critical(0, tr("PIVX Core"),

src/qt/pivx.cpp

@@ -600,7 +600,7 @@ int main(int argc, char* argv[])
     if (!Intro::pickDataDirectory())
         return 0;
 
-    /// 6. Determine availability of data directory and parse pivx.conf
+    /// 6. Determine availability of data and blocks directory and parse pivx.conf
     /// - Do not call GetDataDir(true) before this step finishes
     if (!fs::is_directory(GetDataDir(false))) {
         QMessageBox::critical(0, QObject::tr("PIVX Core"),

src/test/test_pivx.cpp

@@ -38,13 +38,13 @@ std::ostream& operator<<(std::ostream& os, const uint256& num)
     return os;
 }
 
-BasicTestingSetup::BasicTestingSetup()
+BasicTestingSetup::BasicTestingSetup(const std::string& chainName)
 {
         ECC_Start();
         SetupEnvironment();
         InitSignatureCache();
         fCheckBlockIndex = true;
-        SelectParams(CBaseChainParams::MAIN);
+        SelectParams(chainName);
         evoDb.reset(new CEvoDB(1 << 20, true, true));
         deterministicMNManager.reset(new CDeterministicMNManager(*evoDb));
 }
@@ -56,10 +56,10 @@ BasicTestingSetup::~BasicTestingSetup()
         evoDb.reset();
 }
 
-TestingSetup::TestingSetup()
+TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(chainName)
 {
         ClearDatadirCache();
-        pathTemp = GetTempPath() / strprintf("test_pivx_%lu_%i", (unsigned long)GetTime(), (int)(InsecureRandRange(100000)));
+        pathTemp = fs::temp_directory_path() / strprintf("test_pivx_%lu_%i", (unsigned long)GetTime(), (int)(InsecureRandRange(100000)));
         fs::create_directories(pathTemp);
         gArgs.ForceSetArg("-datadir", pathTemp.string());
 
@@ -113,10 +113,9 @@ TestingSetup::~TestingSetup()
         fs::remove_all(pathTemp);
 }
 
-TestChainSetup::TestChainSetup(int blockCount)
+// Test chain only available on regtest
+TestChainSetup::TestChainSetup(int blockCount) : TestingSetup(CBaseChainParams::REGTEST)
 {
-    SelectParams(CBaseChainParams::REGTEST);
-
     // if blockCount is over PoS start, delay it to 100 blocks after.
     if (blockCount > Params().GetConsensus().vUpgrades[Consensus::UPGRADE_POS].nActivationHeight) {
         UpdateNetworkUpgradeParameters(Consensus::UPGRADE_POS, blockCount + 100);

src/test/test_pivx.h

@@ -34,7 +34,9 @@ static inline std::vector<unsigned char> InsecureRandBytes(size_t len) { return
  * This just configures logging and chain parameters.
  */
 struct BasicTestingSetup {
-    BasicTestingSetup();
+    ECCVerifyHandle globalVerifyHandle;
+
+    BasicTestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
     ~BasicTestingSetup();
 };
 
@@ -49,16 +51,16 @@ struct TestingSetup: public BasicTestingSetup {
     boost::thread_group threadGroup;
     CConnman* connman;
     CScheduler scheduler;
-    ECCVerifyHandle globalVerifyHandle;
 
-    TestingSetup();
+    TestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
     ~TestingSetup();
 };
 
 class CBlock;
 struct CMutableTransaction;
 class CScript;
 
+// Test chain only available on regtest
 struct TestChainSetup : public TestingSetup
 {
     TestChainSetup(int blockCount);

src/test/validation_block_tests.cpp

@@ -15,9 +15,7 @@
 #include "validationinterface.h"
 
 struct RegtestingSetup : public TestingSetup {
-    RegtestingSetup() : TestingSetup() {
-        SelectParams(CBaseChainParams::REGTEST);
-    }
+    RegtestingSetup() : TestingSetup(CBaseChainParams::REGTEST) {}
 };
 
 BOOST_FIXTURE_TEST_SUITE(validation_block_tests, RegtestingSetup)

src/util.cpp

@@ -494,7 +494,7 @@ fs::path GetDefaultDataDir()
 #ifdef MAC_OSX
     // Mac
     pathRet /= "Library/Application Support";
-    TryCreateDirectory(pathRet);
+    TryCreateDirectories(pathRet);
     return pathRet / "PIVX";
 #else
     // Unix
@@ -503,6 +503,7 @@ fs::path GetDefaultDataDir()
 #endif
 }
 
+static fs::path g_blocks_path_cache_net_specific;
 static fs::path pathCached;
 static fs::path pathCachedNetSpecific;
 static fs::path zc_paramsPathCached;
@@ -528,7 +529,7 @@ static fs::path ZC_GetBaseParamsDir()
 #ifdef MAC_OSX
     // Mac
     pathRet /= "Library/Application Support";
-    TryCreateDirectory(pathRet);
+    TryCreateDirectories(pathRet);
     return pathRet / "PIVXParams";
 #else
     // Unix
@@ -637,6 +638,34 @@ void initZKSNARKS()
     //std::cout << "### Sapling params initialized ###" << std::endl;
 }
 
+const fs::path &GetBlocksDir()
+{
+
+    LOCK(csPathCached);
+
+    fs::path &path = g_blocks_path_cache_net_specific;
+
+    // This can be called during exceptions by LogPrintf(), so we cache the
+    // value so we don't have to do memory allocations after that.
+    if (!path.empty())
+        return path;
+
+    if (gArgs.IsArgSet("-blocksdir")) {
+        path = fs::system_complete(gArgs.GetArg("-blocksdir", ""));
+        if (!fs::is_directory(path)) {
+            path = "";
+            return path;
+        }
+    } else {
+        path = GetDataDir(false);
+    }
+
+    path /= BaseParams().DataDir();
+    path /= "blocks";
+    fs::create_directories(path);
+    return path;
+}
+
 const fs::path& GetDataDir(bool fNetSpecific)
 {
     LOCK(csPathCached);
@@ -671,6 +700,7 @@ void ClearDatadirCache()
 
     pathCached = fs::path();
     pathCachedNetSpecific = fs::path();
+    g_blocks_path_cache_net_specific = fs::path();
 }
 
 fs::path GetConfigFile(const std::string& confPath)
@@ -807,20 +837,20 @@ bool RenameOver(fs::path src, fs::path dest)
 }
 
 /**
- * Ignores exceptions thrown by Boost's create_directory if the requested directory exists.
+ * Ignores exceptions thrown by Boost's create_directories if the requested directory exists.
  * Specifically handles case where path p exists, but it wasn't possible for the user to
  * write to the parent directory.
  */
-bool TryCreateDirectory(const fs::path& p)
+bool TryCreateDirectories(const fs::path& p)
 {
     try {
-        return fs::create_directory(p);
+        return fs::create_directories(p);
     } catch (const fs::filesystem_error&) {
         if (!fs::exists(p) || !fs::is_directory(p))
             throw;
     }
 
-    // create_directory didn't create the directory, it had to have existed already
+    // create_directories didn't create the directory, it had to have existed already
     return false;
 }
 
@@ -951,11 +981,6 @@ fs::path GetSpecialFolderPath(int nFolder, bool fCreate)
 }
 #endif
 
-fs::path GetTempPath()
-{
-    return fs::temp_directory_path();
-}
-
 void runCommand(std::string strCommand)
 {
     if (strCommand.empty()) return;

src/util.h

@@ -87,8 +87,10 @@ bool TruncateFile(FILE* file, unsigned int length);
 int RaiseFileDescriptorLimit(int nMinFD);
 void AllocateFileRange(FILE* file, unsigned int offset, unsigned int length);
 bool RenameOver(fs::path src, fs::path dest);
-bool TryCreateDirectory(const fs::path& p);
+bool TryCreateDirectories(const fs::path& p);
 fs::path GetDefaultDataDir();
+// The blocks directory is always net specific.
+const fs::path &GetBlocksDir();
 const fs::path &GetDataDir(bool fNetSpecific = true);
 // Sapling network dir
 const fs::path &ZC_GetParamsDir();
@@ -104,7 +106,6 @@ void CreatePidFile(const fs::path& path, pid_t pid);
 #ifdef WIN32
 fs::path GetSpecialFolderPath(int nFolder, bool fCreate = true);
 #endif
-fs::path GetTempPath();
 
 void runCommand(std::string strCommand);
 

src/validation.cpp

@@ -1861,7 +1861,7 @@ bool static FlushStateToDisk(CValidationState& state, FlushStateMode mode)
         // Write blocks and block index to disk.
         if (fDoFullFlush || fPeriodicWrite) {
             // Depend on nMinDiskSpace to ensure we can write block index
-            if (!CheckDiskSpace(0))
+            if (!CheckDiskSpace(0, true))
                 return state.Error("out of disk space");
             // First make sure all block and undo data is flushed to disk.
             FlushBlockFile();
@@ -2665,7 +2665,7 @@ bool FindBlockPos(CValidationState& state, CDiskBlockPos& pos, unsigned int nAdd
         unsigned int nOldChunks = (pos.nPos + BLOCKFILE_CHUNK_SIZE - 1) / BLOCKFILE_CHUNK_SIZE;
         unsigned int nNewChunks = (vinfoBlockFile[nFile].nSize + BLOCKFILE_CHUNK_SIZE - 1) / BLOCKFILE_CHUNK_SIZE;
         if (nNewChunks > nOldChunks) {
-            if (CheckDiskSpace(nNewChunks * BLOCKFILE_CHUNK_SIZE - pos.nPos)) {
+            if (CheckDiskSpace(nNewChunks * BLOCKFILE_CHUNK_SIZE - pos.nPos, true)) {
                 FILE* file = OpenBlockFile(pos);
                 if (file) {
                     LogPrintf("Pre-allocating up to position 0x%x in blk%05u.dat\n", nNewChunks * BLOCKFILE_CHUNK_SIZE, pos.nFile);
@@ -2695,7 +2695,7 @@ bool FindUndoPos(CValidationState& state, int nFile, CDiskBlockPos& pos, unsigne
     unsigned int nOldChunks = (pos.nPos + UNDOFILE_CHUNK_SIZE - 1) / UNDOFILE_CHUNK_SIZE;
     unsigned int nNewChunks = (nNewSize + UNDOFILE_CHUNK_SIZE - 1) / UNDOFILE_CHUNK_SIZE;
     if (nNewChunks > nOldChunks) {
-        if (CheckDiskSpace(nNewChunks * UNDOFILE_CHUNK_SIZE - pos.nPos)) {
+        if (CheckDiskSpace(nNewChunks * UNDOFILE_CHUNK_SIZE - pos.nPos, true)) {
             FILE* file = OpenUndoFile(pos);
             if (file) {
                 LogPrintf("Pre-allocating up to position 0x%x in rev%05u.dat\n", nNewChunks * UNDOFILE_CHUNK_SIZE, pos.nFile);
@@ -3462,9 +3462,9 @@ bool TestBlockValidity(CValidationState& state, const CBlock& block, CBlockIndex
     return true;
 }
 
-bool CheckDiskSpace(uint64_t nAdditionalBytes)
+bool CheckDiskSpace(uint64_t nAdditionalBytes, bool blocks_dir)
 {
-    uint64_t nFreeBytesAvailable = fs::space(GetDataDir()).available;
+    uint64_t nFreeBytesAvailable = fs::space(blocks_dir ? GetBlocksDir() : GetDataDir()).available;
 
     // Check for nMinDiskSpace bytes (currently 50MB)
     if (nFreeBytesAvailable < nMinDiskSpace + nAdditionalBytes)
@@ -3508,7 +3508,7 @@ FILE* OpenUndoFile(const CDiskBlockPos& pos, bool fReadOnly)
 
 fs::path GetBlockPosFilename(const CDiskBlockPos& pos, const char* prefix)
 {
-    return GetDataDir() / "blocks" / strprintf("%s%05u.dat", prefix, pos.nFile);
+    return GetBlocksDir() / strprintf("%s%05u.dat", prefix, pos.nFile);
 }
 
 CBlockIndex* InsertBlockIndex(uint256 hash)

src/validation.h

@@ -175,7 +175,7 @@ static const uint64_t nMinDiskSpace = 52428800;
  */
 bool ProcessNewBlock(CValidationState& state, CNode* pfrom, const std::shared_ptr<const CBlock> pblock, CDiskBlockPos* dbp, bool* fAccepted = nullptr);
 /** Check whether enough disk space is available for an incoming block */
-bool CheckDiskSpace(uint64_t nAdditionalBytes = 0);
+bool CheckDiskSpace(uint64_t nAdditionalBytes = 0, bool blocks_dir = false);
 /** Open a block file (blk?????.dat) */
 FILE* OpenBlockFile(const CDiskBlockPos& pos, bool fReadOnly = false);
 /** Open an undo file (rev?????.dat) */

src/wallet/db.cpp

@@ -76,7 +76,7 @@ bool CDBEnv::Open(const fs::path& pathIn)
 
     strPath = pathIn.string();
     fs::path pathLogDir = pathIn / "database";
-    TryCreateDirectory(pathLogDir);
+    TryCreateDirectories(pathLogDir);
     fs::path pathErrorFile = pathIn / "db.log";
     LogPrintf("CDBEnv::Open: LogDir=%s ErrorFile=%s\n", pathLogDir.string(), pathErrorFile.string());