Repository: bitcoin/bitcoin
File: src/node/blockstorage.cpp (C++)

Code before:

AutoFile BlockManager::OpenBlockFile(const FlatFilePos& pos, bool fReadOnly) const
{
    return AutoFile{BlockFileSeq().Open(pos, fReadOnly)};
}

/** Open an undo file (rev?????.dat) */
AutoFile BlockManager::OpenUndoFile(const FlatFilePos& pos, bool fReadOnly) const
{
    return AutoFile{UndoFileSeq().Open(pos, fReadOnly)};
}

fs::path BlockManager::GetBlockPosFilename(const FlatFilePos& pos) const
{
    return BlockFileSeq().FileName(pos);
}

bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, uint64_t nTime, bool fKnown)
{
    LOCK(cs_LastBlockFile);

    const BlockfileType chain_type = BlockfileTypeForHeight(nHeight);

    if (!m_blockfile_cursors[chain_type]) {
        // If a snapshot is loaded during runtime, we may not have initialized this cursor yet.
        assert(chain_type == BlockfileType::ASSUMED);
        const auto new_cursor = BlockfileCursor{this->MaxBlockfileNum() + 1};
        m_blockfile_cursors[chain_type] = new_cursor;
        LogPrint(BCLog::BLOCKSTORAGE, "[%s] initializing blockfile cursor to %s\n", chain_type, new_cursor);
    }
    const int last_blockfile = m_blockfile_cursors[chain_type]->file_num;

    int nFile = fKnown ? pos.nFile : last_blockfile;
    if (static_cast<int>(m_blockfile_info.size()) <= nFile) {
        m_blockfile_info.resize(nFile + 1);
    }

    bool finalize_undo = false;
    if (!fKnown) {
        unsigned int max_blockfile_size{MAX_BLOCKFILE_SIZE};
        // Use smaller blockfiles in test-only -fastprune mode - but avoid
        // the possibility of having a block not fit into the block file.
        if (m_opts.fast_prune) {
            max_blockfile_size = 0x10000; // 64kiB
            if (nAddSize >= max_blockfile_size) {
                // dynamically adjust the blockfile size to be larger than the added size
                max_blockfile_size = nAddSize + 1;
            }
        }
        assert(nAddSize < max_blockfile_size);


Review comment (bug):
In commit "blockstorage: Add Assume for fKnown / snapshot chainstate" (d6b0bb6dd0d26f3e10386d1afbafd8d52a12b2c5)

I don't think changing this `if` statement is good.

Adding the Assume call above seems good, since it provides information about the context this code is called in and could potentially catch bugs if the code is run in an unanticipated state.

But It's less clear what benefit there is to adding the  `!fKnown &&` condition to this if statement. It just makes the logic more complicated without providing any extra explanation. And if outside code were changed such that `fKnown` was false while the cursor was null, undefined behavior would now happen on line 866 below.

I think this commit would be a clearer if it avoided changing any existing logic and instead just added two Assume statements:

```c++
// Check that chain type is NORMAL if fKnown is true, because fKnown is only
// true during reindexing, and reindexing deletes snapshot chainstates, so
// chain_type will not be SNAPSHOT. Also check that cursor exists, because
// the normal cursor should never be null.
if (fKnown) {
    Assume(chain_type == BlockfileType::NORMAL);
    Assume(m_blockfile_cursors[chain_type]);    
}
```

Code after:

AutoFile BlockManager::OpenBlockFile(const FlatFilePos& pos, bool fReadOnly) const
{
    return AutoFile{BlockFileSeq().Open(pos, fReadOnly)};
}

/** Open an undo file (rev?????.dat) */
AutoFile BlockManager::OpenUndoFile(const FlatFilePos& pos, bool fReadOnly) const
{
    return AutoFile{UndoFileSeq().Open(pos, fReadOnly)};
}

fs::path BlockManager::GetBlockPosFilename(const FlatFilePos& pos) const
{
    return BlockFileSeq().FileName(pos);
}

FlatFilePos BlockManager::FindNextBlockPos(unsigned int nAddSize, unsigned int nHeight, uint64_t nTime)
{
    LOCK(cs_LastBlockFile);

    const BlockfileType chain_type = BlockfileTypeForHeight(nHeight);

    if (!m_blockfile_cursors[chain_type]) {
        // If a snapshot is loaded during runtime, we may not have initialized this cursor yet.
        assert(chain_type == BlockfileType::ASSUMED);
        const auto new_cursor = BlockfileCursor{this->MaxBlockfileNum() + 1};
        m_blockfile_cursors[chain_type] = new_cursor;
        LogPrint(BCLog::BLOCKSTORAGE, "[%s] initializing blockfile cursor to %s\n", chain_type, new_cursor);
    }
    const int last_blockfile = m_blockfile_cursors[chain_type]->file_num;

    int nFile = last_blockfile;
    if (static_cast<int>(m_blockfile_info.size()) <= nFile) {
        m_blockfile_info.resize(nFile + 1);
    }

    bool finalize_undo = false;
    unsigned int max_blockfile_size{MAX_BLOCKFILE_SIZE};
    // Use smaller blockfiles in test-only -fastprune mode - but avoid
    // the possibility of having a block not fit into the block file.
    if (m_opts.fast_prune) {
        max_blockfile_size = 0x10000; // 64kiB
        if (nAddSize >= max_blockfile_size) {
            // dynamically adjust the blockfile size to be larger than the added size
            max_blockfile_size = nAddSize + 1;
        }
    }
    assert(nAddSize < max_blockfile_size);

    while (m_blockfile_info[nFile].nSize + nAddSize >= max_blockfile_size) {