Refactor block file logic #15118

jimpo commented at 6:32 AM on January 7, 2019: contributor

This cleans up and refactors block file helpers so that they may be used by the block filter indexer. Per design discussion about storing BIP 157 block filters, it has been suggested that they are stored in the same way as block and undo data. This refactor is sufficient to simplify file operations for this use case, though in the future perhaps more pruning-related logic ought to be moved into the new classes.

The basic abstraction is a FlatFileSeq which manages access to a sequence of numbered files into which raw data is written.

jimpo renamed this:
~~Flatfile~~
Refactor block file logic
on Jan 7, 2019

fanquake added the label Refactoring on Jan 7, 2019

in src/init.cpp:1634 in 9ab64b2276 outdated

1630 | @@ -1631,8 +1631,8 @@ bool AppInitMain(InitInterfaces& interfaces)
1631 |  
1632 |      // ********************************************************* Step 11: import blocks
1633 |  
1634 | -    if (!CheckDiskSpace() && !CheckDiskSpace(0, true))
1635 | -        return false;
1636 | +    if (!CheckDiskSpace(GetDataDir()) && !CheckDiskSpace(GetBlocksDir()))

Empact commented at 7:04 AM on January 7, 2019:

Should this not be ||? /cc @jonasschnelli 386a6b62a8a1db9dd0f354cb95b7585f555c7e5d

jimpo commented at 5:01 PM on January 7, 2019:

Yeah, seems like it to me.

Empact commented at 6:58 PM on January 7, 2019:

#15124

in src/util/system.cpp:82 in 2af8b710b3 outdated

  77 | @@ -78,6 +78,9 @@
  78 |  #include <openssl/conf.h>
  79 |  #include <thread>
  80 |  
  81 | +/** Minimum disk space required - used in CheckDiskSpace() */
  82 | +static constexpr uint64_t nMinDiskSpace = 52428800;

Empact commented at 7:10 AM on January 7, 2019:

nit: could be static within the function

jimpo force-pushed on Jan 7, 2019

in src/flatfile.h:20 in 88cf78032c outdated

  15 | +class FlatFileSeq
  16 | +{
  17 | +private:
  18 | +    fs::path m_dir;
  19 | +    const char* m_prefix;
  20 | +    size_t m_chunk_size;

Empact commented at 7:22 AM on January 7, 2019:

nit: These could each be const. #include <cstddef> for size_t

promag commented at 9:31 AM on January 7, 2019:

Commit 88cf780

Agree with const members.

in src/flatfile.h:93 in d48754408b outdated

  49 | +    /**
  50 | +     * Commit a file to disk, and optionally truncate off extra pre-allocated bytes if final.
  51 | +     *
  52 | +     * @param[in] pos The first unwritten position in the file to be flushed.
  53 | +     * @param[in] finalize True if no more data will be written to this file.
  54 | +     * @return True on success, false on failure.

Empact commented at 7:34 AM on January 7, 2019:

nit: true

kallewoof commented at 7:33 AM on January 30, 2019:

Start of sentence True is fine IMO.

in src/validation.cpp:1638 in d48754408b outdated

1652 | +    CDiskBlockPos block_pos_old(nLastBlockFile, vinfoBlockFile[nLastBlockFile].nSize);
1653 | +    CDiskBlockPos undo_pos_old(nLastBlockFile, vinfoBlockFile[nLastBlockFile].nUndoSize);
1654 |  
1655 | +    bool status = true;
1656 | +    status &= BlockFileSeq().Flush(block_pos_old, fFinalize);
1657 | +    status &= UndoFileSeq().Flush(undo_pos_old, fFinalize);

Empact commented at 7:40 AM on January 7, 2019:

nit: this adds an extra fseek via Open each.

jimpo commented at 5:07 PM on January 7, 2019:

Good catch

in src/flatfile.h:45 in 13583e8a5e outdated

  42 | +    }
  43 | +
  44 | +    void SetNull() { nFile = -1; nPos = 0; }
  45 | +    bool IsNull() const { return (nFile == -1); }
  46 | +
  47 | +    std::string ToString() const;

Empact commented at 7:47 AM on January 7, 2019:

#include <string>

jimpo commented at 5:07 PM on January 7, 2019:

It's included.

Empact commented at 7:48 AM on January 7, 2019: member

Concept ACK hooray objects!

in src/validation.cpp:2138 in 2af8b710b3 outdated

2134 | @@ -2135,8 +2135,10 @@ bool static FlushStateToDisk(const CChainParams& chainparams, CValidationState &
2135 |          // Write blocks and block index to disk.
2136 |          if (fDoFullFlush || fPeriodicWrite) {
2137 |              // Depend on nMinDiskSpace to ensure we can write block index
2138 | -            if (!CheckDiskSpace(0, true))
2139 | +            if (!CheckDiskSpace(GetBlocksDir(), 0)) {

promag commented at 9:21 AM on January 7, 2019:

Commit 2af8b71

Remove 2nd arg, it's the default?

in src/validation.cpp:3030 in 2af8b710b3 outdated

3027 | @@ -3024,7 +3028,7 @@ static bool FindBlockPos(CDiskBlockPos &pos, unsigned int nAddSize, unsigned int
3028 |                  }
3029 |              }
3030 |              else

promag commented at 9:27 AM on January 7, 2019:

Commit 2af8b71

Care add {}?

jimpo commented at 5:12 PM on January 7, 2019:

I added the braces after the code is moved to FlatFileSeq::Allocate.

in src/flatfile.cpp:17 in 88cf78032c outdated

   0 | @@ -0,0 +1,15 @@
   1 | +// Copyright (c) 2019 The Bitcoin Core developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#include <flatfile.h>
   6 | +#include <tinyformat.h>
   7 | +
   8 | +FlatFileSeq::FlatFileSeq(fs::path dir, const char* prefix, size_t chunk_size)
   9 | +    : m_dir(std::move(dir)), m_prefix(prefix), m_chunk_size(chunk_size)

promag commented at 9:30 AM on January 7, 2019:

Commit 88cf780

nit, initialize one per line?

jimpo commented at 5:12 PM on January 7, 2019:

I don't see this in the style guide?

promag commented at 10:57 PM on January 7, 2019:

You also don't see to declare class members in multiple lines, but fair enough.

kallewoof commented at 7:29 AM on January 30, 2019:

+1 on one per line as it makes diffs prettier when adding/removing stuff, but I'm fine as it is too, since it's only 3 vars.

in src/flatfile.h:8 in 88cf78032c outdated

   0 | @@ -0,0 +1,36 @@
   1 | +// Copyright (c) 2019 The Bitcoin Core developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#ifndef BITCOIN_FLATFILE_H
   6 | +#define BITCOIN_FLATFILE_H
   7 | +
   8 | +#include <chain.h>

promag commented at 9:33 AM on January 7, 2019:

Commit 88cf780

Prefer forward declaration of CDiskBlockPos?

jimpo commented at 5:13 PM on January 7, 2019:

Good point. Though CDiskBlockPos is moved into this header later on, so I don't think it's necessary to change in the earlier commit.

in src/validation.cpp:3809 in 88cf78032c outdated

3814 | @@ -3802,9 +3815,9 @@ static FILE* OpenUndoFile(const CDiskBlockPos &pos, bool fReadOnly) {
3815 |      return OpenDiskFile(pos, "rev", fReadOnly);
3816 |  }
3817 |  
3818 | -fs::path GetBlockPosFilename(const CDiskBlockPos &pos, const char *prefix)

promag commented at 9:36 AM on January 7, 2019:

Commit 88cf780

nit, remove?

jimpo commented at 5:13 PM on January 7, 2019:

It's used once in init.cpp.

promag commented at 5:29 PM on January 7, 2019:

I know, just inline where needed?

Empact commented at 6:54 PM on January 7, 2019:

That would mean require BlockFileSeq, which is currently static and a broader interface.

promag commented at 11:17 PM on January 7, 2019:

Right, just realized it's a static function in validation.cpp

in src/flatfile.cpp:18 in ad59122358 outdated

  13 | @@ -13,3 +14,26 @@ fs::path FlatFileSeq::FileName(const CDiskBlockPos& pos) const
  14 |  {
  15 |      return m_dir / strprintf("%s%05u.dat", m_prefix, pos.nFile);
  16 |  }
  17 | +
  18 | +FILE* FlatFileSeq::Open(const CDiskBlockPos& pos, bool fReadOnly)

promag commented at 9:41 AM on January 7, 2019:

Commit ad59122

Just asking for a follow up, could this be changed to std::fstream?

jimpo commented at 5:15 PM on January 7, 2019:

Maybe... but a lot of file utilities like FileCommit, AllocateFileRange, etc would have to be modified as well. I think the better approach would be to return a CAutoFile and define the move constructor/assignment operator properly. And probably move it to a different module, like in util.

promag commented at 9:44 AM on January 7, 2019: member

Concept ACK.

Code review up to ad59122.

in src/validation.cpp:323 in 88cf78032c outdated

 310 | @@ -310,6 +311,8 @@ static void FindFilesToPruneManual(std::set<int>& setFilesToPrune, int nManualPr
 311 |  static void FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPruneAfterHeight);
 312 |  bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, bool fScriptChecks, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks = nullptr);
 313 |  static FILE* OpenUndoFile(const CDiskBlockPos &pos, bool fReadOnly = false);
 314 | +static FlatFileSeq BlockFileSeq();
 315 | +static FlatFileSeq UndoFileSeq();

jamesob commented at 5:37 PM on January 8, 2019:

Any reason not to just have these on-hand as global objects instead of having to reconstruct an object for each use through these functions? Obviously there isn't much of a performance concern given these calls happen before disk IO, but I'm just curious.

jimpo commented at 8:03 PM on January 8, 2019:

They are constructed with GetDataDir/GetBlocksDir which are not static, so the initialization logic would get a bit tricky. Could be OK with lazy evaluation or something, but didn't seem worth it. Open to suggestions.

jamesob commented at 8:18 PM on January 8, 2019:

Ah, okay. Like I said, no big deal AFAICT.

in src/validation.cpp:3021 in 930e283954 outdated

3031 | -                }
3032 | -            }
3033 | -            else
3034 | -                return AbortNode("Disk space is low!", _("Error: Disk space is low!"));
3035 | +        bool out_of_space;
3036 | +        size_t bytes_allocated = UndoFileSeq().Allocate(pos, nAddSize, out_of_space);

jamesob commented at 6:14 PM on January 8, 2019:

This should be BlockFileSeq and not UndoFileSeq, no?

jimpo commented at 8:04 PM on January 8, 2019:

Oops, good catch.

in src/test/flatfile_tests.cpp:34 in 13583e8a5e outdated

  29 | +
  30 | +    std::string line1("A purely peer-to-peer version of electronic cash would allow online "
  31 | +                      "payments to be sent directly from one party to another without going "
  32 | +                      "through a financial institution.");
  33 | +    std::string line2("Digital signatures provide part of the solution, but the main benefits are "
  34 | +                      "lost if a trusted third party is still required to prevent double-spending.");

jamesob commented at 6:48 PM on January 8, 2019:

Cute.

jamesob commented at 7:28 PM on January 8, 2019: member

All commits so far reviewed (up to 697d4c3fc). Great change aside from the bug I've noted above. This does a nice job of cleaning up and deduplicating flatfile data access code, and adds some test coverage. When taking into account the use for BIP157 filter storage (forthcoming), this is a very valuable patch.

jimpo force-pushed on Jan 8, 2019

jimpo force-pushed on Jan 9, 2019

in src/flatfile.h:73 in fbbf962422 outdated

  70 | +
  71 | +    /** Get the name of the file at the given position. */
  72 | +    fs::path FileName(const FlatFilePos& pos) const;
  73 | +
  74 | +    /** Open a handle to the file at the given position. */
  75 | +    FILE* Open(const FlatFilePos& pos, bool read_only = false);

practicalswift commented at 8:44 PM on January 9, 2019:

Make sure parameter names match between declaration and definition :-)

in src/flatfile.cpp:68 in fbbf962422 outdated

  63 | +                AllocateFileRange(file, pos.nPos, inc_size);
  64 | +                fclose(file);
  65 | +                return inc_size;
  66 | +            }
  67 | +        }
  68 | +        else {

practicalswift commented at 8:45 PM on January 9, 2019:

Move else { one line up :-)

DrahtBot commented at 10:48 PM on January 9, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15340 (gui: Introduce bilingual GUI error messages by hebasto)
#13868 (Remove unused fScriptChecks parameter from CheckInputs by Empact)
#13746 (-masterdatadir for datadir bootstrapping by kallewoof)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/flatfile.h:34 in fbbf962422 outdated

  29 | +    }
  30 | +
  31 | +    FlatFilePos(int nFileIn, unsigned int nPosIn) {
  32 | +        nFile = nFileIn;
  33 | +        nPos = nPosIn;
  34 | +    }

jamesob commented at 3:36 PM on January 24, 2019:

I know this is kind of a move, but worth using an initializer list?

laanwj added this to the "Blockers" column in a project

jimpo force-pushed on Jan 24, 2019

jamesob approved

jamesob commented at 4:28 PM on January 25, 2019: member

tACK https://github.com/bitcoin/bitcoin/commit/9b2d9ee0ef221668b834ffe9668b9a15297bc93f, though the last two commits are in need of fixup.

I read over the code once more since the last rebase and feedback commits. To test, I ran a reindex using this branch on a datadir with a preexisting chain of height 522,000 that had been built without these changes. The reindex completed successfully. I started a recent version of master (12b30105fc) and saw that block verification succeeded and startup ran as normal.

I'll reACK after squash.

in src/flatfile.cpp:2 in 9b2d9ee0ef outdated

   0 | @@ -0,0 +1,91 @@
   1 | +// Copyright (c) 2009-2010 Satoshi Nakamoto
   2 | +// Copyright (c) 2009-2019 The Bitcoin Core developers

kallewoof commented at 7:27 AM on January 30, 2019:

Copyright looked off to me initially, but I guess if you move code, the copyright stays the same. (I was expecting "2019" only).

in src/flatfile.cpp:36 in 9b2d9ee0ef outdated

  23 | +}
  24 | +
  25 | +FILE* FlatFileSeq::Open(const FlatFilePos& pos, bool read_only)
  26 | +{
  27 | +    if (pos.IsNull())
  28 | +        return nullptr;

kallewoof commented at 7:35 AM on January 30, 2019:

This is a move I believe, so feel free to ignore: add {} or remove newline. (Also applies to code 4 lines down.)

jimpo commented at 7:40 AM on February 1, 2019:

Yeah, it's a move. Would rather not change.

in src/flatfile.cpp:39 in 9b2d9ee0ef outdated

  34 | +    if (!file) {
  35 | +        LogPrintf("Unable to open file %s\n", path.string());
  36 | +        return nullptr;
  37 | +    }
  38 | +    if (pos.nPos) {
  39 | +        if (fseek(file, pos.nPos, SEEK_SET)) {

kallewoof commented at 7:38 AM on January 30, 2019:

Can do single if here.

if (pos.nPos && fseek(file, pos.nPos, SEEK_SET)) {

in src/util/system.h:74 in 9b2d9ee0ef outdated

  70 | @@ -71,6 +71,7 @@ void AllocateFileRange(FILE *file, unsigned int offset, unsigned int length);
  71 |  bool RenameOver(fs::path src, fs::path dest);
  72 |  bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only=false);
  73 |  bool DirIsWritable(const fs::path& directory);
  74 | +bool CheckDiskSpace(const fs::path& dir, uint64_t nAdditionalBytes = 0);

kallewoof commented at 7:45 AM on January 30, 2019:

Nit/style: naming convention, additional_bytes?

jimpo commented at 7:34 AM on February 1, 2019:

This is just moving code out of validation.{h,cpp}. Would rather not also rename vars.

in src/test/flatfile_tests.cpp:103 in 9b2d9ee0ef outdated

  98 | +    BOOST_CHECK_EQUAL(fs::file_size(seq.FileName(FlatFilePos(0, 99))), 100);
  99 | +    BOOST_CHECK(!out_of_space);
 100 | +
 101 | +    BOOST_CHECK_EQUAL(seq.Allocate(FlatFilePos(0, 99), 2, out_of_space), 101);
 102 | +    BOOST_CHECK_EQUAL(fs::file_size(seq.FileName(FlatFilePos(0, 99))), 200);
 103 | +    BOOST_CHECK(!out_of_space);

kallewoof commented at 7:52 AM on January 30, 2019:

Would adding

    uint64_t too_big = 1024 * 1024 + fs::space(data_dir).available;
    BOOST_CHECK_EQUAL(seq.Allocate(FlatFilePos(0, 0), too_big, out_of_space), 0);
    BOOST_CHECK_EQUAL(fs::file_size(seq.FileName(FlatFilePos(0, 0))), 200);
    BOOST_CHECK(out_of_space);

be overkill?

jimpo commented at 7:40 AM on February 1, 2019:

Good idea.

jimpo commented at 11:07 PM on February 1, 2019:

I couldn't get this test passing on Travis... tried setting too_big to a petabyte as well, and there was still some issue.

kallewoof commented at 9:42 AM on February 14, 2019:

The reason is that size_t is 32 bit (edit: on certain systems) and there is more than 2^32 bytes available on the system, resulting in an overflow. The Allocate method should probably take a uint64_t to avoid this.

in src/util/system.cpp:142 in 9b2d9ee0ef outdated

 134 | @@ -135,6 +135,13 @@ bool DirIsWritable(const fs::path& directory)
 135 |      return true;
 136 |  }
 137 |  
 138 | +bool CheckDiskSpace(const fs::path& dir, uint64_t nAdditionalBytes)
 139 | +{
 140 | +    constexpr uint64_t nMinDiskSpace = 52428800;
 141 | +    uint64_t nFreeBytesAvailable = fs::space(dir).available;
 142 | +    return nFreeBytesAvailable >= nMinDiskSpace + nAdditionalBytes;

kallewoof commented at 7:56 AM on January 30, 2019:

Nit/style: var_names

jimpo commented at 7:34 AM on February 1, 2019:

This is just moving code out of validation.{h,cpp}. Would rather not also rename vars.

in src/validation.cpp:3010 in 9b2d9ee0ef outdated

3031 | +        size_t bytes_allocated = BlockFileSeq().Allocate(pos, nAddSize, out_of_space);
3032 | +        if (out_of_space) {
3033 | +            return AbortNode("Disk space is low!", _("Error: Disk space is low!"));
3034 | +        }
3035 | +        if (bytes_allocated != 0 && fPruneMode) {
3036 | +            fCheckForPruning = true;

kallewoof commented at 8:04 AM on January 30, 2019:

Can also do

fCheckForPruning |= bytes_allocated != 0 && fPruneMode;

and remove the if stuff.

in src/validation.cpp:3034 in 9b2d9ee0ef outdated

3074 | +    if (out_of_space) {
3075 | +        AbortNode("Disk space is low!", _("Error: Disk space is low!"));
3076 | +        return state.Error("out of disk space");
3077 | +    }
3078 | +    if (bytes_allocated != 0 && fPruneMode) {
3079 | +        fCheckForPruning = true;

kallewoof commented at 8:05 AM on January 30, 2019:

Can get rid of if case here as described above.

kallewoof commented at 8:20 AM on January 30, 2019: member

utACK with some nits

in src/flatfile.cpp:18 in 9b2d9ee0ef outdated

  13 | +    return strprintf("FlatFilePos(nFile=%i, nPos=%i)", nFile, nPos);
  14 | +}
  15 | +
  16 | +FlatFileSeq::FlatFileSeq(fs::path dir, const char* prefix, size_t chunk_size)
  17 | +    : m_dir(std::move(dir)), m_prefix(prefix), m_chunk_size(chunk_size)
  18 | +{}

etscrivner commented at 8:11 PM on January 31, 2019:

Should this raise an error if chunk_size = 0?

in src/flatfile.cpp:55 in 9b2d9ee0ef outdated

  43 | +        }
  44 | +    }
  45 | +    return file;
  46 | +}
  47 | +
  48 | +size_t FlatFileSeq::Allocate(const FlatFilePos& pos, size_t add_size, bool& out_of_space)

etscrivner commented at 8:22 PM on January 31, 2019:

Should add_size = 0 be allowed? Seems like this would always cause n_new_chunks = n_old_chunks thereby causing the function to return 0.

jimpo commented at 8:11 AM on February 1, 2019:

Returning 0 seems like the right thing to do there. Probably better than throwing an exception or something.

in src/util/system.cpp:140 in 9b2d9ee0ef outdated

 134 | @@ -135,6 +135,13 @@ bool DirIsWritable(const fs::path& directory)
 135 |      return true;
 136 |  }
 137 |  
 138 | +bool CheckDiskSpace(const fs::path& dir, uint64_t nAdditionalBytes)
 139 | +{
 140 | +    constexpr uint64_t nMinDiskSpace = 52428800;

etscrivner commented at 8:35 PM on January 31, 2019:

It would be good to add an additional comment above this indicating its approximate size. Previously this comment was present:

// Check for nMinDiskSpace bytes (currently 50MB)

in src/flatfile.cpp:66 in 9b2d9ee0ef outdated

  54 | +    if (n_new_chunks > n_old_chunks) {
  55 | +        size_t old_size = pos.nPos;
  56 | +        size_t new_size = n_new_chunks * m_chunk_size;
  57 | +        size_t inc_size = new_size - old_size;
  58 | +
  59 | +        if (CheckDiskSpace(m_dir, inc_size)) {

etscrivner commented at 8:38 PM on January 31, 2019:

Any concerns about the type mismatch here between inc_size (size_t) and the second argument to CheckDiskSpace which is a uint64_t?

jimpo commented at 7:42 AM on February 1, 2019:

Do you get a compiler warning? If not, the implicit cast ought to be fine.

etscrivner commented at 7:56 PM on February 2, 2019:

No compiler warning, maybe just a note for the future that the sizes of the integer types should probably be reconciled at some point.

jimpo force-pushed on Feb 1, 2019

jimpo force-pushed on Feb 5, 2019

jimpo force-pushed on Feb 6, 2019

DrahtBot added the label Needs rebase on Feb 13, 2019

jimpo force-pushed on Feb 13, 2019

DrahtBot removed the label Needs rebase on Feb 13, 2019

in src/validation.cpp:3013 in 72d6932090 outdated

3015 | @@ -3016,21 +3016,13 @@ static bool FindBlockPos(CDiskBlockPos &pos, unsigned int nAddSize, unsigned int
3016 |          vinfoBlockFile[nFile].nSize += nAddSize;
3017 |  
3018 |      if (!fKnown) {
3019 | -        unsigned int nOldChunks = (pos.nPos + BLOCKFILE_CHUNK_SIZE - 1) / BLOCKFILE_CHUNK_SIZE;
3020 | -        unsigned int nNewChunks = (vinfoBlockFile[nFile].nSize + BLOCKFILE_CHUNK_SIZE - 1) / BLOCKFILE_CHUNK_SIZE;

ryanofsky commented at 9:10 PM on February 20, 2019:

In commit "validation: Refactor block file pre-allocation into FlatFileSeq." (72d693209018fc0b94d7875574686283936751e6)

Note (just for reference because I found this function confusing): In the new code this line is replaced with:

unsigned int n_new_chunks = (pos.nPos + add_size + m_chunk_size - 1) / m_chunk_size;

which is equivalent because vinfoBlockFile[nFile].nSize == pos.nPos + add_size in the !fKnown case due to:

pos.nPos = vinfoBlockFile[nFile].nSize;
...
vinfoBlockFile[nFile].nSize += nAddSize;

above

gwillen commented at 1:12 AM on February 22, 2019:

Probably the right move would be to add some comments in the new code, to reduce confusion to future readers?

jimpo commented at 2:47 AM on February 22, 2019:

I'm happy to add comments anywhere you think it would make the new code more clear, but I don't want to add comments because the old code was unnecessarily confusing and this is a slight change for anyone who happened to be familiar with it. I personally find the new Allocate function clear enough as is (including the n_new_chunks calculation), but of course I wrote it...

A PR comment like the one above seems like a fine way to address this sort of thing IMO.

in src/validation.cpp:3010 in 72d6932090 outdated

3035 | +        size_t bytes_allocated = BlockFileSeq().Allocate(pos, nAddSize, out_of_space);
3036 | +        if (out_of_space) {
3037 | +            return AbortNode("Disk space is low!", _("Error: Disk space is low!"));
3038 | +        }
3039 | +        if (bytes_allocated != 0 && fPruneMode) {
3040 | +            fCheckForPruning = true;

ryanofsky commented at 9:13 PM on February 20, 2019:

In commit "validation: Refactor block file pre-allocation into FlatFileSeq." (72d693209018fc0b94d7875574686283936751e6)

Probably doesn't make any difference, but previously the fCheckForPruning = true setting would happen before return AbortNode, but now it will be skipped if there is no disk space.

Same comment applies in FindUndoPos below.

in src/flatfile.cpp:91 in 5bc3ece663 outdated

  81 | +    }
  82 | +    if (finalize && !TruncateFile(file, pos.nPos)) {
  83 | +        fclose(file);
  84 | +        return error("%s: failed to truncate file %d", __func__, pos.nFile);
  85 | +    }
  86 | +    if (!FileCommit(file)) {

ryanofsky commented at 9:30 PM on February 20, 2019:

In commit "validation: Refactor file flush logic into FlatFileSeq." (5bc3ece663177235606e4ee272ff0de2bac1306a)

Probably ok, but previous code would still call FileCommit when TruncateFile failed, and now it is skipped.

ryanofsky approved

ryanofsky commented at 9:35 PM on February 20, 2019: member

utACK 2e476e9488616a3413e15fdc2281902b28e3beeb

in src/validation.cpp:2138 in 6c8f5d0094 outdated

2133 | @@ -2134,8 +2134,10 @@ bool static FlushStateToDisk(const CChainParams& chainparams, CValidationState &
2134 |          // Write blocks and block index to disk.
2135 |          if (fDoFullFlush || fPeriodicWrite) {
2136 |              // Depend on nMinDiskSpace to ensure we can write block index
2137 | -            if (!CheckDiskSpace(0, true))
2138 | +            if (!CheckDiskSpace(GetBlocksDir())) {
2139 | +                AbortNode("Disk space is low!", _("Error: Disk space is low!"));

gwillen commented at 1:18 AM on February 22, 2019:

Nit: There's an overload of AbortNode that takes a CValidationState for just what you're doing here, unless you specifically want the messages to be different (which would make sense if you want to preserve them exactly as they were before, although it doesn't look like you preserve them everywhere.)

in src/validation.cpp:2174 in 6c8f5d0094 outdated

2169 | @@ -2168,8 +2170,10 @@ bool static FlushStateToDisk(const CChainParams& chainparams, CValidationState &
2170 |              // twice (once in the log, and once in the tables). This is already
2171 |              // an overestimation, as most will delete an existing entry or
2172 |              // overwrite one. Still, use a conservative safety factor of 2.
2173 | -            if (!CheckDiskSpace(48 * 2 * 2 * pcoinsTip->GetCacheSize()))
2174 | +            if (!CheckDiskSpace(GetDataDir(), 48 * 2 * 2 * pcoinsTip->GetCacheSize())) {
2175 | +                AbortNode("Disk space is low!", _("Error: Disk space is low!"));

gwillen commented at 1:19 AM on February 22, 2019:

As above.

in src/flatfile.cpp:33 in 2e476e9488 outdated

  29 | @@ -30,25 +30,23 @@ fs::path FlatFileSeq::FileName(const FlatFilePos& pos) const
  30 |      return m_dir / strprintf("%s%05u.dat", m_prefix, pos.nFile);
  31 |  }
  32 |  
  33 | -FILE* FlatFileSeq::Open(const FlatFilePos& pos, bool fReadOnly)
  34 | +FILE* FlatFileSeq::Open(const FlatFilePos& pos, bool read_only)

gwillen commented at 6:53 AM on February 22, 2019:

Nit: Braces on the "if" below, since you're doing a style cleanup pass?

in src/validation.cpp:3063 in 6c8f5d0094 outdated

3060 |                  fclose(file);
3061 |              }
3062 |          }
3063 | -        else
3064 | +        else {
3065 | +            AbortNode("Disk space is low!", _("Error: Disk space is low!"));

gwillen commented at 6:55 AM on February 22, 2019:

As above.

gwillen commented at 7:06 AM on February 22, 2019: contributor

utACK 2e476e9, all comments are nits.

jimpo force-pushed on Feb 22, 2019

util: Move CheckDiskSpace to util. 62e7addb63

validation: Extract basic block file logic into FlatFileSeq class. 9183d6ef65

validation: Refactor OpenDiskFile into method on FlatFileSeq. e2d2abb99f

validation: Refactor block file pre-allocation into FlatFileSeq. 992404b31e

validation: Refactor file flush logic into FlatFileSeq. e0380933e3

Move CDiskBlockPos from chain to flatfile. d6d8a78f26

scripted-diff: Rename CBlockDiskPos to FlatFilePos.

-BEGIN VERIFY SCRIPT-
sed -i 's/CDiskBlockPos/FlatFilePos/g' $(git ls-files 'src/*.h' 'src/*.cpp')
-END VERIFY SCRIPT-

65a489e93d

flatfile: Unit tests for FlatFileSeq methods. 4c01e4e159

Style cleanup. 04cca33094

jimpo force-pushed on Feb 23, 2019

laanwj commented at 10:21 PM on March 2, 2019: member

utACK 04cca330944f859b4ed68cb8da8a79f5206fd630

laanwj merged this on Mar 2, 2019

laanwj closed this on Mar 2, 2019

laanwj referenced this in commit 2d46f1be0c on Mar 2, 2019

fanquake removed this from the "Blockers" column in a project

jimpo deleted the branch on Mar 5, 2019

furszy referenced this in commit 6b95c76daf on May 15, 2021

kittywhiskers referenced this in commit 9bfa2b7851 on Aug 2, 2021

kittywhiskers referenced this in commit b1d481eb78 on Aug 2, 2021

kittywhiskers referenced this in commit d921f4fecf on Aug 8, 2021

UdjinM6 referenced this in commit db810bdb1f on Aug 11, 2021

kittywhiskers referenced this in commit 4d3e3d7b07 on Aug 12, 2021

UdjinM6 referenced this in commit 607bd4b6b5 on Aug 13, 2021

DrahtBot locked this on Dec 16, 2021