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

Code before:
    char result_flags;
    try {
        CTxOut output;
        output.nValue = modify_value;
        test.cache.AddCoin(OUTPOINT, Coin(std::move(output), 1, coinbase), coinbase);
        test.cache.SelfTest();
        GetCoinsMapEntry(test.cache.map(), result_value, result_flags);
    } catch (std::logic_error&) {
        result_value = FAIL;
        result_flags = NO_ENTRY;
    }

    BOOST_CHECK_EQUAL(result_value, expected_value);
    BOOST_CHECK_EQUAL(result_flags, expected_flags);
}

// Simple wrapper for CheckAddCoinBase function above that loops through
// different possible base_values, making sure each one gives the same results.
// This wrapper lets the coins_add test below be shorter and less repetitive,
// while still verifying that the CoinsViewCache::AddCoin implementation
// ignores base values.
template <typename... Args>
static void CheckAddCoin(Args&&... args)
{
    for (const CAmount base_value : {ABSENT, SPENT, VALUE1}) {
        // NOLINTNEXTLINE(bugprone-use-after-move)
        CheckAddCoinBase(base_value, std::forward<Args>(args)...);
    }
}

BOOST_AUTO_TEST_CASE(ccoins_add)
{
    /* Check AddCoin behavior, requesting a new coin from a cache view,
     * writing a modification to the coin, and then checking the resulting
     * entry in the cache after the modification. Verify behavior with the
     * AddCoin possible_overwrite argument set to false, and to true.
     *
     *           Cache   Write   Result  Cache        Result       possible_overwrite
     *           Value   Value   Value   Flags        Flags
     */
    CheckAddCoin(ABSENT, VALUE3, VALUE3, NO_ENTRY   , DIRTY|FRESH, false);
    CheckAddCoin(ABSENT, VALUE3, VALUE3, NO_ENTRY   , DIRTY      , true );
    CheckAddCoin(SPENT , VALUE3, VALUE3, 0          , DIRTY|FRESH, false);
    CheckAddCoin(SPENT , VALUE3, VALUE3, 0          , DIRTY      , true );
    CheckAddCoin(SPENT , VALUE3, VALUE3, FRESH      , DIRTY|FRESH, false);
    CheckAddCoin(SPENT , VALUE3, VALUE3, FRESH      , DIRTY|FRESH, true );
    CheckAddCoin(SPENT , VALUE3, VALUE3, DIRTY      , DIRTY      , false);
    CheckAddCoin(SPENT , VALUE3, VALUE3, DIRTY      , DIRTY      , true );
    CheckAddCoin(SPENT , VALUE3, VALUE3, DIRTY|FRESH, DIRTY|FRESH, false);
    CheckAddCoin(SPENT , VALUE3, VALUE3, DIRTY|FRESH, DIRTY|FRESH, true );
    CheckAddCoin(VALUE2, VALUE3, FAIL  , 0          , NO_ENTRY   , false);

Review comment (bug):
As mentioned [here](https://github.com/bitcoin/bitcoin/pull/31364#discussion_r1857161138), this change struck me as dangerous.

> It shouldn't be marked nolint, rather it should be fixed: drop the forward and pass by const Args&.

I understand the desire to turn on new checks and notate the false-positives, but the result should _always_ be better/safer/cleaner code.

In this case, the result would've been a latent bug explicitly marked notabug. And while a buggy test isn't the end of the world, the suggestion that the approach for recent similar PR's might have been "quiet the static analysis tool first, fix the bug later" struck me as so backwards that I had to wonder if any other actual bugs in more important code had been masked as well. Looking through recent/current PRs, I don't see any other examples of that (modulo the boost issue pointed out here).

Please prioritize correctness and consider what you're signaling to reviewers.

Code after:
     * writing a modification to the coin, and then checking the resulting
     * entry in the cache after the modification. Verify behavior with the
     * AddCoin coinbase argument set to false, and to true.
     *               Base        Cache               Write   Expected              Coinbase
     */
    for (auto base_value : {ABSENT, SPENT, VALUE1}) {
        CheckAddCoin(base_value, MISSING,            VALUE3, VALUE3_DIRTY_FRESH,   false);
        CheckAddCoin(base_value, MISSING,            VALUE3, VALUE3_DIRTY,         true );

        CheckAddCoin(base_value, SPENT_CLEAN,        VALUE3, VALUE3_DIRTY_FRESH,   false);
        CheckAddCoin(base_value, SPENT_CLEAN,        VALUE3, VALUE3_DIRTY,         true );
        CheckAddCoin(base_value, SPENT_FRESH,        VALUE3, VALUE3_DIRTY_FRESH,   false);
        CheckAddCoin(base_value, SPENT_FRESH,        VALUE3, VALUE3_DIRTY_FRESH,   true );
        CheckAddCoin(base_value, SPENT_DIRTY,        VALUE3, VALUE3_DIRTY,         false);
        CheckAddCoin(base_value, SPENT_DIRTY,        VALUE3, VALUE3_DIRTY,         true );
        CheckAddCoin(base_value, SPENT_DIRTY_FRESH,  VALUE3, VALUE3_DIRTY_FRESH,   false);
        CheckAddCoin(base_value, SPENT_DIRTY_FRESH,  VALUE3, VALUE3_DIRTY_FRESH,   true );

        CheckAddCoin(base_value, VALUE2_CLEAN,       VALUE3, EX_OVERWRITE_UNSPENT, false);
        CheckAddCoin(base_value, VALUE2_CLEAN,       VALUE3, VALUE3_DIRTY,         true );
        CheckAddCoin(base_value, VALUE2_FRESH,       VALUE3, EX_OVERWRITE_UNSPENT, false);
        CheckAddCoin(base_value, VALUE2_FRESH,       VALUE3, VALUE3_DIRTY_FRESH,   true );
        CheckAddCoin(base_value, VALUE2_DIRTY,       VALUE3, EX_OVERWRITE_UNSPENT, false);
        CheckAddCoin(base_value, VALUE2_DIRTY,       VALUE3, VALUE3_DIRTY,         true );
        CheckAddCoin(base_value, VALUE2_DIRTY_FRESH, VALUE3, EX_OVERWRITE_UNSPENT, false);
        CheckAddCoin(base_value, VALUE2_DIRTY_FRESH, VALUE3, VALUE3_DIRTY_FRESH,   true );
    }
}

static void CheckWriteCoins(const MaybeCoin& parent, const MaybeCoin& child, const CoinOrError& expected)
{
    SingleEntryCacheTest test{ABSENT, parent};
    auto write_coins{[&] { WriteCoinsViewEntry(test.cache, child); }};
    if (auto* expected_coin{std::get_if<MaybeCoin>(&expected)}) {
        write_coins();
        test.cache.SelfTest(/*sanity_check=*/false);
        BOOST_CHECK_EQUAL(GetCoinsMapEntry(test.cache.map()), *expected_coin);
    } else {
        BOOST_CHECK_EXCEPTION(write_coins(), std::logic_error, HasReason(std::get<std::string>(expected)));
    }
}

BOOST_AUTO_TEST_CASE(ccoins_write)
{
    /* Check BatchWrite behavior, flushing one entry from a child cache to a
     * parent cache, and checking the resulting entry in the parent cache
     * after the write.
     *              Parent              Child               Expected
     */
    CheckWriteCoins(MISSING,            MISSING,            MISSING            );
    CheckWriteCoins(MISSING,            SPENT_DIRTY,        SPENT_DIRTY        );