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

Code before:
    DisconnectedBlockTransactions& disconnectpool)
{
    AssertLockHeld(cs_main);
    if (m_mempool) AssertLockHeld(m_mempool->cs);

    assert(pindexNew->pprev == m_chain.Tip());
    // Read block from disk.
    const auto time_1{SteadyClock::now()};
    if (!block_to_connect) {
        std::shared_ptr<CBlock> pblockNew = std::make_shared<CBlock>();
        if (!m_blockman.ReadBlock(*pblockNew, *pindexNew)) {
            return FatalError(m_chainman.GetNotifications(), state, _("Failed to read block."));
        }
        block_to_connect = std::move(pblockNew);
    } else {
        LogDebug(BCLog::BENCH, "  - Using cached block\n");
    }
    // Apply the block atomically to the chain state.
    const auto time_2{SteadyClock::now()};
    SteadyClock::time_point time_3;
    // When adding aggregate statistics in the future, keep in mind that
    // num_blocks_total may be zero until the ConnectBlock() call below.
    LogDebug(BCLog::BENCH, "  - Load block from disk: %.2fms\n",
             Ticks<MillisecondsDouble>(time_2 - time_1));
    {
        auto& view{*m_coins_views->m_connect_block_view};
        bool rv = ConnectBlock(*block_to_connect, state, pindexNew, view);
        if (m_chainman.m_options.signals) {
            m_chainman.m_options.signals->BlockChecked(block_to_connect, state);
        }
        if (!rv) {
            if (state.IsInvalid())
                InvalidBlockFound(pindexNew, state);
            LogError("%s: ConnectBlock %s failed, %s\n", __func__, pindexNew->GetBlockHash().ToString(), state.ToString());
            view.Reset();
            return false;
        }
        time_3 = SteadyClock::now();
        m_chainman.time_connect_total += time_3 - time_2;
        assert(m_chainman.num_blocks_total > 0);
        LogDebug(BCLog::BENCH, "  - Connect total: %.2fms [%.2fs (%.2fms/blk)]\n",
                 Ticks<MillisecondsDouble>(time_3 - time_2),
                 Ticks<SecondsDouble>(m_chainman.time_connect_total),
                 Ticks<MillisecondsDouble>(m_chainman.time_connect_total) / m_chainman.num_blocks_total);
        bool flushed = view.Flush(/*will_reuse_cache=*/false); // No need to reallocate since it only has capacity for 1 block
        assert(flushed);
        view.Reset();
    }
    const auto time_4{SteadyClock::now()};
    m_chainman.time_flush += time_4 - time_3;
    LogDebug(BCLog::BENCH, "  - Flush: %.2fms [%.2fs (%.2fms/blk)]\n",

Review comment (performance):
This PR reuses the cache in `ConnectTip()` only, but we still construct short-lived `CCoinsViewCache` instances in `DisconnectTip()` and `TestBlockValidity()`:
* https://github.com/bitcoin/bitcoin/blob/adbb4b320834516dc97fa33aaa853a45cbabca67/src/validation.cpp#L2981
* https://github.com/bitcoin/bitcoin/blob/adbb4b320834516dc97fa33aaa853a45cbabca67/src/validation.cpp#L4593

We also have a few other ones that may not need any reusability (we might want to comment for those cases for why we chose single-use caches, since they're rare events and would inflate the cache with more than 1 block of data):
* https://github.com/bitcoin/bitcoin/blob/adbb4b320834516dc97fa33aaa853a45cbabca67/src/validation.cpp#L4700
* https://github.com/bitcoin/bitcoin/blob/adbb4b320834516dc97fa33aaa853a45cbabca67/src/validation.cpp#L4849

If the goal is avoiding repeated allocations, I think we should also consider reusing the persistent view in the related cases in this PR.

----

Even if we decide not to do it in this PR, it raises the question: should we assert that nobody is using the same cache at the same time we are, which would require us adding some sanity check after acquiring it to make sure we're using a properly reset cache, e.g. `assert(view.GetCacheSize() == 0)`.

Code after:
    if (m_mempool) AssertLockHeld(m_mempool->cs);

    assert(pindexNew->pprev == m_chain.Tip());
    // Read block from disk.
    const auto time_1{SteadyClock::now()};
    if (!block_to_connect) {
        std::shared_ptr<CBlock> pblockNew = std::make_shared<CBlock>();
        if (!m_blockman.ReadBlock(*pblockNew, *pindexNew)) {
            return FatalError(m_chainman.GetNotifications(), state, _("Failed to read block."));
        }
        block_to_connect = std::move(pblockNew);
    } else {
        LogDebug(BCLog::BENCH, "  - Using cached block\n");
    }
    // Apply the block atomically to the chain state.
    const auto time_2{SteadyClock::now()};
    SteadyClock::time_point time_3;
    // When adding aggregate statistics in the future, keep in mind that
    // num_blocks_total may be zero until the ConnectBlock() call below.
    LogDebug(BCLog::BENCH, "  - Load block from disk: %.2fms\n",
             Ticks<MillisecondsDouble>(time_2 - time_1));
    {
        CCoinsViewCache& view{*m_coins_views->m_connect_block_view};
        const auto reset_guard{view.CreateResetGuard()};
        bool rv = ConnectBlock(*block_to_connect, state, pindexNew, view);
        if (m_chainman.m_options.signals) {
            m_chainman.m_options.signals->BlockChecked(block_to_connect, state);
        }
        if (!rv) {
            if (state.IsInvalid())
                InvalidBlockFound(pindexNew, state);
            LogError("%s: ConnectBlock %s failed, %s\n", __func__, pindexNew->GetBlockHash().ToString(), state.ToString());
            return false;
        }
        time_3 = SteadyClock::now();
        m_chainman.time_connect_total += time_3 - time_2;
        assert(m_chainman.num_blocks_total > 0);
        LogDebug(BCLog::BENCH, "  - Connect total: %.2fms [%.2fs (%.2fms/blk)]\n",
                 Ticks<MillisecondsDouble>(time_3 - time_2),
                 Ticks<SecondsDouble>(m_chainman.time_connect_total),
                 Ticks<MillisecondsDouble>(m_chainman.time_connect_total) / m_chainman.num_blocks_total);
        view.Flush(/*reallocate_cache=*/false); // No need to reallocate since it only has capacity for 1 block
    }
    const auto time_4{SteadyClock::now()};
    m_chainman.time_flush += time_4 - time_3;
    LogDebug(BCLog::BENCH, "  - Flush: %.2fms [%.2fs (%.2fms/blk)]\n",
             Ticks<MillisecondsDouble>(time_4 - time_3),
             Ticks<SecondsDouble>(m_chainman.time_flush),
             Ticks<MillisecondsDouble>(m_chainman.time_flush) / m_chainman.num_blocks_total);
    // Write the chain state to disk, if necessary.
    if (!FlushStateToDisk(state, FlushStateMode::IF_NEEDED)) {