ronantakizawa/codereview-bench huggingface dataset:
Repository: bitcoin/bitcoin
File: src/test/util/logging.cpp (C++)

Code before:
// Copyright (c) 2019-2021 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <test/util/logging.h>

#include <logging.h>
#include <noui.h>
#include <tinyformat.h>

#include <stdexcept>

DebugLogHelper::DebugLogHelper(std::string message, MatchFn match)
    : m_message{std::move(message)}, m_match(std::move(match))
{
    m_print_connection = LogInstance().PushBackCallback(
        [this](const std::string& s) {
            if (m_found) return;
            m_found = s.find(m_message) != std::string::npos && m_match(&s);
        });
    noui_test_redirect();
}

void DebugLogHelper::check_found()
{
    noui_reconnect();
    LogInstance().DeleteCallback(m_print_connection);
    if (!m_found && m_match(nullptr)) {
        throw std::runtime_error(strprintf("'%s' not found in debug log\n", m_message));
    }
}


Review comment (performance):
This parameter change is a separate concern. Please split it into its own commit to keep the risky behavior change easy to review.
Though personally, I'd pass both by value and move into the members to handle rvalues efficiently.

Code after:
// Copyright (c) 2019-2021 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <test/util/logging.h>

#include <logging.h>
#include <noui.h>
#include <tinyformat.h>

#include <cstdlib>
#include <iostream>

DebugLogHelper::DebugLogHelper(std::string message, MatchFn match)
    : m_message{std::move(message)}, m_match(std::move(match))
{
    m_print_connection = LogInstance().PushBackCallback(
        [this](const std::string& s) {
            if (m_found) return;
            m_found = s.find(m_message) != std::string::npos && m_match(&s);
        });
    noui_test_redirect();
}

DebugLogHelper::~DebugLogHelper()
{
    noui_reconnect();
    LogInstance().DeleteCallback(m_print_connection);
    if (!m_found && m_match(nullptr)) {
        tfm::format(std::cerr, "Fatal error: expected message not found in the debug log: '%s'\n", m_message);
        std::abort();
    }
}
Repository: bitcoin/bitcoin
File: src/test/util/logging.h (C/C++)

Code before:
// Copyright (c) 2019-2022 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#ifndef BITCOIN_TEST_UTIL_LOGGING_H
#define BITCOIN_TEST_UTIL_LOGGING_H

#include <util/macros.h>

#include <functional>
#include <list>
#include <string>

class DebugLogHelper
{
    const std::string m_message;
    bool m_found{false};
    std::list<std::function<void(const std::string&)>>::iterator m_print_connection;

    //! Custom match checking function.
    //!
    //! Invoked with pointers to lines containing matching strings, and with
    //! null if check_found() is called without any successful match.
    //!
    //! Can return true to enable default DebugLogHelper behavior of:
    //! (1) ending search after first successful match, and
    //! (2) raising an error in check_found if no match was found
    //! Can return false to do the opposite in either case.
    using MatchFn = std::function<bool(const std::string* line)>;
    MatchFn m_match;

    void check_found();

public:
    explicit DebugLogHelper(std::string message, MatchFn match = [](const std::string*){ return true; });
    ~DebugLogHelper() noexcept(false) { check_found(); }
};

#define ASSERT_DEBUG_LOG(message) DebugLogHelper UNIQUE_NAME(debugloghelper)(message)

#endif // BITCOIN_TEST_UTIL_LOGGING_H


Review comment (bug):
nit: we can simplify it a bit (+ pass by value mentioned earlier):
```suggestion
    explicit DebugLogHelper(std::string message, MatchFn match = [](auto){ return true; });
```

Code after:
    //! null if check_found() is called without any successful match.
    //!
    //! Can return true to enable default DebugLogHelper behavior of:
    //! (1) ending search after first successful match, and
    //! (2) raising an error in check_found if no match was found
    //! Can return false to do the opposite in either case.
    using MatchFn = std::function<bool(const std::string* line)>;

    explicit DebugLogHelper(std::string message, MatchFn match = [](const std::string*){ return true; });

    DebugLogHelper(const DebugLogHelper&) = delete;
    DebugLogHelper& operator=(const DebugLogHelper&) = delete;

    ~DebugLogHelper();

private:
    const std::string m_message;
    bool m_found{false};
    std::list<std::function<void(const std::string&)>>::iterator m_print_connection;
    MatchFn m_match;
};

#define ASSERT_DEBUG_LOG(message) DebugLogHelper UNIQUE_NAME(debugloghelper)(message)

#endif // BITCOIN_TEST_UTIL_LOGGING_H
Repository: bitcoin/bitcoin
File: src/test/util/logging.cpp (C++)

Code before:
// Copyright (c) 2019-2021 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <test/util/logging.h>

#include <logging.h>
#include <noui.h>
#include <tinyformat.h>

#include <stdexcept>

DebugLogHelper::DebugLogHelper(std::string message, MatchFn match)
    : m_message{std::move(message)}, m_match(std::move(match))
{
    m_print_connection = LogInstance().PushBackCallback(
        [this](const std::string& s) {
            if (m_found) return;
            m_found = s.find(m_message) != std::string::npos && m_match(&s);
        });
    noui_test_redirect();
}

void DebugLogHelper::check_found()
{
    noui_reconnect();
    LogInstance().DeleteCallback(m_print_connection);
    if (!m_found && m_match(nullptr)) {
        throw std::runtime_error(strprintf("'%s' not found in debug log\n", m_message));
    }
}


Review comment (performance):
The `message` parameter was optimized to `const std::string&`, but `match` still uses pass-by-value. For consistency, shouldn't `match` also be `const MatchFn&`?

Code after:
// Copyright (c) 2019-2021 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <test/util/logging.h>

#include <logging.h>
#include <noui.h>
#include <tinyformat.h>

#include <cstdlib>
#include <iostream>

DebugLogHelper::DebugLogHelper(std::string message, MatchFn match)
    : m_message{std::move(message)}, m_match(std::move(match))
{
    m_print_connection = LogInstance().PushBackCallback(
        [this](const std::string& s) {
            if (m_found) return;
            m_found = s.find(m_message) != std::string::npos && m_match(&s);
        });
    noui_test_redirect();
}

DebugLogHelper::~DebugLogHelper()
{
    noui_reconnect();
    LogInstance().DeleteCallback(m_print_connection);
    if (!m_found && m_match(nullptr)) {
        tfm::format(std::cerr, "Fatal error: expected message not found in the debug log: '%s'\n", m_message);
        std::abort();
    }
}
Repository: bitcoin/bitcoin
File: src/test/util/logging.cpp (C++)

Code before:
// Copyright (c) 2019-2021 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <test/util/logging.h>

#include <logging.h>
#include <noui.h>
#include <tinyformat.h>

#include <stdexcept>

DebugLogHelper::DebugLogHelper(std::string message, MatchFn match)
    : m_message{std::move(message)}, m_match(std::move(match))
{
    m_print_connection = LogInstance().PushBackCallback(
        [this](const std::string& s) {
            if (m_found) return;
            m_found = s.find(m_message) != std::string::npos && m_match(&s);
        });
    noui_test_redirect();
}

void DebugLogHelper::check_found()
{
    noui_reconnect();
    LogInstance().DeleteCallback(m_print_connection);
    if (!m_found && m_match(nullptr)) {
        throw std::runtime_error(strprintf("'%s' not found in debug log\n", m_message));
    }
}


Review comment (bug):
This commit mixes exception safety fixes with parameter optimization. Like @l0rinc mention perhaps consider splitting the parameter changes into a separate commit for cleaner git history and easier debugging.

Code after:
// Copyright (c) 2019-2021 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <test/util/logging.h>

#include <logging.h>
#include <noui.h>
#include <tinyformat.h>

#include <cstdlib>
#include <iostream>

DebugLogHelper::DebugLogHelper(std::string message, MatchFn match)
    : m_message{std::move(message)}, m_match(std::move(match))
{
    m_print_connection = LogInstance().PushBackCallback(
        [this](const std::string& s) {
            if (m_found) return;
            m_found = s.find(m_message) != std::string::npos && m_match(&s);
        });
    noui_test_redirect();
}

DebugLogHelper::~DebugLogHelper()
{
    noui_reconnect();
    LogInstance().DeleteCallback(m_print_connection);
    if (!m_found && m_match(nullptr)) {
        tfm::format(std::cerr, "Fatal error: expected message not found in the debug log: '%s'\n", m_message);
        std::abort();
    }
}
Repository: bitcoin/bitcoin
File: src/test/util/logging.cpp (C++)

Code before:
#include <test/util/logging.h>

#include <logging.h>
#include <noui.h>
#include <tinyformat.h>

#include <stdexcept>

DebugLogHelper::DebugLogHelper(std::string message, MatchFn match)
    : m_message{std::move(message)}, m_match(std::move(match))
{
    m_print_connection = LogInstance().PushBackCallback(
        [this](const std::string& s) {
            if (m_found) return;
            m_found = s.find(m_message) != std::string::npos && m_match(&s);
        });
    noui_test_redirect();
}

void DebugLogHelper::check_found()
{
    noui_reconnect();
    LogInstance().DeleteCallback(m_print_connection);
    if (!m_found && m_match(nullptr)) {
        throw std::runtime_error(strprintf("'%s' not found in debug log\n", m_message));
    }
}


Review comment (bug):
The original error had the message string in quotes. I think this should be preserved because it is easier to debug when the string contains whitespace at the end.

Code after:
#include <test/util/logging.h>

#include <logging.h>
#include <noui.h>
#include <tinyformat.h>

#include <cstdlib>
#include <iostream>

DebugLogHelper::DebugLogHelper(std::string message, MatchFn match)
    : m_message{std::move(message)}, m_match(std::move(match))
{
    m_print_connection = LogInstance().PushBackCallback(
        [this](const std::string& s) {
            if (m_found) return;
            m_found = s.find(m_message) != std::string::npos && m_match(&s);
        });
    noui_test_redirect();
}

DebugLogHelper::~DebugLogHelper()
{
    noui_reconnect();
    LogInstance().DeleteCallback(m_print_connection);
    if (!m_found && m_match(nullptr)) {
        tfm::format(std::cerr, "Fatal error: expected message not found in the debug log: '%s'\n", m_message);
        std::abort();
    }
}
Repository: bitcoin/bitcoin
File: src/test/util/logging.h (C/C++)

Code before:

class DebugLogHelper
{
    const std::string m_message;
    bool m_found{false};
    std::list<std::function<void(const std::string&)>>::iterator m_print_connection;

    //! Custom match checking function.
    //!
    //! Invoked with pointers to lines containing matching strings, and with
    //! null if check_found() is called without any successful match.
    //!
    //! Can return true to enable default DebugLogHelper behavior of:
    //! (1) ending search after first successful match, and
    //! (2) raising an error in check_found if no match was found
    //! Can return false to do the opposite in either case.
    using MatchFn = std::function<bool(const std::string* line)>;
    MatchFn m_match;

    void check_found();

public:
    explicit DebugLogHelper(std::string message, MatchFn match = [](const std::string*){ return true; });
    ~DebugLogHelper() noexcept(false) { check_found(); }
};

#define ASSERT_DEBUG_LOG(message) DebugLogHelper UNIQUE_NAME(debugloghelper)(message)

#endif // BITCOIN_TEST_UTIL_LOGGING_H


Review comment (suggestion):
nit/unrelated: I'm surprised to see an `std::list` here, `std::vector` is likely faster even for random erases

Code after:

class DebugLogHelper
{
public:
    //! Custom match checking function.
    //!
    //! Invoked with pointers to lines containing matching strings, and with
    //! null if check_found() is called without any successful match.
    //!
    //! Can return true to enable default DebugLogHelper behavior of:
    //! (1) ending search after first successful match, and
    //! (2) raising an error in check_found if no match was found
    //! Can return false to do the opposite in either case.
    using MatchFn = std::function<bool(const std::string* line)>;

    explicit DebugLogHelper(std::string message, MatchFn match = [](const std::string*){ return true; });

    DebugLogHelper(const DebugLogHelper&) = delete;
    DebugLogHelper& operator=(const DebugLogHelper&) = delete;

    ~DebugLogHelper();

private:
    const std::string m_message;
    bool m_found{false};
    std::list<std::function<void(const std::string&)>>::iterator m_print_connection;
    MatchFn m_match;
};

#define ASSERT_DEBUG_LOG(message) DebugLogHelper UNIQUE_NAME(debugloghelper)(message)

#endif // BITCOIN_TEST_UTIL_LOGGING_H
Repository: bitcoin/bitcoin
File: src/common/system.cpp (C++)

Code before:
#endif
}

bool SetupNetworking()
{
#ifdef WIN32
    // Initialize Windows Sockets
    WSADATA wsadata;
    int ret = WSAStartup(MAKEWORD(2,2), &wsadata);
    if (ret != NO_ERROR || LOBYTE(wsadata.wVersion ) != 2 || HIBYTE(wsadata.wVersion) != 2)
        return false;
#endif
    return true;
}

int GetNumCores()
{
    return std::thread::hardware_concurrency();
}

std::optional<size_t> GetTotalRAM()
{
    auto clamp{[](uint64_t v) { return size_t(std::min(v, uint64_t{std::numeric_limits<size_t>::max()})); }};
#ifdef WIN32
    if (MEMORYSTATUSEX m{}; (m.dwLength = sizeof(m), GlobalMemoryStatusEx(&m))) return clamp(m.ullTotalPhys);
#elif defined(__linux__) || defined(__APPLE__)
    if (long p{sysconf(_SC_PHYS_PAGES)}, s{sysconf(_SC_PAGESIZE)}; p > 0 && s > 0) return clamp(1ULL * p * s);
#endif
    return std::nullopt;
}

// Obtain the application startup time (used for uptime calculation)
int64_t GetStartupTime()
{
    return nStartupTime;
}


Review comment (suggestion):
```suggestion
#elif defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__) || defined(__OpenBSD__) || defined(__NetBSD__) || defined(__illumos__)
```

Code after:
#endif
}

bool SetupNetworking()
{
#ifdef WIN32
    // Initialize Windows Sockets
    WSADATA wsadata;
    int ret = WSAStartup(MAKEWORD(2,2), &wsadata);
    if (ret != NO_ERROR || LOBYTE(wsadata.wVersion ) != 2 || HIBYTE(wsadata.wVersion) != 2)
        return false;
#endif
    return true;
}

int GetNumCores()
{
    return std::thread::hardware_concurrency();
}

std::optional<size_t> GetTotalRAM()
{
    [[maybe_unused]] auto clamp{[](uint64_t v) { return size_t(std::min(v, uint64_t{std::numeric_limits<size_t>::max()})); }};
#ifdef WIN32
    if (MEMORYSTATUSEX m{}; (m.dwLength = sizeof(m), GlobalMemoryStatusEx(&m))) return clamp(m.ullTotalPhys);
#elif defined(__APPLE__) || \
      defined(__FreeBSD__) || \
      defined(__NetBSD__) || \
      defined(__OpenBSD__) || \
      defined(__illumos__) || \
      defined(__linux__)
    if (long p{sysconf(_SC_PHYS_PAGES)}, s{sysconf(_SC_PAGESIZE)}; p > 0 && s > 0) return clamp(1ULL * p * s);
#endif
    return std::nullopt;
}

// Obtain the application startup time (used for uptime calculation)
int64_t GetStartupTime()
{
    return nStartupTime;
}
Repository: bitcoin/bitcoin
File: src/test/system_ram_tests.cpp (C++)

Code before:
// Copyright (c) 2025-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
//

#include <common/system.h>
#include <test/util/setup_common.h>

#include <boost/test/unit_test.hpp>

#include <cstdint>
#include <optional>

BOOST_AUTO_TEST_SUITE(system_ram_tests)

BOOST_AUTO_TEST_CASE(total_ram)
{
    const auto total{GetTotalRAM()};
    if (!total) {
        BOOST_WARN_MESSAGE(false, "skipping total_ram: total RAM unknown");
        return;
    }

    BOOST_CHECK_GE(*total, 1000_MiB);

    if constexpr (SIZE_MAX == UINT64_MAX) {
        // Upper bound check only on 64-bit: 32-bit systems can reasonably have max memory,
        // but extremely large values on 64-bit likely indicate detection errors
        BOOST_CHECK_LT(*total, 10'000'000_MiB); // >10 TiB memory is unlikely

Review comment (suggestion):
```suggestion
// file COPYING or https://opensource.org/license/mit/.
```

Code after:
// Copyright (c) 2025-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or https://opensource.org/license/mit/.

#include <common/system.h>
#include <test/util/setup_common.h>

#include <boost/test/unit_test.hpp>

#include <cstdint>
#include <optional>

BOOST_AUTO_TEST_SUITE(system_ram_tests)

BOOST_AUTO_TEST_CASE(total_ram)
{
    const auto total{GetTotalRAM()};
    if (!total) {
        BOOST_WARN_MESSAGE(false, "skipping total_ram: total RAM unknown");
        return;
    }

    BOOST_CHECK_GE(*total, 1000_MiB);

    if constexpr (SIZE_MAX == UINT64_MAX) {
        // Upper bound check only on 64-bit: 32-bit systems can reasonably have max memory,
        // but extremely large values on 64-bit likely indicate detection errors
        BOOST_CHECK_LT(*total, 10'000'000_MiB); // >10 TiB memory is unlikely
    }
Repository: bitcoin/bitcoin
File: src/chain.cpp (C++)

Code before:
        sign = -1;
    }
    r = r * arith_uint256(params.nPowTargetSpacing) / GetBlockProof(tip);
    if (r.bits() > 63) {
        return sign * std::numeric_limits<int64_t>::max();
    }
    return sign * int64_t(r.GetLow64());
}

/** Find the last common ancestor two blocks have.
 *  Both pa and pb must be non-nullptr. */
const CBlockIndex* LastCommonAncestor(const CBlockIndex* pa, const CBlockIndex* pb) {
    if (pa->nHeight > pb->nHeight) {
        pa = pa->GetAncestor(pb->nHeight);
    } else if (pb->nHeight > pa->nHeight) {
        pb = pb->GetAncestor(pa->nHeight);
    }

    while (pa != pb && pa && pb) {
        pa = pa->pprev;
        pb = pb->pprev;
    }

    // Eventually all chain branches meet at the genesis block.
    assert(pa == pb);
    return pa;
}


Review comment (bug):
If I understand it correctly, we don't have to worry about dereferencing a `nullptr` here, because `pskip`  is only  `nullptr` for genesis, and if one of the blocks was genesis, we couldn't get to this spot.

Code after:
        r = from.nChainWork - to.nChainWork;
        sign = -1;
    }
    r = r * arith_uint256(params.nPowTargetSpacing) / GetBlockProof(tip);
    if (r.bits() > 63) {
        return sign * std::numeric_limits<int64_t>::max();
    }
    return sign * int64_t(r.GetLow64());
}

/** Find the last common ancestor two blocks have.
 *  Both pa and pb must be non-nullptr. */
const CBlockIndex* LastCommonAncestor(const CBlockIndex* pa, const CBlockIndex* pb) {
    // First rewind to the last common height (the forking point cannot be past one of the two).
    if (pa->nHeight > pb->nHeight) {
        pa = pa->GetAncestor(pb->nHeight);
    } else if (pb->nHeight > pa->nHeight) {
        pb = pb->GetAncestor(pa->nHeight);
    }
    while (pa != pb) {
        // Jump back until pa and pb have a common "skip" ancestor.
        while (pa->pskip != pb->pskip) {
            // This logic relies on the property that equal-height blocks have equal-height skip
            // pointers.
            Assume(pa->nHeight == pb->nHeight);
            Assume(pa->pskip->nHeight == pb->pskip->nHeight);
            pa = pa->pskip;
            pb = pb->pskip;
        }
        // At this point, pa and pb are different, but have equal pskip. The forking point lies in
        // between pa/pb on the one end, and pa->pskip/pb->pskip on the other end.
        pa = pa->pprev;
        pb = pb->pprev;
    }
    return pa;
}
Repository: bitcoin/bitcoin
File: src/node/types.h (C/C++)

Code before:
     * the chain tip changes and the previous template is no longer valid.
     *
     * A caller may not be interested in templates with higher fees, and
     * determining whether fee_threshold is reached is also expensive. So as
     * an optimization, when fee_threshold is set to MAX_MONEY (default), the
     * implementation is able to be much more efficient, skipping expensive
     * checks and only returning new templates when the chain tip changes.
     */
    CAmount fee_threshold{MAX_MONEY};
};

struct BlockCheckOptions {
    /**
     * Set false to omit the merkle root check
     */
    bool check_merkle_root{true};

    /**
     * Set false to omit the proof-of-work check
     */
    bool check_pow{true};
};
} // namespace node

#endif // BITCOIN_NODE_TYPES_H


Review comment (suggestion):
This isn't a method
```suggestion
 * Enum to signal to broadcast a local transaction.
```

Code after:
     * least fee_threshold sats higher than the current template, or unless
     * the chain tip changes and the previous template is no longer valid.
     *
     * A caller may not be interested in templates with higher fees, and
     * determining whether fee_threshold is reached is also expensive. So as
     * an optimization, when fee_threshold is set to MAX_MONEY (default), the
     * implementation is able to be much more efficient, skipping expensive
     * checks and only returning new templates when the chain tip changes.
     */
    CAmount fee_threshold{MAX_MONEY};
};

struct BlockCheckOptions {
    /**
     * Set false to omit the merkle root check
     */
    bool check_merkle_root{true};

    /**
     * Set false to omit the proof-of-work check
     */
    bool check_pow{true};
};

/**
 * How to broadcast a local transaction.
 * Used to influence `BroadcastTransaction()` and its callers.
 */
enum class TxBroadcast : uint8_t {
    /// Add the transaction to the mempool and broadcast to all peers for which tx relay is enabled.
    MEMPOOL_AND_BROADCAST_TO_ALL,
    /// Add the transaction to the mempool, but don't broadcast to anybody.
    MEMPOOL_NO_BROADCAST,
};

} // namespace node

#endif // BITCOIN_NODE_TYPES_H